Skip to content

Add sanity checks for writing number in variable length format#47608

Merged
robot-ch-test-poll3 merged 2 commits intoClickHouse:masterfrom
azat:varuint
Mar 21, 2023
Merged

Add sanity checks for writing number in variable length format#47608
robot-ch-test-poll3 merged 2 commits intoClickHouse:masterfrom
azat:varuint

Conversation

@azat
Copy link
Copy Markdown
Member

@azat azat commented 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

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

@robot-clickhouse-ci-1 robot-clickhouse-ci-1 added the pr-not-for-changelog This PR should not be mentioned in the changelog label Mar 15, 2023
@rschu1ze rschu1ze self-assigned this Mar 15, 2023
azat added 2 commits March 15, 2023 13:08
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]>
@azat
Copy link
Copy Markdown
Member Author

azat commented Mar 16, 2023

Same here, all CI is green, but MergeOnApproval found something:

2023-03-15 12:13:43,449 The PR #47608 has more than one workflows in progress, check URLs:
https://github.com/ClickHouse/ClickHouse/actions/runs/4426140003
https://github.com/ClickHouse/ClickHouse/actions/runs/4426137158

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.

@Felixoid
Copy link
Copy Markdown
Member

I see https://api.github.com/repos/ClickHouse/ClickHouse/pulls/47608/reviews

I use any review.status in the code, but it actually should use only valid status. I'll try to address in #47789

@Felixoid
Copy link
Copy Markdown
Member

BTW, the approval is done before the last changes, so it is supposed to not be merged

python merge_pr.py --dry-run --pr 47608 --check-approved --token=$GITHUB_TOKEN     
2023-03-20 18:07:25,229 Going to process PR #47608 in repo ClickHouse/ClickHouse
2023-03-20 18:07:26,846 Checking that all PR's statuses are green
2023-03-20 18:07:27,771 Checking the PR for approvals
2023-03-20 18:07:28,792 The following users have reviewed the PR:
  rschu1ze: APPROVED
  azat: COMMENTED
2023-03-20 18:07:28,792 The following users from core team approved the PR: rschu1ze
2023-03-20 18:07:29,038 The PR is approved at 2023-03-15T11:09:40
2023-03-20 18:07:29,038 There are changes after approve at 2023-03-15T11:09:40
2023-03-20 18:07:29,038 We don't merge the PR

The merge_pr.py is executed in the FinishCheck too

@azat
Copy link
Copy Markdown
Member Author

azat commented Mar 20, 2023

BTW, the approval is done before the last changes, so it is supposed to not be merged

Oh, I see, then sorry for the noise.

@Felixoid
Copy link
Copy Markdown
Member

The value of the review status is definitely wrong, and I've found it here. So it was useful 👍

@alexey-milovidov alexey-milovidov self-assigned this Mar 21, 2023
@robot-ch-test-poll3 robot-ch-test-poll3 merged commit 01df100 into ClickHouse:master Mar 21, 2023
@azat azat deleted the varuint branch March 21, 2023 10:29
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants