Fix unbound read in Azure::readBigAt - #119054
Conversation
|
Workflow [PR], commit [a515da0] Summary: ✅
AI ReviewSummaryThis PR fixes Final VerdictStatus: ✅ Approve LLVM Coverage ReportMeasured on commit a515da0.
Changed lines: Changed C/C++ lines covered: 94/100 (94.00%) · Uncovered code |
clickhouse-gh
Bot
added
the
pr-bugfix
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
…g a full read `readBigAt` returned the requested size even when the response bodies kept ending prematurely and the retry loop ran out of attempts, so callers such as `Parquet::Prefetcher::readSync` could not detect the truncation. Return the accumulated byte count, like the S3 and HTTP implementations.
clickhouse-gh
Bot
added
the
comp-azure
Build profile diff (arm_release)Comparing ✅ No significant changes. Binary sizes
The official master build is compiled with Compile time of recompiled translation units13 translation units recompiled, 44 s compile time in total, 13 of them have a recent master baseline. Median compile-time ratio to the baselines is ×1.06 (machine-speed difference or a change affecting every TU); per-TU deltas below are relative to that ratio. |
… hit a null client `readBigAt` dereferenced `blob_client` unconditionally, while the client was only created lazily by `initialize` or `tryGetFileSize`, so a caller following the documented `supportsReadAt` handshake could hit a null pointer. Route every user through `getBlobClient`, which creates the client under `std::call_once`: `readBigAt` is `const` and may be called from multiple threads in parallel, and creating the client is cheap (it only composes the blob URL, no request is made). Also make the regression test exercise the documented handshake instead of forcing a `getFileSize` call.
The existing test only served responses at least as long as requested, so it exercised the overlong-body clamp but left the short-count return unproved: a regression to returning the requested size after a truncated Azure response would still have passed. Parameterize the mock transport by advertised vs. served size and add a case whose body ends early, asserting `readBigAt` returns the accumulated byte count and leaves the unread tail untouched.
robot-ch-test-poll
added
the
pr-synced-to-cloud
…-buffer-overflows Master merged #119054, an independent fix of the same `readBigAt` overflow in `ReadBufferFromAzureBlobStorage`. Resolution: - `ReadBufferFromAzureBlobStorage.cpp`: take master's thread-safe lazy `getBlobClient` (created under `call_once`) everywhere, which supersedes this branch's call-local blob client in `readBigAt`; keep this branch's `checkReturnedRange` / `checkReturnedETag`, `copyFromAzureBodyStream`, the null body stream guard and the short-read warning; keep master's `chassert(bytes_copied <= n)`. - `gtest_azure_read_buffer.cpp` (add/add): keep this branch's file and port master's two tests `AzureReadBigAt.DoesNotTrustResponseLength` and `AzureReadBigAt.ReturnsAccumulatedCountOnTruncatedResponse` onto its `makeFreshBuffer` harness. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
5 participants
Problem
The reported length of a response from blob storage was trusted when copying data, which could result in a heap buffer overflow.
Fix
The read is now limited by the local buffer capacity. Test coverage is added.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix a possible heap buffer overflow on malformed responses from blob storage.
Workflow [PR]
Sync PR [sync-upstream/pr/119054]
Version info
26.9.1.1439(included in26.9and later)