This is an automated email from the ASF dual-hosted git repository.
gavinchou pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new d11c5e7e794 [fix](cloud) Decode transaction IDs safely through
Versionstamp (#67641)
d11c5e7e794 is described below
commit d11c5e7e794182cd9284f356337265929b22d231
Author: Gavin Chou <[email protected]>
AuthorDate: Mon Sep 14 20:26:28 2026 +0800
[fix](cloud) Decode transaction IDs safely through Versionstamp (#67641)
### What problem does this PR solve?
Problem Summary:
`get_txn_id_from_fdb_ts` decodes a character buffer through `int64_t*`
dereferences, including an overlapping read at offset 2. These reads
violate alignment and aliasing requirements.
- Copy the 10 input bytes into a byte array and use
`Versionstamp::version()` / `order()` to decode the big-endian fields in
one place.
- Preserve the transaction ID bit layout, sequence limit, error codes,
unchanged output on rejected input, and existing little-endian
restriction. This does not change the version range or overflow policy.
- Remove the `fmt` dependency from `versionstamp.h`, preserving its
exact 20-character lowercase hexadecimal output. Add an explicit
`byteswap.h` include to the consumer that previously relied on the
transitive include.
- Add differential tests against both the pre-Versionstamp memcpy
implementation and the original reinterpret-cast implementation. Each
reference covers 8 version boundaries, all 65,536 sequence values, and 8
input alignments (4,194,304 combinations), plus malformed lengths and
empty views. Also check the complete decoded Versionstamp fields and
compare hexadecimal formatting against the previous fmt rule.
The unsafe historical load is retained only as a test oracle on
x86_64/AArch64, with its known alignment sanitizer violation locally
suppressed. Production code and the memcpy reference retain normal
instrumentation; matching the historical output does not make the old
implementation well-defined.
### Release note
None
### Check List (For Author)
- Test
- [ ] Regression test
- [x] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
Validation:
- Cloud ASAN build and compilation/linking of all Cloud unit-test
targets pass on Linux x86_64 with Clang 20.1.8.
- `doris_txn_test:TxnIdConvert.*`: 5 tests pass;
`versionstamp_test:VersionstampTest.*`: 5 tests pass. GTest XML reports
zero failures, errors, or skips.
- Nine isolated related test bodies pass under ASAN+UBSAN on local
ARM64.
- A standalone program including only `versionstamp.h` compiles, links,
and runs without third-party include paths or fmt linkage.
- Changed-line clang-tidy, clang-format 16.0.6, and `git diff --check`
pass.
- External FDB/S3 integration tests were not run.
- Behavior changed:
- [x] No.
- [ ] Yes.
- Does this need documentation?
- [x] No.
- [ ] Yes.
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label
---
cloud/src/meta-service/doris_txn.cpp | 34 ++--
.../src/meta-service/meta_service_tablet_stats.cpp | 1 +
cloud/src/meta-store/versionstamp.h | 14 +-
cloud/test/doris_txn_test.cpp | 176 +++++++++++++++++++++
cloud/test/versionstamp_test.cpp | 23 +++
5 files changed, 219 insertions(+), 29 deletions(-)
diff --git a/cloud/src/meta-service/doris_txn.cpp
b/cloud/src/meta-service/doris_txn.cpp
index eb01dd4d874..5b8b2665a79 100644
--- a/cloud/src/meta-service/doris_txn.cpp
+++ b/cloud/src/meta-service/doris_txn.cpp
@@ -17,34 +17,24 @@
#include "doris_txn.h"
+#include <array>
#include <bit>
+#include <cstring>
+
+#include "meta-store/versionstamp.h"
namespace doris::cloud {
int get_txn_id_from_fdb_ts(std::string_view fdb_vts, int64_t* txn_id) {
if (fdb_vts.size() != 10) return 1; // Malformed version timestamp
- // fdb version timestamp is big-endian
- // MSB LSB
- // 0000000000000000 0000
- // ts seq
- // byte addr 0 1 2 3 4 5 6 7 8 9
- int64_t ver = *reinterpret_cast<const int64_t*>(fdb_vts.data());
-
- // TODO(gavin): implementation for big-endian or make it endian-independent
- static_assert(std::endian::native == std::endian::little); // Since c++20
- // Convert big endian to little endian
- static auto to_little = [](int64_t v) {
- v = ((v & 0xffffffff00000000) >> 32) | ((v & 0x00000000ffffffff) <<
32);
- v = ((v & 0xffff0000ffff0000) >> 16) | ((v & 0x0000ffff0000ffff) <<
16);
- v = ((v & 0xff00ff00ff00ff00) >> 8) | ((v & 0x00ff00ff00ff00ff) << 8);
- return v;
- };
- ver = to_little(ver);
-
- int64_t seq = *reinterpret_cast<const int64_t*>(fdb_vts.data() + 2);
- seq = to_little(seq);
- seq &= 0x000000000000ffff; // Strip off non-seq part
+ static_assert(std::endian::native == std::endian::little);
+ // Copy the possibly unaligned input before decoding its big-endian fields.
+ std::array<uint8_t, 10> bytes;
+ std::memcpy(bytes.data(), fdb_vts.data(), bytes.size());
+ const Versionstamp versionstamp(bytes);
+ uint64_t ver = versionstamp.version();
+ uint16_t seq = versionstamp.order();
// CAUTION: DO NOT EVER TOUCH IT!!! UNLESS YOU ARE PREPARED FOR THE DOOM!!!
// CAUTION: DO NOT EVER TOUCH IT!!! UNLESS YOU ARE PREPARED FOR THE DOOM!!!
@@ -61,7 +51,7 @@ int get_txn_id_from_fdb_ts(std::string_view fdb_vts, int64_t*
txn_id) {
ver <<= SEQ_RETAIN_BITS;
ver |= seq;
- *txn_id = ver;
+ *txn_id = static_cast<int64_t>(ver);
return 0;
}
diff --git a/cloud/src/meta-service/meta_service_tablet_stats.cpp
b/cloud/src/meta-service/meta_service_tablet_stats.cpp
index af8b7051d54..b8260dd2639 100644
--- a/cloud/src/meta-service/meta_service_tablet_stats.cpp
+++ b/cloud/src/meta-service/meta_service_tablet_stats.cpp
@@ -17,6 +17,7 @@
#include "meta-service/meta_service_tablet_stats.h"
+#include <byteswap.h>
#include <fmt/core.h>
#include <fmt/format.h>
#include <gen_cpp/cloud.pb.h>
diff --git a/cloud/src/meta-store/versionstamp.h
b/cloud/src/meta-store/versionstamp.h
index 0fe716e5092..a4276744ed3 100644
--- a/cloud/src/meta-store/versionstamp.h
+++ b/cloud/src/meta-store/versionstamp.h
@@ -17,11 +17,10 @@
#pragma once
-#include <byteswap.h>
-#include <fmt/format.h>
-
+#include <algorithm>
#include <array>
#include <bit>
+#include <compare>
#include <cstddef>
#include <cstdint>
#include <limits>
@@ -143,10 +142,11 @@ public:
}
std::string to_string() const {
- std::string result;
- result.reserve(21); // 10 bytes * 2 hex digits + 1 for null terminator
- for (const auto& byte : data_) {
- result += fmt::format("{:02x}", byte);
+ constexpr char hex_digits[] = "0123456789abcdef";
+ std::string result(data_.size() * 2, '0');
+ for (size_t i = 0; i < data_.size(); ++i) {
+ result[2 * i] = hex_digits[data_[i] >> 4];
+ result[2 * i + 1] = hex_digits[data_[i] & 0x0f];
}
return result;
}
diff --git a/cloud/test/doris_txn_test.cpp b/cloud/test/doris_txn_test.cpp
index af386763e50..ad048b31eaa 100644
--- a/cloud/test/doris_txn_test.cpp
+++ b/cloud/test/doris_txn_test.cpp
@@ -21,10 +21,129 @@
#include <brpc/controller.h>
#include <gtest/gtest.h>
+#include <array>
+#include <bit>
+#include <cstring>
+#include <utility>
+
#include "common/config.h"
#include "common/util.h"
#include "meta-service/meta_service.h"
#include "meta-store/txn_kv.h"
+#include "meta-store/versionstamp.h"
+
+namespace {
+
+#if defined(__x86_64__) || defined(__aarch64__)
+// Preserve the original, unsafe load solely as a historical output oracle on
+// hosts that support unaligned integer loads. Suppress only its known
alignment
+// violation, not instrumentation of the memcpy reference or production
decoder.
+// Matching its results does not establish that the old code was well-defined.
+__attribute__((noinline, no_sanitize("alignment"))) int64_t
legacy_aliased_load(const char* data) {
+ return *reinterpret_cast<const int64_t*>(data);
+}
+#endif
+
+int64_t legacy_memcpy_load(const char* data) {
+ int64_t value;
+ std::memcpy(&value, data, sizeof(value));
+ return value;
+}
+
+// Freeze the pre-Versionstamp algorithm, including the overlapping eight-byte
+// sequence read and its signed intermediate values. Do not use Versionstamp or
+// its byte-swap helpers here: this must remain an independent regression
oracle.
+template <int64_t (*load)(const char*)>
+int legacy_get_txn_id_from_fdb_ts(std::string_view fdb_vts, int64_t* txn_id) {
+ if (fdb_vts.size() != 10) {
+ return 1;
+ }
+ static_assert(std::endian::native == std::endian::little);
+ auto to_little = [](int64_t v) {
+ v = ((v & 0xffffffff00000000) >> 32) | ((v & 0x00000000ffffffff) <<
32);
+ v = ((v & 0xffff0000ffff0000) >> 16) | ((v & 0x0000ffff0000ffff) <<
16);
+ v = ((v & 0xff00ff00ff00ff00) >> 8) | ((v & 0x00ff00ff00ff00ff) << 8);
+ return v;
+ };
+ int64_t ver = to_little(load(fdb_vts.data()));
+ int64_t seq = to_little(load(fdb_vts.data() + 2));
+ seq &= 0xffff;
+ static constexpr int SEQ_RETAIN_BITS = 10;
+ if (seq >= (1L << SEQ_RETAIN_BITS)) {
+ return 2;
+ }
+ seq &= ((1L << SEQ_RETAIN_BITS) - 1L);
+ ver <<= SEQ_RETAIN_BITS;
+ ver |= seq;
+ *txn_id = ver;
+ return 0;
+}
+
+using TxnIdDecoder = int (*)(std::string_view, int64_t*);
+
+void check_decoded_txn_id(TxnIdDecoder legacy_decode, std::string_view input,
int expected_ret) {
+ int64_t expected_txn_id = -1;
+ int64_t actual_txn_id = -1;
+ ASSERT_EQ(legacy_decode(input, &expected_txn_id), expected_ret);
+ ASSERT_EQ(doris::cloud::get_txn_id_from_fdb_ts(input, &actual_txn_id),
expected_ret);
+ ASSERT_EQ(actual_txn_id, expected_txn_id);
+ if (expected_ret != 0) {
+ ASSERT_EQ(actual_txn_id, -1);
+ }
+}
+
+void check_legacy_txn_id_compatibility(TxnIdDecoder legacy_decode) {
+ // Include the positive txn_id limit, the sign transition, and discarded
high
+ // version bits. These latter cases preserve old behavior, not uniqueness.
+ constexpr std::array<uint64_t, 8> versions = {0,
+ 1,
+ 0x00000182a5ed173f,
+ 0x001f82a5ed173f80,
+ 0x001fffffffffffff,
+ 0x0020000000000000,
+ 0x0040000000000000,
+ 0x7fffffffffffffff};
+ alignas(int64_t) std::array<char, 10 + alignof(int64_t) - 1> buffer {};
+ for (uint64_t ver : versions) {
+ SCOPED_TRACE(ver);
+ std::array<uint8_t, 10> bytes {};
+ for (size_t i = 0; i < 8; ++i) {
+ bytes[i] = static_cast<uint8_t>(ver >> (8 * (7 - i)));
+ }
+ for (uint32_t seq = 0; seq <= 0xffff; ++seq) {
+ bytes[8] = static_cast<uint8_t>(seq >> 8);
+ bytes[9] = static_cast<uint8_t>(seq);
+ // Also check the complete fields: txn_id packing discards version
+ // bits and rejects most orders, so output equality alone misses
them.
+ const doris::cloud::Versionstamp versionstamp(bytes);
+ ASSERT_EQ(std::make_pair(versionstamp.version(),
versionstamp.order()),
+ std::make_pair(ver, static_cast<uint16_t>(seq)));
+ for (size_t offset = 0; offset < alignof(int64_t); ++offset) {
+ std::memcpy(buffer.data() + offset, bytes.data(),
bytes.size());
+ const std::string_view input(buffer.data() + offset,
bytes.size());
+ ASSERT_NO_FATAL_FAILURE(
+ check_decoded_txn_id(legacy_decode, input, seq < 1024
? 0 : 2))
+ << "seq=" << seq << " offset=" << offset;
+ }
+ }
+ }
+}
+
+void check_invalid_txn_id_inputs(TxnIdDecoder legacy_decode) {
+ // Check short/long inputs and that failures leave the output untouched.
+ std::array<char, 17> buffer {};
+ for (size_t size = 0; size <= buffer.size(); ++size) {
+ if (size == 10) {
+ continue;
+ }
+ SCOPED_TRACE(size);
+ const std::string_view input(buffer.data(), size);
+ ASSERT_NO_FATAL_FAILURE(check_decoded_txn_id(legacy_decode, input, 1));
+ }
+ check_decoded_txn_id(legacy_decode, {}, 1);
+}
+
+} // namespace
int main(int argc, char** argv) {
doris::cloud::config::init(nullptr, true);
@@ -98,3 +217,60 @@ TEST(TxnIdConvert, TxnIdTest) {
ASSERT_EQ(ret, 1);
}
}
+
+TEST(TxnIdConvert, UnalignedVersionstamp) {
+ // Cover every input alignment, including odd addresses.
+ // The payload has high bits set in both version bytes and sequence bytes.
+ constexpr std::array<unsigned char, 10> versionstamp = {0x00, 0x1f, 0x82,
0xa5, 0xed,
+ 0x17, 0x3f, 0x80,
0x03, 0xff};
+ constexpr int64_t expected_txn_id = 0x7e0a97b45cfe03ff;
+ alignas(int64_t) std::array<char, 10 + alignof(int64_t) - 1> buffer {};
+ for (size_t offset = 0; offset < alignof(int64_t); ++offset) {
+ SCOPED_TRACE(offset);
+ std::memcpy(buffer.data() + offset, versionstamp.data(),
versionstamp.size());
+ int64_t txn_id = -1;
+ ASSERT_EQ(doris::cloud::get_txn_id_from_fdb_ts(
+ std::string_view(buffer.data() + offset,
versionstamp.size()), &txn_id),
+ 0);
+ EXPECT_EQ(txn_id, expected_txn_id);
+ }
+}
+
+TEST(TxnIdConvert, SequenceValues) {
+ // Exercise all two-byte sequences at an odd address. In particular, 0x00ff
+ // must remain positive after decoding, and 0x0400 and above must fail.
+ alignas(int64_t) std::array<unsigned char, 11> buffer = {0, 0x00, 0x00,
0x01, 0x82, 0xa5,
+ 0xed, 0x17, 0x3f,
0, 0};
+ const std::string_view versionstamp(reinterpret_cast<const
char*>(buffer.data() + 1), 10);
+ constexpr int64_t base_txn_id = 0x00060a97b45cfc00;
+ for (uint32_t seq = 0; seq <= 0xffff; ++seq) {
+ SCOPED_TRACE(seq);
+ buffer[9] = static_cast<unsigned char>(seq >> 8);
+ buffer[10] = static_cast<unsigned char>(seq);
+ int64_t txn_id = -1;
+ const int ret = doris::cloud::get_txn_id_from_fdb_ts(versionstamp,
&txn_id);
+ if (seq < 1024) {
+ ASSERT_EQ(ret, 0);
+ EXPECT_EQ(txn_id, base_txn_id + seq);
+ } else {
+ ASSERT_EQ(ret, 2);
+ EXPECT_EQ(txn_id, -1);
+ }
+ }
+}
+
+TEST(TxnIdConvert, LegacyMemcpyCompatibility) {
+ ASSERT_NO_FATAL_FAILURE(
+
check_legacy_txn_id_compatibility(legacy_get_txn_id_from_fdb_ts<legacy_memcpy_load>));
+
check_invalid_txn_id_inputs(legacy_get_txn_id_from_fdb_ts<legacy_memcpy_load>);
+}
+
+TEST(TxnIdConvert, LegacyReinterpretCastCompatibility) {
+#if defined(__x86_64__) || defined(__aarch64__)
+ ASSERT_NO_FATAL_FAILURE(
+
check_legacy_txn_id_compatibility(legacy_get_txn_id_from_fdb_ts<legacy_aliased_load>));
+
check_invalid_txn_id_inputs(legacy_get_txn_id_from_fdb_ts<legacy_aliased_load>);
+#else
+ GTEST_SKIP() << "The original decoder requires a host that supports
unaligned integer loads";
+#endif
+}
diff --git a/cloud/test/versionstamp_test.cpp b/cloud/test/versionstamp_test.cpp
index 5ca138506c1..9dac078f08f 100644
--- a/cloud/test/versionstamp_test.cpp
+++ b/cloud/test/versionstamp_test.cpp
@@ -19,8 +19,10 @@
#include <bthread/bthread.h>
#include <bthread/countdown_event.h>
+#include <fmt/format.h>
#include <gtest/gtest.h>
+#include <array>
#include <cstdint>
#include <cstring>
#include <iostream>
@@ -50,6 +52,27 @@ TEST(VersionstampTest, ByteSwap) {
EXPECT_EQ(swapped16, 0x0201);
}
+TEST(VersionstampTest, ToString) {
+ using doris::cloud::Versionstamp;
+
+ EXPECT_EQ(Versionstamp::min().to_string(), "00000000000000000000");
+ EXPECT_EQ(Versionstamp::max().to_string(), "ffffffffffffffffffff");
+ const Versionstamp versionstamp(0x0001020304050607, 0x08ff);
+ EXPECT_EQ(versionstamp.to_string(), "000102030405060708ff");
+
+ // Verify every byte at every position against an independent formatter.
+ for (size_t position = 0; position < 10; ++position) {
+ for (unsigned int byte = 0; byte <= 0xff; ++byte) {
+ std::array<uint8_t, 10> bytes {};
+ bytes[position] = static_cast<uint8_t>(byte);
+ const auto text = Versionstamp(bytes).to_string();
+ std::string expected(20, '0');
+ expected.replace(position * 2, 2, fmt::format("{:02x}", byte));
+ EXPECT_EQ(text, expected) << "position=" << position << " byte="
<< byte;
+ }
+ }
+}
+
TEST(VersionstampTest, Usage) {
using namespace doris::cloud;
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
