Add sanity checks for writing number in variable length format#47608
Merged
robot-ch-test-poll3 merged 2 commits intoClickHouse:masterfrom Mar 21, 2023
Merged
Add sanity checks for writing number in variable length format#47608robot-ch-test-poll3 merged 2 commits intoClickHouse:masterfrom
robot-ch-test-poll3 merged 2 commits intoClickHouse:masterfrom
Conversation
rschu1ze
reviewed
Mar 15, 2023
rschu1ze
approved these changes
Mar 15, 2023
And just to double check:
# var_uint 9223372036854775807
ffffffffffffffff7f
ffffffffffffffff7f
ffffffffffffffff7f
x: 9223372036854775807, y: 9223372036854775807
# var_uint 9223372036854775808
808080808080808080
808080808080808080
808080808080808080
x: 9223372036854775808, y: 0
Signed-off-by: Azat Khuzhin <[email protected]>
Signed-off-by: Azat Khuzhin <[email protected]>
Member
Author
|
Same here, all CI is green, but MergeOnApproval found something: But likely this is another problem, looks like here, MergeOnApproval was run just too early and was not restarted for other jobs attempts, @Felixoid maybe you will be interesting. |
Member
|
I use any review.status in the code, but it actually should use only valid status. I'll try to address in #47789 |
Member
|
BTW, the approval is done before the last changes, so it is supposed to not be merged The merge_pr.py is executed in the FinishCheck too |
Member
Author
Oh, I see, then sorry for the noise. |
Member
|
The value of the review status is definitely wrong, and I've found it here. So it was useful 👍 |
alexey-milovidov
approved these changes
Mar 21, 2023
rschu1ze
added a commit
to rschu1ze/ClickHouse
that referenced
this pull request
Jul 6, 2023
Resolves: ClickHouse#51486 Until now, it was illegal to encode 64-bit (unsigned) integers with MSB=1, i.e. values > (1ULL<<63) - 1, as var-int. In more detail, the var-int code used by ClickHouse server and client spent at most 9 bytes per value such that 9 * 7 = 63 bits could be encoded. Some 3rd party clients (e.g. Rust clickhouse-rs) had the same limitation, whereas other clients understand the full range (Python clickhouse-driver). PRs ClickHouse#47608 and ClickHouse#48628 added sanity checks as asserts or exceptions during var-int encoding on the server side. This was considered okay as such huge integers so far occurred only during testing (usually fuzzing) but not in practice. Issue ClickHouse#51486 is a new fuzzing issue where the exception thrown from the sanity check led to a half-baked progress packet and as a result, a logical error / server crash. The only fix which is not another bandaid is to allow the full range in var-int coding. Clients will have to allow the full range too, a note will be added to the changelog. (the alternative was to create another protocol version but as var-int is used all over the place this was considered infeasible) Review note: this is the relevant commit.
This file contains hidden or 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
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.
And just to double check:
Changelog category (leave one):