Skip to content

Commit 9f6fcdb

Browse files
committed
fix: repair Singleton double-checked locking race and harden aarch64 portability
An aarch64 portability audit (7-category static sweep + on-hardware validation on a 128-core Kunpeng-920) found one real concurrency bug hidden by x86 TSO and four latent UB/divergence risks: - Singleton<T>::GetInstance() published the instance with a plain store guarded only by a compiler-only MEMORY_BARRIER, and the fast path read it with a plain non-atomic load. On aarch64 this allows readers to observe a non-null pointer to a not-yet-constructed object (TSan confirms the race; a 128-thread litmus of the exact pattern observed torn reads). Use std::atomic<T*> with acquire/release ordering. - IOHook::Impl::mode_ was a plain enum raced by Reset() and Try() (TSan-confirmed). Make it std::atomic<Mode>, stored before the seq_cst pos_/io_count_ stores so it is published together with them. - FieldSumAgg INT8 sum/neg computed on plain char (unsigned on aarch64). Bytes were bit-identical to Java today, but any future widening or comparison would diverge (the bug class PR #181 fixed in min/max). Compute through int8_t like the sibling aggregators. - SerializationUtils::DeserializeBinaryRow read arity from a byte-filled buffer through reinterpret_cast<int32_t*> (strict-aliasing UB). Use memcpy like the serialize side; identical codegen. - CacheManager and SstFileWriter relied on undefined double->int conversions (x86 yields INT_MIN, aarch64 saturates) for extreme configs. Add common-layer SaturatingDoubleToInteger with the Java saturation policy and use it at both sites. Tests: new SingletonTest concurrent GetInstance storms and IOHookTest.TestConcurrentResetAndTry are deterministic TSan regression gates (verified red on the pre-fix code, clean after). FieldSumAgg INT8 boundary tests lock the Java signed-byte semantics. SerializationUtilsTest gains a DataInputStream round-trip that also pins the big-endian wire format. CacheManagerTest locks the saturated capacity semantics (fails pre-fix on x86). Generated-by: Claude Code (claude-opus-4-8)
1 parent f75a464 commit 9f6fcdb

14 files changed

Lines changed: 493 additions & 23 deletions

File tree

include/paimon/factories/singleton.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,9 @@ class PAIMON_EXPORT LazyInstantiation {
3030
protected:
3131
template <typename T>
3232
static void Create(T*& ptr) {
33-
T* tmp = new T;
34-
MEMORY_BARRIER();
35-
ptr = tmp;
33+
// Publication ordering is handled by the release store in
34+
// Singleton<T, InstPolicy>::GetInstance(), so no barrier is needed here.
35+
ptr = new T;
3636
static std::shared_ptr<T> destroyer(ptr);
3737
}
3838
};

src/paimon/CMakeLists.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -634,6 +634,7 @@ if(PAIMON_BUILD_TESTS)
634634
common/utils/range_helper_test.cpp
635635
common/utils/read_ahead_cache_test.cpp
636636
common/io/cache/lru_cache_test.cpp
637+
common/io/cache/cache_manager_test.cpp
637638
common/utils/byte_range_combiner_test.cpp
638639
common/utils/scope_guard_test.cpp
639640
common/utils/sensitive_config_utils_test.cpp
@@ -665,6 +666,7 @@ if(PAIMON_BUILD_TESTS)
665666

666667
add_paimon_test(common_factories_test
667668
SOURCES
669+
common/factories/singleton_test.cpp
668670
common/factories/factory_creator_test.cpp
669671
common/factories/io_hook_test.cpp
670672
STATIC_LINK_LIBS

src/paimon/common/factories/io_hook.cpp

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ class IOHook::Impl {
3232
if (io_count_.fetch_add(1) < pos_.load()) {
3333
return Status::OK();
3434
} else {
35-
switch (mode_) {
35+
switch (mode_.load(std::memory_order_relaxed)) {
3636
case IOHook::Mode::SILENT:
3737
return Status::OK();
3838
case IOHook::Mode::RETURN_ERROR:
@@ -49,9 +49,11 @@ class IOHook::Impl {
4949
}
5050

5151
inline void Reset(int64_t pos, IOHook::Mode mode) {
52+
// Store mode_ first: the seq_cst stores below then publish it, so a Try()
53+
// that observes the reset pos_ also observes the new mode_.
54+
mode_.store(mode, std::memory_order_relaxed);
5255
pos_ = pos;
5356
io_count_ = 0;
54-
mode_ = mode;
5557
}
5658

5759
int64_t IOCount() const {
@@ -65,7 +67,7 @@ class IOHook::Impl {
6567
private:
6668
std::atomic<int64_t> io_count_ = {0};
6769
std::atomic<int64_t> pos_ = {-1};
68-
IOHook::Mode mode_ = IOHook::Mode::SILENT;
70+
std::atomic<IOHook::Mode> mode_{IOHook::Mode::SILENT};
6971
};
7072

7173
IOHook::IOHook() : impl_(std::make_unique<IOHook::Impl>()) {}

src/paimon/common/factories/io_hook_test.cpp

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,13 @@
1919

2020
#include "paimon/common/factories/io_hook.h"
2121

22+
#include <atomic>
2223
#include <stdexcept>
24+
#include <thread>
25+
#include <vector>
2326

2427
#include "gtest/gtest.h"
28+
#include "paimon/status.h"
2529
#include "paimon/testing/utils/testharness.h"
2630

2731
namespace paimon::test {
@@ -64,4 +68,49 @@ TEST(IOHookTest, TestThrowExceptionMode) {
6468
hook->Clear();
6569
}
6670

71+
// Regression test for the data race on IOHook's mode: Reset()/Clear() run on one
72+
// thread while other threads call Try() concurrently. Under a ThreadSanitizer build
73+
// this deterministically reports the unsynchronized mode access; functionally it must
74+
// never crash and every Try() must return either OK or the injected IOError.
75+
TEST(IOHookTest, TestConcurrentResetAndTry) {
76+
auto hook = IOHook::GetInstance();
77+
78+
constexpr int32_t kResetIterations = 200000;
79+
constexpr int32_t kTryIterations = 50000;
80+
constexpr int32_t kNumWorkers = 4;
81+
82+
std::atomic<bool> unexpected_status{false};
83+
84+
std::thread reset_thread([hook]() {
85+
for (int32_t i = 0; i < kResetIterations; i++) {
86+
hook->Reset(i, IOHook::Mode::RETURN_ERROR);
87+
hook->Clear();
88+
}
89+
});
90+
91+
std::vector<std::thread> workers;
92+
workers.reserve(kNumWorkers);
93+
for (int32_t t = 0; t < kNumWorkers; t++) {
94+
workers.emplace_back([hook, &unexpected_status]() {
95+
for (int32_t i = 0; i < kTryIterations; i++) {
96+
Status status = hook->Try("concurrent_path");
97+
// Only RETURN_ERROR mode is armed here, so Try() may only return OK or
98+
// IOError; anything else means the mode was read as garbage.
99+
if (!status.ok() && !status.IsIOError()) {
100+
unexpected_status.store(true, std::memory_order_relaxed);
101+
}
102+
}
103+
});
104+
}
105+
106+
reset_thread.join();
107+
for (auto& worker : workers) {
108+
worker.join();
109+
}
110+
111+
ASSERT_FALSE(unexpected_status.load(std::memory_order_relaxed));
112+
// Leave the process-wide singleton in its default SILENT state for later tests.
113+
hook->Clear();
114+
}
115+
67116
} // namespace paimon::test

src/paimon/common/factories/singleton.cpp

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919

2020
#include "paimon/factories/singleton.h"
2121

22+
#include <atomic>
2223
#include <mutex>
2324

2425
#include "paimon/common/factories/io_hook.h"
@@ -28,15 +29,20 @@ namespace paimon {
2829

2930
template <typename T, typename InstPolicy>
3031
T* Singleton<T, InstPolicy>::GetInstance() {
31-
static T* ptr;
32+
static std::atomic<T*> ptr{nullptr};
3233
static std::mutex mutex;
33-
if (PAIMON_UNLIKELY(!ptr)) {
34+
T* p = ptr.load(std::memory_order_acquire);
35+
if (PAIMON_UNLIKELY(p == nullptr)) {
3436
std::lock_guard<std::mutex> lg(mutex);
35-
if (!ptr) {
36-
InstPolicy::Create(ptr);
37+
// Re-check under the mutex with a relaxed load; the mutex already
38+
// synchronizes with the creating thread.
39+
p = ptr.load(std::memory_order_relaxed);
40+
if (p == nullptr) {
41+
InstPolicy::Create(p);
42+
ptr.store(p, std::memory_order_release);
3743
}
3844
}
39-
return const_cast<T*>(ptr);
45+
return p;
4046
}
4147

4248
template class Singleton<FactoryCreator>;
Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
#include "paimon/factories/singleton.h"
21+
22+
#include <array>
23+
#include <atomic>
24+
#include <cstdint>
25+
#include <thread>
26+
#include <vector>
27+
28+
#include "gtest/gtest.h"
29+
#include "paimon/common/factories/io_hook.h"
30+
#include "paimon/factories/factory.h"
31+
#include "paimon/factories/factory_creator.h"
32+
33+
namespace paimon::test {
34+
35+
namespace {
36+
37+
constexpr int32_t kNumThreads = 32;
38+
39+
class StormFactory : public Factory {
40+
public:
41+
const char* Identifier() const override {
42+
return "storm_factory";
43+
}
44+
};
45+
46+
// Runs `worker(i)` on kNumThreads threads that are all blocked on a shared start
47+
// flag and released at (nearly) the same time, so that they race on the first
48+
// Singleton::GetInstance() publication. Joins all threads before returning.
49+
template <typename Worker>
50+
void RunStorm(const Worker& worker) {
51+
std::atomic<bool> start{false};
52+
std::vector<std::thread> threads;
53+
threads.reserve(kNumThreads);
54+
for (int32_t i = 0; i < kNumThreads; ++i) {
55+
threads.emplace_back([&start, &worker, i]() {
56+
while (!start.load(std::memory_order_acquire)) {
57+
std::this_thread::yield();
58+
}
59+
worker(i);
60+
});
61+
}
62+
start.store(true, std::memory_order_release);
63+
for (auto& thread : threads) {
64+
thread.join();
65+
}
66+
}
67+
68+
} // namespace
69+
70+
TEST(SingletonTest, TestConcurrentIOHookGetInstance) {
71+
std::array<IOHook*, kNumThreads> hooks{};
72+
std::array<bool, kNumThreads> try_oks{};
73+
RunStorm([&hooks, &try_oks](int32_t i) {
74+
hooks[i] = Singleton<IOHook>::GetInstance();
75+
// The default (and cleared) IOHook state is SILENT, so Try() must succeed.
76+
try_oks[i] = hooks[i]->Try("singleton_storm_path").ok();
77+
});
78+
79+
IOHook* expected = hooks[0];
80+
ASSERT_NE(expected, nullptr);
81+
for (int32_t i = 0; i < kNumThreads; ++i) {
82+
ASSERT_EQ(expected, hooks[i]);
83+
ASSERT_TRUE(try_oks[i]);
84+
}
85+
ASSERT_GE(expected->IOCount(), kNumThreads);
86+
// Leave the process-wide singleton in its default SILENT state for later tests.
87+
expected->Clear();
88+
}
89+
90+
TEST(SingletonTest, TestConcurrentFactoryCreatorGetInstance) {
91+
std::array<FactoryCreator*, kNumThreads> creators{};
92+
RunStorm([&creators](int32_t i) { creators[i] = Singleton<FactoryCreator>::GetInstance(); });
93+
94+
FactoryCreator* expected = creators[0];
95+
ASSERT_NE(expected, nullptr);
96+
for (int32_t i = 0; i < kNumThreads; ++i) {
97+
ASSERT_EQ(expected, creators[i]);
98+
}
99+
100+
// Every thread must observe a fully constructed FactoryCreator: a lookup of a
101+
// registered factory must succeed from any thread.
102+
auto* factory = new StormFactory();
103+
expected->Register(factory->Identifier(), factory);
104+
std::array<Factory*, kNumThreads> looked_up{};
105+
RunStorm([&looked_up](int32_t i) {
106+
looked_up[i] = Singleton<FactoryCreator>::GetInstance()->Create("storm_factory");
107+
});
108+
for (int32_t i = 0; i < kNumThreads; ++i) {
109+
ASSERT_EQ(factory, looked_up[i]);
110+
}
111+
}
112+
113+
} // namespace paimon::test

src/paimon/common/io/cache/cache_manager.h

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
#include "paimon/cache/cache.h"
2626
#include "paimon/common/io/cache/cache_key.h"
2727
#include "paimon/common/io/cache/lru_cache.h"
28+
#include "paimon/common/utils/saturating_cast.h"
2829
#include "paimon/memory/memory_segment.h"
2930
#include "paimon/result.h"
3031

@@ -59,9 +60,13 @@ class PAIMON_EXPORT CacheManager {
5960
/// @param high_priority_pool_ratio Ratio of capacity reserved for index cache [0.0, 1.0).
6061
/// If 0, index and data share the same cache.
6162
CacheManager(int64_t max_memory_bytes, double high_priority_pool_ratio) {
62-
auto index_cache_bytes = static_cast<int64_t>(max_memory_bytes * high_priority_pool_ratio);
63+
// Both factors are config-validated non-negative values, so the products are finite;
64+
// saturation is only a defense against the undefined double->int64_t conversion when
65+
// max_memory_bytes is close enough to INT64_MAX that the product rounds to 2^63.
66+
auto index_cache_bytes =
67+
SaturatingDoubleToInteger<int64_t>(max_memory_bytes * high_priority_pool_ratio);
6368
auto data_cache_bytes =
64-
static_cast<int64_t>(max_memory_bytes * (1.0 - high_priority_pool_ratio));
69+
SaturatingDoubleToInteger<int64_t>(max_memory_bytes * (1.0 - high_priority_pool_ratio));
6570
data_cache_ = std::make_shared<LruCache>(data_cache_bytes);
6671
if (high_priority_pool_ratio == 0.0) {
6772
index_cache_ = data_cache_;

0 commit comments

Comments
 (0)