[fix](tde) fix issues related to TDE (#55692) #55950
Conversation
luwei16
requested review from
CalvinKirs,
dataroaring,
gavinchou and
morningman
as code owners
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
github-actions
bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
dataroaring
left a comment
dataroaring
left a comment
There was a problem hiding this comment.
CRITICAL: Buffer Overflow Risk
This code has a critical buffer overflow vulnerability:
std::vector<uint8_t> magic_code_buf;
magic_code_buf.reserve(sizeof(uint64_t)); // ❌ CRITICAL BUG
Slice magic_code(magic_code_buf.data(), sizeof(uint64_t));Issue: reserve() only allocates capacity but doesn't resize the vector. The vector size remains 0, so data() points to uninitialized memory.
Fix: Use resize() instead:
std::vector<uint8_t> magic_code_buf;
magic_code_buf.resize(sizeof(uint64_t)); // ✅ CORRECT
Slice magic_code(magic_code_buf.data(), sizeof(uint64_t));This must be fixed before merging as it could cause crashes or undefined behavior.
|
Hardcoded Magic Numbers Need Documentation File: std::vector<uint8_t> answer = {'A', 'B', 'C', 'D', 'E', 'A', 'B', 'C'};Issue: Magic byte sequence is hardcoded without explanation or named constants. Problems:
Recommendation: Define as named constants with documentation: // Magic bytes used to identify encrypted tablet segments
static constexpr std::array<uint8_t, 8> ENCRYPTION_MAGIC_BYTES =
{'A', 'B', 'C', 'D', 'E', 'A', 'B', 'C'};
std::vector<uint8_t> answer(ENCRYPTION_MAGIC_BYTES.begin(), ENCRYPTION_MAGIC_BYTES.end());This improves code readability and maintainability. |
github-actions
bot
removed
the
approved
|
Missing Header Guards File: Issue: This header file lacks proper include guards, which can cause multiple inclusion problems during compilation. Risk:
Fix: Add header guards at the beginning of the file: #pragma once
// OR traditional guards:
#ifndef BE_SRC_HTTP_ACTION_CHECK_ENCRYPTION_ACTION_H
#define BE_SRC_HTTP_ACTION_CHECK_ENCRYPTION_ACTION_H
// ... existing content ...
#endif // BE_SRC_HTTP_ACTION_CHECK_ENCRYPTION_ACTION_HRecommend using |
|
Remove Commented Code Block File: Issue: Large blocks of commented-out code are present, particularly around lines 1903-1912 in PropertyAnalyzer.java. Problems:
Recommendation:
Example: // Remove these commented blocks:
// if (properties.containsKey(PropertyAnalyzer.PROPERTIES_COLOCATE_WITH)) {
// ... (multiple lines of commented code)
// If truly needed for future work, replace with:
// TODO: Implement colocate_with property validation for TDE scenarios (see commit ABC123) |
|
Question: Privilege Type Consistency File: Issue: The new encryption check endpoint uses Code: Status CheckEncryptionAction::check_tablet_encryption_request(HttpRequest* req, TCheckTabletEncryptionReq& request) {
// Uses TPrivilegeType::ALLQuestions:
Security Consideration:
Recommendation: Please clarify the intended privilege level and ensure it aligns with the security model for TDE-related operations. |
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 35601 ms |
TPC-DS: Total hot run time: 189363 ms |
ClickBench: Total hot run time: 30.41 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run cloud_p0 |
|
run nonConcurrent |
|
run p0 |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
1 similar comment
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
1 similar comment
FE Regression Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run cloud_p0 |
FE Regression Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
github-actions
bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
6 participants
pick 3.1 #55692