fix: panic index out of range for invalid series keys #24565
Merged
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
…to fix/tsm-out-of-range-index
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
jdockerty
force-pushed
the
fix/tsm-out-of-range-index
branch
from
1f3fb89
to
2556e38
Compare
There was a problem hiding this comment.
The overall comment I have is that the tests when ReadSeriesKeyLen return a zero size or an empty buffer should be stricter in preventing the next operation, but you need to be sure they are correct. For instance, can we have a key with no tags? What does that produce out of the various parsing functions? I believe it is possible, so depending on what happens after the parsing depends on what's being done; are we processing the tags, or only the key name?
So, take a look at each test you've added and see what comes next, then make the test as simple and robust as possible.
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
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
jdockerty
force-pushed
the
fix/tsm-out-of-range-index
branch
from
52b83b9
to
362468d
Compare
jdockerty
changed the title
davidby-influx
requested changes
Jan 17, 2024
davidby-influx
requested changes
Jan 17, 2024
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.
davidby-influx
previously approved these changes
Jan 19, 2024
There was a problem hiding this comment.
LGTM - pass on to @gwossum for a second clean review and prepare for all the ports (1.11, main-2.x, 2.7)
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.
…data/influxdb into fix/tsm-out-of-range-index
davidby-influx
previously approved these changes
Jan 19, 2024
There was a problem hiding this comment.
LGTM! Thanks for contributing. Please get a review from @gwossum and if he approves, create issues for ports to main-2.x, 2.7, and 1.11 branches, then cherry-pick into those and put up the PRs.
gwossum
reviewed
Jan 22, 2024
gwossum
reviewed
Jan 22, 2024
…data/influxdb into fix/tsm-out-of-range-index
jdockerty
added a commit
that referenced
this pull request
Jan 23, 2024
* 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
This was referenced Jan 23, 2024
jdockerty
added a commit
that referenced
this pull request
Jan 23, 2024
* 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
chengshiwen
pushed a commit
to chengshiwen/influxdb
that referenced
this pull request
Aug 11, 2024
* 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
chengshiwen
pushed a commit
to chengshiwen/influxdb
that referenced
this pull request
Aug 27, 2024
* 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
chengshiwen
pushed a commit
to chengshiwen/influxdb
that referenced
this pull request
Aug 28, 2024
* 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.
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: #24454, #24469, #24432, #17409, #20245, #23266