Skip to content

Commit 9ab1357

Browse files
bwplotkabwplotka
authored
store: Fixed critical bug, when certain not-existing value queried was causing "invalid size" error. (#2393)
Reason why we could not reproduce it locally was that for most of non-existing value we were lucky that buffer was still long enough and we could read and decode some (malformed) variadic type. For certain rare cases, buffer was not long enough. Fixed and spotted thanks to amazing @mkabischev! * Added more regression tests for binary header. Without the fix it fails with: ``` header_test.go:154: header_test.go:154: exp: range not found got: get postings offset entry: invalid size ``` Signed-off-by: Bartlomiej Plotka <[email protected]>
1 parent c0b9179 commit 9ab1357

File tree

3 files changed

+118
-51
lines changed

3 files changed

+118
-51
lines changed

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ We use *breaking* word for marking changes that are not backward compatible (rel
2626
- [#2319](https://github.com/thanos-io/thanos/pull/2319) Query: fixed inconsistent naming of metrics.
2727
- [#2390](https://github.com/thanos-io/thanos/pull/2390) Store: Fixed bug which was causing all posting offsets to be used instead of 1/32 as it was meant.
2828
Added hidden flag to control this behavior.
29+
- [#2393](https://github.com/thanos-io/thanos/pull/2393) Store: Fixed bug causing certain not-existing label values queried to fail with "invalid-size" error from binary header.
2930

3031
### Added
3132

‎pkg/block/indexheader/binary_reader.go

+77-34
Original file line numberDiff line numberDiff line change
@@ -653,6 +653,18 @@ func (r BinaryReader) PostingsOffset(name string, value string) (index.Range, er
653653
return rngs[0], nil
654654
}
655655

656+
func skipNAndName(d *encoding.Decbuf, buf *int) {
657+
if *buf == 0 {
658+
// Keycount+LabelName are always the same number of bytes,
659+
// and it's faster to skip than parse.
660+
*buf = d.Len()
661+
d.Uvarint() // Keycount.
662+
d.UvarintBytes() // Label name.
663+
*buf -= d.Len()
664+
return
665+
}
666+
d.Skip(*buf)
667+
}
656668
func (r BinaryReader) postingsOffset(name string, values ...string) ([]index.Range, error) {
657669
rngs := make([]index.Range, 0, len(values))
658670
if r.indexVersion == index.FormatV1 {
@@ -679,76 +691,107 @@ func (r BinaryReader) postingsOffset(name string, values ...string) ([]index.Ran
679691
return nil, nil
680692
}
681693

682-
skip := 0
694+
buf := 0
683695
valueIndex := 0
684696
for valueIndex < len(values) && values[valueIndex] < e.offsets[0].value {
685697
// Discard values before the start.
686698
valueIndex++
687699
}
688700

689-
var tmpRngs []index.Range // The start, end offsets in the postings table in the original index file.
701+
var newSameRngs []index.Range // The start, end offsets in the postings table in the original index file.
690702
for valueIndex < len(values) {
691-
value := values[valueIndex]
703+
wantedValue := values[valueIndex]
692704

693-
i := sort.Search(len(e.offsets), func(i int) bool { return e.offsets[i].value >= value })
705+
i := sort.Search(len(e.offsets), func(i int) bool { return e.offsets[i].value >= wantedValue })
694706
if i == len(e.offsets) {
695707
// We're past the end.
696708
break
697709
}
698-
if i > 0 && e.offsets[i].value != value {
710+
if i > 0 && e.offsets[i].value != wantedValue {
699711
// Need to look from previous entry.
700712
i--
701713
}
714+
702715
// Don't Crc32 the entire postings offset table, this is very slow
703716
// so hope any issues were caught at startup.
704717
d := encoding.NewDecbufAt(r.b, int(r.toc.PostingsOffsetTable), nil)
705718
d.Skip(e.offsets[i].tableOff)
706719

707-
tmpRngs = tmpRngs[:0]
708720
// Iterate on the offset table.
721+
newSameRngs = newSameRngs[:0]
709722
for d.Err() == nil {
710-
if skip == 0 {
711-
// These are always the same number of bytes,
712-
// and it's faster to skip than parse.
713-
skip = d.Len()
714-
d.Uvarint() // Keycount.
715-
d.UvarintBytes() // Label name.
716-
skip -= d.Len()
717-
} else {
718-
d.Skip(skip)
723+
// Posting format entry is as follows:
724+
// │ ┌────────────────────────────────────────┐ │
725+
// │ │ n = 2 <1b> │ │
726+
// │ ├──────────────────────┬─────────────────┤ │
727+
// │ │ len(name) <uvarint> │ name <bytes> │ │
728+
// │ ├──────────────────────┼─────────────────┤ │
729+
// │ │ len(value) <uvarint> │ value <bytes> │ │
730+
// │ ├──────────────────────┴─────────────────┤ │
731+
// │ │ offset <uvarint64> │ │
732+
// │ └────────────────────────────────────────┘ │
733+
// First, let's skip n and name.
734+
skipNAndName(&d, &buf)
735+
value := d.UvarintBytes() // Label value.
736+
postingOffset := int64(d.Uvarint64())
737+
738+
if len(newSameRngs) > 0 {
739+
// We added some ranges in previous iteration. Use next posting offset as end of all our new ranges.
740+
for j := range newSameRngs {
741+
newSameRngs[j].End = postingOffset - crc32.Size
742+
}
743+
rngs = append(rngs, newSameRngs...)
744+
newSameRngs = newSameRngs[:0]
719745
}
720-
v := d.UvarintBytes() // Label value.
721-
postingOffset := int64(d.Uvarint64()) // Offset.
722-
for string(v) >= value {
723-
if string(v) == value {
724-
tmpRngs = append(tmpRngs, index.Range{Start: postingOffset + postingLengthFieldSize})
746+
747+
for string(value) >= wantedValue {
748+
// If wantedValue is equals of greater than current value, loop over all given wanted values in the values until
749+
// this is no longer true or there are no more values wanted.
750+
// This ensures we cover case when someone asks for postingsOffset(name, value1, value1, value1).
751+
752+
// Record on the way if wanted value is equal to the current value.
753+
if string(value) == wantedValue {
754+
newSameRngs = append(newSameRngs, index.Range{Start: postingOffset + postingLengthFieldSize})
725755
}
726756
valueIndex++
727757
if valueIndex == len(values) {
728758
break
729759
}
730-
value = values[valueIndex]
760+
wantedValue = values[valueIndex]
731761
}
762+
732763
if i+1 == len(e.offsets) {
733-
for i := range tmpRngs {
734-
tmpRngs[i].End = e.lastValOffset
764+
// No more offsets for this name.
765+
// Break this loop and record lastOffset on the way for ranges we just added if any.
766+
for j := range newSameRngs {
767+
newSameRngs[j].End = e.lastValOffset
735768
}
736-
rngs = append(rngs, tmpRngs...)
737-
// Need to go to a later postings offset entry, if there is one.
769+
rngs = append(rngs, newSameRngs...)
738770
break
739771
}
740772

741-
if value >= e.offsets[i+1].value || valueIndex == len(values) {
742-
d.Skip(skip)
743-
d.UvarintBytes() // Label value.
744-
postingOffset := int64(d.Uvarint64()) // Offset.
745-
for j := range tmpRngs {
746-
tmpRngs[j].End = postingOffset - crc32.Size
773+
if valueIndex != len(values) && wantedValue <= e.offsets[i+1].value {
774+
// wantedValue is smaller or same as the next offset we know about, let's iterate further to add those.
775+
continue
776+
}
777+
778+
// Nothing wanted or wantedValue is larger than next offset we know about.
779+
// Let's exit and do binary search again / exit if nothing wanted.
780+
781+
if len(newSameRngs) > 0 {
782+
// We added some ranges in this iteration. Use next posting offset as the end of our ranges.
783+
// We know it exists as we never go further in this loop than e.offsets[i, i+1].
784+
785+
skipNAndName(&d, &buf)
786+
d.UvarintBytes() // Label value.
787+
postingOffset := int64(d.Uvarint64())
788+
789+
for j := range newSameRngs {
790+
newSameRngs[j].End = postingOffset - crc32.Size
747791
}
748-
rngs = append(rngs, tmpRngs...)
749-
// Need to go to a later postings offset entry, if there is one.
750-
break
792+
rngs = append(rngs, newSameRngs...)
751793
}
794+
break
752795
}
753796
if d.Err() != nil {
754797
return nil, errors.Wrap(d.Err(), "get postings offset entry")

‎pkg/block/indexheader/header_test.go

+40-17
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ func TestReaders(t *testing.T) {
5555
{{Name: "a", Value: "12"}},
5656
{{Name: "a", Value: "13"}},
5757
{{Name: "a", Value: "1"}, {Name: "longer-string", Value: "1"}},
58+
{{Name: "a", Value: "1"}, {Name: "longer-string", Value: "2"}},
5859
}, 100, 0, 1000, labels.Labels{{Name: "ext1", Value: "1"}}, 124)
5960
testutil.Ok(t, err)
6061

@@ -110,12 +111,14 @@ func TestReaders(t *testing.T) {
110111
testutil.Equals(t, 1, br.version)
111112
testutil.Equals(t, 2, br.indexVersion)
112113
testutil.Equals(t, &BinaryTOC{Symbols: headerLen, PostingsOffsetTable: 69}, br.toc)
113-
testutil.Equals(t, int64(666), br.indexLastPostingEnd)
114+
testutil.Equals(t, int64(710), br.indexLastPostingEnd)
114115
testutil.Equals(t, 8, br.symbols.Size())
116+
testutil.Equals(t, 0, len(br.postingsV1))
117+
testutil.Equals(t, 2, len(br.nameSymbols))
115118
testutil.Equals(t, map[string]*postingValueOffsets{
116119
"": {
117120
offsets: []postingOffset{{value: "", tableOff: 4}},
118-
lastValOffset: 416,
121+
lastValOffset: 440,
119122
},
120123
"a": {
121124
offsets: []postingOffset{
@@ -125,15 +128,44 @@ func TestReaders(t *testing.T) {
125128
{value: "7", tableOff: 75},
126129
{value: "9", tableOff: 89},
127130
},
128-
lastValOffset: 612,
131+
lastValOffset: 640,
129132
},
130133
"longer-string": {
131-
offsets: []postingOffset{{value: "1", tableOff: 96}},
132-
lastValOffset: 662,
134+
offsets: []postingOffset{
135+
{value: "1", tableOff: 96},
136+
{value: "2", tableOff: 115},
137+
},
138+
lastValOffset: 706,
133139
},
134140
}, br.postings)
135-
testutil.Equals(t, 0, len(br.postingsV1))
136-
testutil.Equals(t, 2, len(br.nameSymbols))
141+
142+
vals, err := br.LabelValues("not-existing")
143+
testutil.Ok(t, err)
144+
testutil.Equals(t, []string(nil), vals)
145+
146+
// Regression tests for https://github.com/thanos-io/thanos/issues/2213.
147+
// Most of not existing value was working despite bug, except in certain unlucky cases
148+
// it was causing "invalid size" errors.
149+
_, err = br.PostingsOffset("not-existing", "1")
150+
testutil.Equals(t, NotFoundRangeErr, err)
151+
_, err = br.PostingsOffset("a", "0")
152+
testutil.Equals(t, NotFoundRangeErr, err)
153+
// Unlucky case, because the bug was causing unnecessary read & decode requiring more bytes than
154+
// available. For rest cases read was noop wrong, but at least not failing.
155+
_, err = br.PostingsOffset("a", "10")
156+
testutil.Equals(t, NotFoundRangeErr, err)
157+
_, err = br.PostingsOffset("a", "121")
158+
testutil.Equals(t, NotFoundRangeErr, err)
159+
_, err = br.PostingsOffset("a", "131")
160+
testutil.Equals(t, NotFoundRangeErr, err)
161+
_, err = br.PostingsOffset("a", "91")
162+
testutil.Equals(t, NotFoundRangeErr, err)
163+
_, err = br.PostingsOffset("longer-string", "0")
164+
testutil.Equals(t, NotFoundRangeErr, err)
165+
_, err = br.PostingsOffset("longer-string", "11")
166+
testutil.Equals(t, NotFoundRangeErr, err)
167+
_, err = br.PostingsOffset("longer-string", "21")
168+
testutil.Equals(t, NotFoundRangeErr, err)
137169
}
138170

139171
compareIndexToHeader(t, b, br)
@@ -151,7 +183,7 @@ func TestReaders(t *testing.T) {
151183
if id == id1 {
152184
testutil.Equals(t, 14, len(jr.symbols))
153185
testutil.Equals(t, 2, len(jr.lvals))
154-
testutil.Equals(t, 14, len(jr.postings))
186+
testutil.Equals(t, 15, len(jr.postings))
155187
}
156188

157189
compareIndexToHeader(t, b, jr)
@@ -251,15 +283,6 @@ func compareIndexToHeader(t *testing.T, indexByteSlice index.ByteSlice, headerRe
251283
testutil.Ok(t, err)
252284
testutil.Equals(t, expRanges[labels.Label{Name: "", Value: ""}].Start, ptr.Start)
253285
testutil.Equals(t, expRanges[labels.Label{Name: "", Value: ""}].End, ptr.End)
254-
255-
// Check not existing.
256-
vals, err := indexReader.LabelValues("not-existing")
257-
testutil.Ok(t, err)
258-
testutil.Equals(t, []string(nil), vals)
259-
_, err = headerReader.PostingsOffset("not-existing", "1")
260-
testutil.NotOk(t, err)
261-
_, err = headerReader.PostingsOffset("a", "10")
262-
testutil.NotOk(t, err)
263286
}
264287

265288
func prepareIndexV2Block(t testing.TB, tmpDir string, bkt objstore.Bucket) *metadata.Meta {

0 commit comments

Comments
 (0)