Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: avoid panic when reading corrupt index in TSM file #24033

Open
wants to merge 2 commits into
base: gw-master-1.x-24033
Choose a base branch
from
Open

fix: avoid panic when reading corrupt index in TSM file #24033

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4233042
fix: for critical issue #19916 (influxdb server crashed when reading …
gggevorgyan Jan 11, 2023
96fa817
fix: Improved error message to reflect count in it
Feb 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion tsdb/engine/tsm1/reader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1233,6 +1233,10 @@ func (d *indirectIndex) UnmarshalBinary(b []byte) error {
return fmt.Errorf("indirectIndex: not enough data for index entries count")
}
count := int32(binary.BigEndian.Uint16(b[i : i+indexCountSize]))

if count <= 0 {
return fmt.Errorf("invalid index %d; count must be greater than 0", count)
}
Comment on lines +1237 to +1239
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no way for count to be less than 0. All possible values of a uint16 will fit in an int32 and since uint16 is only positive, count will never be less than 0.

count could be 0, though, and we definitely don't want to continue further in this case. It's possible count == 0 is valid, but having logged is good for now.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fundamental solution for this case which I have considered was to add CRC for whole Index block as well, as it is done for other parts. But the solution obviously will break backward compatibility.
Are there any mechanism allowing to have support of different incompatible versions of *tsm file, with special hangling?
Another thing I have taken account during implementation is the following check in writer code tsdb/engine/tsm1/writer.go: 708 line , which skips count == 0 cases.

func (t *tsmWriter) WriteIndex() error {
	indexPos := t.n

	if t.index.KeyCount() == 0 {
		return ErrNoValues
	}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is unlikely we will introduce an incompatible TSM format at this point in the product's lifecycle.

i += indexCountSize

// Find the min time for the block
Expand Down Expand Up @@ -1350,7 +1354,7 @@ func (m *mmapAccessor) init() (*indirectIndex, error) {

indexOfsPos := len(m.b) - 8
indexStart := binary.BigEndian.Uint64(m.b[indexOfsPos : indexOfsPos+8])
if indexStart >= uint64(indexOfsPos) {
if indexStart >= uint64(indexOfsPos) || (uint64(indexOfsPos)-indexStart) > math.MaxInt32 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added check ensures that index can be accessed using signed 32-bit offsets. Since int32 index offsets are used internally (see indirectIndex.UnmarshalBinary), I think this is a correct check. I would prefer a distinct error message for this case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does look like the correct check. The error message (all the error messages in this method) should include the file name, which we have through m.f.Name()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the most effective way to wrap the file name onto error massage is the line 240 in tsdb/engine/tsm1/reader.go.
What go you think @davidby-influx ?

index, err := t.accessor.init()
	if err != nil {
		**_return nil, err_**
	}

return nil, fmt.Errorf("mmapAccessor: invalid indexStart")
}

Expand Down