Repository navigation
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new test uses logging APIs unavailable with glog enabled, causing compilation failure in that configuration.
1 open issue
What changed in this PR
This PR hardens bRPC’s CRC32C verification against malformed checksum lengths and unaligned byte buffers while preserving valid-input behavior.
Changes:
- Reject checksum values that are not four bytes before computing the checksum.
- Decode checksum bytes with
memcpyinstead of a pointer cast. - Add regression coverage for invalid lengths, valid-value restoration, and absence of CHECK failures.
| File | Description |
|---|---|
test/brpc_checksum_unittest.cpp |
Adds malformed-length regression coverage and scoped log capture. |
src/brpc/policy/crc32c_checksum.cpp |
Validates checksum length and safely decodes checksum bytes. |
Review effort: Balanced
Comment on lines
+64
to
+79
| logging::StringSink logs; | ||
| { | ||
| logging::LogSink* previous = logging::SetLogSink(&logs); | ||
| BUTIL_SCOPE_EXIT { logging::SetLogSink(previous); }; | ||
| for (size_t size = 0; size <= 8; ++size) { | ||
| if (size == valid.size()) { | ||
| continue; | ||
| } | ||
| const std::string invalid(size, '\0'); | ||
| accessor.set_checksum_value(invalid.data(), invalid.size()); | ||
| EXPECT_FALSE(brpc::policy::Crc32cVerify(in)) << "size=" << size; | ||
| } | ||
| accessor.set_checksum_value(valid.data(), valid.size()); | ||
| EXPECT_TRUE(brpc::policy::Crc32cVerify(in)); | ||
| } | ||
| EXPECT_EQ(std::string::npos, logs.find("Check failed: val.size()")); |
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.
What problem does this PR solve?
Problem Summary:
CRC32C verification assumes a four-byte checksum and reads it through a uint32_t pointer. Invalid lengths reach a CHECK, and the byte buffer need not provide uint32_t alignment.
What is changed and the side effects?
Changed:
Side effects:
Check List: