fix: avoid SIGBUS when reading non-std series segment files #24509
Conversation
Some series files which are smaller than the standard sizes cause SIGBUS in influx_inspect and influxd, because entry iteration walks onto mapped memory not backed by the the file. Avoid walking off the end of the file while iterating series entries in oddly sized files. closes #24508
tsdb/series_segment.go
Outdated
| // Only recalculate segment data size if writing. | ||
| var size uint32 | ||
| for size = uint32(SeriesSegmentHeaderSize); size < s.size; { | ||
| flag, _, _, sz := ReadSeriesEntry(s.data[size:]) |
There was a problem hiding this comment.
ReadSeriesEntry could read beyond the end of the file into the rounded-up page or into SIGBUS land here. Even if the slice is bounded to the physical size of the file (s.data[size:s.size]) you could get an out of bounds panic from ReadSeriesEntry because it does not perform bounds checking.
There was a problem hiding this comment.
Good catch. Fix coming.
There was a problem hiding this comment.
That panic could only happen in a corrupt file, I think, right?
There was a problem hiding this comment.
I now think that this can't happen. Every place we call ReadSeriesEntry we know we have at least one byte in the array.
influxdb/tsdb/series_segment.go
Line 254 in e82ffeb
influxdb/tsdb/series_segment.go
Line 131 in e82ffeb
We will crash with a bounds check in binary.BigEndian.Uint64 if we don't have enough bytes, or in ReadSeriesKey but I don't see how to avoid that, because we don't know how many bytes we need to unpack to get the uint64 in binary.Uvarint, but I think that's an inevitable fact of a corrupt file.
There was a problem hiding this comment.
In fact, I can't always pass a bounded slice in. If I pass a slice bounded by s.size in here I get an out of bounds from the underlying binary decoding of the Uint64, when I run the PartialWrite test.
-- FAIL: TestSeriesSegment_PartialWrite (0.00s)
panic: runtime error: index out of range [7] with length 7 [recovered]
panic: runtime error: index out of range [7] with length 7
goroutine 41420 [running]:
testing.tRunner.func1.2({0x14c6fc0, 0xc000fd0120})
/home/davidby/.gvm/gos/go1.20.10/src/testing/testing.go:1526 +0x24e
testing.tRunner.func1()
/home/davidby/.gvm/gos/go1.20.10/src/testing/testing.go:1529 +0x39f
panic({0x14c6fc0, 0xc000fd0120})
/home/davidby/.gvm/gos/go1.20.10/src/runtime/panic.go:884 +0x213
encoding/binary.bigEndian.Uint64(...)
/home/davidby/.gvm/gos/go1.20.10/src/encoding/binary/binary.go:179
github.com/influxdata/influxdb/tsdb.ReadSeriesEntry({0x7f100c400013?, 0xc000c90df8?, 0xe93dbe?})
/home/davidby/plut/go/src/github.com/influxdata/influxdb/tsdb/series_segment.go:436 +0x105
github.com/influxdata/influxdb/tsdb.(*SeriesSegment).InitForWrite(0xc0000e5f08)
/home/davidby/plut/go/src/github.com/influxdata/influxdb/tsdb/series_segment.go:132 +0x6c
github.com/influxdata/influxdb/tsdb_test.TestSeriesSegment_PartialWrite(0xc000ff89c0)
/home/davidby/plut/go/src/github.com/influxdata/influxdb/tsdb/series_segment_test.go:179 +0x545
There was a problem hiding this comment.
Yes, the binary calls do bounds checking, but the slice operations do not. It seems like something is really wrong though if limiting the slice to the s.size causes a test to break. s.size is how big we think the file on disk actually is. We shouldn't go past where we think the end of the file is. Either we do go past the end, which is bad, or the test has a bug.
There was a problem hiding this comment.
The test is about partial writes, so I think it is a deliberate attempt to have a too-short file. But, it doesn't generate a SIGBUS, so I'm not sure if it is walking off the end of the file, or whether I am putting the wrong limit on the slice size.
Some series files which are smaller than the standard sizes cause SIGBUS in influx_inspect and influxd, because entry iteration walks onto mapped memory not backed by the the file. Avoid walking off the end of the file while iterating series entries in oddly sized files. closes #24508 Co-authored-by: Geoffrey Wossum <[email protected]> (cherry picked from commit 969abf3) closes #24510
…24515) Some series files which are smaller than the standard sizes cause SIGBUS in influx_inspect and influxd, because entry iteration walks onto mapped memory not backed by the the file. Avoid walking off the end of the file while iterating series entries in oddly sized files. closes #24508 Co-authored-by: Geoffrey Wossum <[email protected]> (cherry picked from commit 969abf3) closes #24510
Some series files which are smaller than the standard sizes cause SIGBUS in influx_inspect and influxd, because entry iteration walks onto mapped memory not backed by the the file. Avoid walking off the end of the file while iterating series entries in oddly sized files. closes #24508 Co-authored-by: Geoffrey Wossum <[email protected]> (cherry picked from commit 969abf3) closes #24511
…24520) Some series files which are smaller than the standard sizes cause SIGBUS in influx_inspect and influxd, because entry iteration walks onto mapped memory not backed by the the file. Avoid walking off the end of the file while iterating series entries in oddly sized files. closes #24508 Co-authored-by: Geoffrey Wossum <[email protected]> (cherry picked from commit 969abf3) closes #24511
…24520) Some series files which are smaller than the standard sizes cause SIGBUS in influx_inspect and influxd, because entry iteration walks onto mapped memory not backed by the the file. Avoid walking off the end of the file while iterating series entries in oddly sized files. closes #24508 Co-authored-by: Geoffrey Wossum <[email protected]> (cherry picked from commit 969abf3) closes #24511 (cherry picked from commit 081f951) closes #24512
…24520) (#24521) Some series files which are smaller than the standard sizes cause SIGBUS in influx_inspect and influxd, because entry iteration walks onto mapped memory not backed by the the file. Avoid walking off the end of the file while iterating series entries in oddly sized files. closes #24508 Co-authored-by: Geoffrey Wossum <[email protected]> (cherry picked from commit 969abf3) closes #24511 (cherry picked from commit 081f951) closes #24512
…ta#24509) Some series files which are smaller than the standard sizes cause SIGBUS in influx_inspect and influxd, because entry iteration walks onto mapped memory not backed by the the file. Avoid walking off the end of the file while iterating series entries in oddly sized files. closes influxdata#24508 Co-authored-by: Geoffrey Wossum <[email protected]>
…ta#24509) Some series files which are smaller than the standard sizes cause SIGBUS in influx_inspect and influxd, because entry iteration walks onto mapped memory not backed by the the file. Avoid walking off the end of the file while iterating series entries in oddly sized files. closes influxdata#24508 Co-authored-by: Geoffrey Wossum <[email protected]>
Some series files which are smaller than the standard sizes
cause SIGBUS in
influx_inspectandinfluxd, becauseentry iteration walks onto mapped memory not backed by
the file. Avoid walking off the end of the file while iterating
series entries in oddly sized files.
closes #24508