fix: panic index out of range for invalid series keys [Port to 1.11] #24595
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* chore: add scaffolding for naive solution * feat: test case scaffolding * fix: implement check for series key before proceeding * fix: add validation for ReadSeriesKeyMeasurement usage * refactor: explicit use of series key len * feat: add remaining check to index * feat: add check to remaining files As the Len function is used as part of the parseSeriesKey, this also needs to be accounted for on the nil return from this function as it is used in different contexts * feat: expand test cases * chore: go fmt * chore: update test failure message * chore: impl feedback on unnecessary sz checks * feat: expand test cases * fix: nil series key check In both sections for index.go there is a pre-existing length check against the series key which should catch invalid values, perhaps this explains why it hasn't cropped up in the reported panics. For even more safety, we can also skip a nil key because we know that subsequent calls will cause a panic where this key is attempted to be used * fix: remove nil tags check A key with no tags is valid, so we should not check for BOTH nil key and tags as a key could be nil, which is invalid, yet still have tags and therefore cause the check to pass which we do not want * feat: extend test cases from feedback * fix: extend checks for CompareSeriesKeys * feat: add nilKeyHandler for shared key checking logic * fix: logical error in nilKeyHandler Prior to this, the else was always defaulted to at the end of the conditional branch, which causes unexpected behaviour and a failure of a bunch of tests. * fix: return tags keep nil data In a recent change to this, we agreed on a simple name == nil check for the actual data. As a follow on to this, I just realised that we don't actually want to nil back the tags, even if they're not checked, because having no tags is a valid input so we can simply return whatever we were passed unchanged. * fix: use len == 0 for extra safety * feat: extra test for blank series key
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a cherry-pick of 6af0be9 (introduced in #24565) for a port to 1.11.
Original pull request comment:
Panics can be avoided by checking the validity of the return from
ReadSeriesKeyLen, at the moment the size check resulting from this function is entirely discarded. The contained call tobinary.Uvarintcontains an implicit error, depending on the returned value:In every occurrence of
ReadSeriesKeyMeasurement, theReadSeriesKeyLenfunction is called prior to it, determined by glancing over the output ofrg "ReadSeriesKeyMeasurement" -B 5across the full codebase.This PR refactors the flow slightly to include
ReadSeriesKeyLenreturned value checks and the appropriate action to avoid panicking when trying to access memory out of bounds, it has a knock-on effect such that theParseSeriesKeyfunction can returnnilat another point which indicates an invalid result and the appropriate action should be taken.Closes #24593