Skip to content

[BUG] Data race on non-atomic unsorted_since_ field in MutableCsr #722

Description

@zhanglei1949

Describe the bug

MutableCsr::unsorted_since_ is a plain timestamp_t member (not std::atomic) but is read and written from multiple threads without synchronization, constituting a data race (undefined behavior per C++ standard).

Affected locations

The field is declared at include/neug/storages/csr/mutable_csr.h:257:

timestamp_t unsorted_since_;  // ← not atomic

It is accessed concurrently in the following methods:

Writes (from put_edge): include/neug/storages/csr/mutable_csr.h:157-159

// Called from multiple threads with different src locks (locks_[src])
// but all writing to the same unsorted_since_
if (ts < unsorted_since_) {
    unsorted_since_ = 0;
}

Writes (from batch_put_edges): src/storages/csr/mutable_csr.cc:540-543

if (ts < unsorted_since_) {
    unsorted_since_ = 0;
}

Writes (from batch_sort_by_edge_data): src/storages/csr/mutable_csr.cc:277

unsorted_since_ = ts;

Reads (from get_generic_view): include/neug/storages/csr/mutable_csr.h:71

return CsrView(..., ts, unsorted_since_, prefetch_policy_);

This value is then used by CsrView to determine sorted vs. unsorted regions for binary search optimization in foreach_nbr_gt / foreach_nbr_lt (see csr_view.h:430-450).

The race

  • Thread A calls put_edge(src1, ..., ts=5) holding locks_[src1], reads unsorted_since_ and writes it to 0.
  • Thread B calls put_edge(src2, ..., ts=10) holding locks_[src2], simultaneously reads/writes unsorted_since_.
  • Thread C calls get_generic_view(ts) and reads unsorted_since_ concurrently.

Since locks_[src1] and locks_[src2] are different locks, there is no mutual exclusion for unsorted_since_.

Expected behavior

unsorted_since_ should use atomic operations to ensure tear-free reads and writes, with appropriate memory ordering (relaxed is sufficient since it's a hint for optimization, not a correctness-critical value).

Suggested fix

// In the class declaration:
std::atomic<timestamp_t> unsorted_since_{0};

// In get_generic_view:
return CsrView(..., ts, unsorted_since_.load(std::memory_order_acquire), ...);

// In put_edge:
if (ts < unsorted_since_.load(std::memory_order_relaxed)) {
    unsorted_since_.store(0, std::memory_order_relaxed);
}

// In batch_sort_by_edge_data:
unsorted_since_.store(ts, std::memory_order_relaxed);

Impact

  • Severity: Medium — undefined behavior per C++ standard. In practice, on x86 this likely manifests as torn reads (unlikely for uint32_t) or stale values, causing CsrView to incorrectly apply binary search on unsorted regions, potentially missing edges in range queries.
  • Likelihood: Medium — occurs whenever concurrent writes and reads happen on the same CSR (common in Service Mode with concurrent transactions).
  • Note: Since timestamp_t = uint32_t, torn reads are unlikely on x86 (atomic for aligned 32-bit), but the compiler may still reorder accesses. The fix is still required for standard compliance and portability to ARM.

Metadata

Metadata

Assignees

Labels

bugSomething isn't workingstoreStorage layertransactionTransaction management

Type

Projects

Status
To do

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions