Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix crash for uploading parts which size is greater then INT_MAX to S3 #47693

Merged
merged 6 commits into from
Apr 17, 2023
Merged

Fix crash for uploading parts which size is greater then INT_MAX to S3 #47693

merged 6 commits into from
Apr 17, 2023

Conversation

azat
Copy link
Collaborator

@azat azat commented Mar 17, 2023 •
edited

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in official stable or prestable release)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Fix crash for uploading parts which size is greater then INT_MAX to S3

The problem was that std::stringstream from libc++ crashes when you have > INT_MAX bytes there (after calling gcount()).

Refs: ClickHouse/llvm-project#9
Refs: ClickHouse/llvm-project#16
Fixes: #47679
Reverts: #48816
Cc: @rschu1ze

@azat azat changed the title Fix crash for uploading parts > INT_MAX to S3 Fix crash for uploading parts which size is greater then INT_MAX to S3 Mar 17, 2023
@robot-ch-test-poll1 robot-ch-test-poll1 added pr-bugfix Pull request with bugfix, not backported by default submodule changed At least one submodule changed in this PR. labels Mar 17, 2023
@alexey-milovidov
Copy link
Member

@azat a submodule is updated by mistake.

@alexey-milovidov
Copy link
Member

stringstream, as well as iostreams - are not allowed in the ClickHouse source code.

@alexey-milovidov
Copy link
Member

Parts should be uploaded in a streaming fashion without extra buffering or copying.

@azat
Copy link
Collaborator Author

azat commented Mar 20, 2023

a submodule is updated by mistake.

It was done by intention, since submodule update contain a fix for the std::stringstream - ClickHouse/llvm-project#9 (but as comment in the header of the PR states it also contains few unrelated changes because HEAD was not pointed correctly before ClickHouse/llvm-project#10)

stringstream, as well as iostreams - are not allowed in the ClickHouse source code.

I'm not the fan of them either, but aws-cpp-sdk accepts them.

Parts should be uploaded in a streaming fashion without extra buffering or copying.

By default they "do" (as you can see the test sets s3_max_single_part_upload_size to disable this).

But recently I've played with R2 storage, and it does not work nicely with multi-part upload, I've tried to upload everything in one part and server crashed, and after some digging I found this bug in the libc++ implementation of std::stringstream when there is more then INT_MAX bytes.

Also default size of s3_max_upload_part_size is 5GiB, which can crash the server with default settings. And frankly speaking this does not looks like a sane default.

@azat azat force-pushed the stringstream-INT_MAX branch 2 times, most recently from e7d69bc to 5c701b0 Compare March 20, 2023 17:47
@rschu1ze rschu1ze self-assigned this Mar 21, 2023
@alexey-milovidov alexey-milovidov marked this pull request as ready for review April 15, 2023 22:00
@alexey-milovidov
Copy link
Member

Changes to the LLVM submodule are merged, let's update and merge this PR.

@azat
Add a test for stringstream with INT_MAX
9ea4a55 
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
@azat
Add a test with S3 single part upload that exceeds INT_MAX
9171332 
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
@azat
Decrease part size in 02700_s3_part_INT_MAX
8dcee4c 
Seems that the IO is too slow [1].

  [1]: https://s3.amazonaws.com/clickhouse-test-reports/47693/6d869e3b0d3e439260eb54c32e3fa99acecae063/stateless_tests_flaky_check__asan_.html

Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
@azat
Avoid using s3_truncate_on_insert in 02700_s3_part_INT_MAX
f55f33b 
Seems that version of MinIO that is used on CI is too slow for this [1],
I've tried the same locally with RELEASE.2023-02-10T18-48-39Z and
everythings works OK, but I have NVME locally and more recent MinIO.

  [1]: https://s3.amazonaws.com/clickhouse-test-reports/47693/5c701b090c3ec10cb2a4a708b60d364eb02192fe/stateless_tests_flaky_check__asan_.html

Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
@azat
Remove quirk for s3_max_single_part_upload_size (when it exceed INT_MAX)
f5c3f3a 
Reverts: ClickHouse#48816
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
@azat
Update llvm to fix std::stringstream for payload > INT_MAX
fff6062 
Refs: https://reviews.llvm.org/D146294
Refs: ClickHouse/llvm-project#9
Refs: ClickHouse/llvm-project#16
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
@azat
Copy link
Collaborator Author

azat commented Apr 17, 2023

Bump llvm

@rschu1ze it still had some conflicts, so I'd rebased on top of upstream/master and updated the submodule. And also I'd reverted #48816

@azat
Copy link
Collaborator Author

azat commented Apr 17, 2023

Integration tests (asan) [3/6] — fail: 1, passed: 231, flaky: 0

  • test_backup_restore_on_cluster/test_disallow_concurrency.py::test_concurrent_restores_on_same_node - is flaky

@rschu1ze rschu1ze merged commit d658305 into ClickHouse:master Apr 17, 2023
136 checks passed
@azat azat deleted the stringstream-INT_MAX branch April 18, 2023 08:53
@CheSema CheSema mentioned this pull request May 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alexey-milovidov alexey-milovidov

Labels
pr-bugfix Pull request with bugfix, not backported by default submodule changed At least one submodule changed in this PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SIGSEGV while uploading to S3 for part > INT_MAX
4 participants
@azat @alexey-milovidov @robot-ch-test-poll1 @rschu1ze