Skip to content

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

Merged
rschu1ze merged 6 commits intoClickHouse:masterfrom
azat:stringstream-INT_MAX
Apr 17, 2023
Merged

Fix crash for uploading parts which size is greater then INT_MAX to S3#47693
rschu1ze merged 6 commits intoClickHouse:masterfrom
azat:stringstream-INT_MAX

Conversation

@azat
Copy link
Copy Markdown
Member

@azat azat commented Mar 17, 2023 •
edited
Loading

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
@azat azat force-pushed the stringstream-INT_MAX branch from 9f5beae to 6d869e3 Compare March 17, 2023 18:41
@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Mar 17, 2023

@azat a submodule is updated by mistake.

@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Mar 17, 2023

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

@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Mar 17, 2023

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

@azat
Copy link
Copy Markdown
Member 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
Copy Markdown
Member

alexey-milovidov commented Apr 15, 2023

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

azat added 6 commits April 17, 2023 10:46
@azat
Add a test for stringstream with INT_MAX
9ea4a55 
Signed-off-by: Azat Khuzhin <[email protected]>
@azat
Add a test with S3 single part upload that exceeds INT_MAX
9171332 
Signed-off-by: Azat Khuzhin <[email protected]>
@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 <[email protected]>
@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 <[email protected]>
@azat
Remove quirk for s3_max_single_part_upload_size (when it exceed INT_MAX)
f5c3f3a 
Reverts: ClickHouse#48816
Signed-off-by: Azat Khuzhin <[email protected]>
@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 <[email protected]>
@azat azat force-pushed the stringstream-INT_MAX branch from b690664 to fff6062 Compare April 17, 2023 08:50
@azat
Copy link
Copy Markdown
Member 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
Copy Markdown
Member 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
@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