github-actions[bot] commented on code in PR #67013:
URL: https://github.com/apache/doris/pull/67013#discussion_r3828553386
##########
cloud/test/blob_message_test.cpp:
##########
@@ -259,6 +358,32 @@ TEST(BlobMessageTest, GetRangeWithTxnKv) {
}
}
+TEST(BlobMessageTest, BlobIteratorRejectsMalformedChunks) {
Review Comment:
[P2] Apply malformed-fragment validation to ordinary blob_get
These new checks exercise only `BlobIterator`. `ValueBuf::get` still decodes
the first suffix and then retains every KV in the range without validating
later versions or sequences, so `(v0, seq0)` plus `(v0, seq2)` or `(v1, seq1)`
returns `TXN_OK` and concatenates corrupted data. Please share the
origin/version/contiguous-sequence validation with `blob_get` and add
equivalent ordinary-reader cases.
##########
cloud/src/meta-store/blob_message.cpp:
##########
@@ -40,6 +42,15 @@ static std::vector<std::string_view> split_string(const
std::string_view& str, i
return substrings;
}
+std::string encode_blob_key(std::string_view origin_key, uint8_t version,
size_t sequence) {
+ std::string split_key(origin_key);
+ int64_t suffix = version;
+ suffix <<= 56;
Review Comment:
[P2] Preserve sequence order for high blob versions
The API now advertises the full `uint8_t` version range, but versions
129-255 produce negative suffixes. `encode_int64` stores a negative value as
tag `0x11` plus its absolute magnitude, so for version 255 sequence 1 sorts
before sequence 0. FDB therefore feeds `ValueBuf` chunks in reverse order,
while `BlobIterator` starts at the highest sequence and rejects the next lower
one. This remains after special-casing the existing version-128/`INT64_MIN`
overflow, so please either reject versions above 127 or use an order-preserving
suffix representation, with a multi-chunk high-version test through both
readers.
##########
cloud/test/blob_message_test.cpp:
##########
@@ -259,6 +358,32 @@ TEST(BlobMessageTest, GetRangeWithTxnKv) {
}
}
+TEST(BlobMessageTest, BlobIteratorRejectsMalformedChunks) {
+ auto expect_invalid = [](std::string_view origin_key,
+ const std::vector<std::string>& raw_keys) {
+ auto txn_kv = std::make_shared<MemTxnKv>();
+ ASSERT_EQ(txn_kv->init(), 0);
+ std::unique_ptr<Transaction> txn;
+ ASSERT_EQ(txn_kv->create_txn(&txn), TxnErrorCode::TXN_OK);
+ for (const auto& raw_key : raw_keys) {
+ txn->put(raw_key, "value");
+ }
+ ASSERT_EQ(txn->commit(), TxnErrorCode::TXN_OK);
+
+ auto iter = blob_get_range(txn_kv, origin_key, std::string(origin_key)
+ "\xff");
+ EXPECT_FALSE(iter->valid());
+ EXPECT_EQ(iter->error_code(), TxnErrorCode::TXN_INVALID_DATA);
+ };
+
+ expect_invalid("sequence_gap",
Review Comment:
[P2] Reject blobs whose first available chunk is not sequence zero
`load_current_blob` seeds `next_sequence` from the first persisted key, so a
range containing only `encode_blob_key("missing_prefix", 0, 1)` passes the
first comparison and is exposed as a complete blob with `TXN_OK`. The new `0,2`
case catches an internal gap but not loss of the leading chunk. Please require
the first decoded sequence to be zero and add this missing-prefix negative case.
##########
cloud/src/meta-store/blob_message.cpp:
##########
@@ -150,13 +161,10 @@ void blob_put(Transaction* txn, std::string_view key,
const google::protobuf::Me
void blob_put(Transaction* txn, std::string_view key, std::string_view value,
uint8_t ver,
size_t split_size) {
+ split_size = std::max(split_size, MIN_BLOB_SPLIT_SIZE);
auto split_vec = split_string(value, split_size);
- int64_t suffix_base = ver;
- suffix_base <<= 56;
for (size_t i = 0; i < split_vec.size(); ++i) {
- std::string k(key);
- encode_int64(suffix_base + i, &k);
- txn->put(k, split_vec[i]);
+ txn->put(encode_blob_key(key, ver, i), split_vec[i]);
Review Comment:
[P2] Remove old chunks before same-versionstamp rewrites
This loop only overwrites sequences emitted by the replacement value.
`convert_tmp_rowsets` reads an existing operation log, lowers its protobuf
`min_timestamp`, and calls the specified-versionstamp `blob_put` for the same
key without removing `log_value`. Because a lower uint64 varint can make a
large log cross from N chunks to N-1 at the 90 KB boundary, the old final chunk
survives and remains a contiguous same-version sequence, so both readers accept
corrupted protobuf bytes. Please have that caller retain and remove the loaded
`log_value` keys in the same transaction before rewriting, and add a shrinking
same-versionstamp test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
