Use-after-free: PostReadCc dereferences a recycled NonBlockingLock when a read-intent is released before its post-read (under CleanDataForTest kickout)

[liunyl]

Summary

A pre-existing use-after-free in the cc-entry lock lifetime. Under a multi-node cluster with the data store + aggressive eviction (kickout_data_for_test=true, checkpointer_interval=1) plus concurrent scans/writes, a PostReadCc can dereference a NonBlockingLock that has already been recycled, aborting the node.

Crash signature

eloqkv: .../abseil-cpp/absl/container/internal/raw_hash_set.h:325:
probe_seq<16>::probe_seq(size_t, size_t): Assertion `((mask + 1) & mask) == 0 && "not a mask"' failed.   (SIGABRT)

Crashing thread (core + gdb):

#3  absl::container_internal::probe_seq<16>::probe_seq        (corrupt capacity)
#6  raw_hash_set<FlatHashMapPolicy<uint64,uint32>>::find_non_soo
#8  txservice::NonBlockingLock::ReleaseReadIntent             non_blocking_lock.cpp:503
#9  txservice::NonBlockingLock::ClearTx                       non_blocking_lock.cpp:562
#10 txservice::CcMap::ReleaseCceLock                          cc_map.cpp:412  (lk_type=NoLock, recycle_lock=true)
#11 TemplateCcMap<EloqKey,RedisEloqObject>::Execute           template_cc_map.h:1314
#12 PostReadCc::Execute                                       cc_request.h:262
#13 CcShard::ProcessRequests -> TxProcessor::RunOneRound      (is_ext_proc=true)

In the core, the NonBlockingLock's read_intentions_ (absl::flat_hash_map<TxNumber,uint32_t>) has garbage capacity_ / size_ / slot pointer — the lock object was recycled/clobbered while still referenced.

Root cause

PostReadCc::Execute (template_cc_map.h:1243) obtains the entry from the cached cce_addr (cce_addr.ExtractCce(), a KeyGapLockAndExtraData* captured at read time), then calls cc_entry.GetKeyLock()->ReleaseReadIntent(). If the cce's read-intent is released before that PostReadCc runs, the entry becomes IsFree() (cc_lock_and_extra_ == nullptr && IsPersistent()), the CleanDataForTest kickout (LocalCcShards::KickoutDataForTest → KickoutCcEntryCc with CleanType::CleanDataForTest) recycles it, and the later PostReadCc dereferences the recycled lock → UAF.

What is already correct (so the gap is the early intent release)

• Read-set insertion already guards lock_type != LockType::NoLock at every site (point read tx_execution.cpp:2206; scan sites tx_execution.cpp:3007 / 3148 / 3369 / 5886). ReadCommitted reads map to NoLock (LockTypeUtil::DeduceLockType) and are not tracked; scans AcquireReadIntent per tuple.
• The kickout gate already skips locked entries: CleanDataForTest → CcPageCleanGuardWithoutKickoutCc::CanBeCleaned = IsFree() && !GetBeingCkpt().

So the entry was IsFree() at kickout time, i.e. its read-intent had been released before the post-read. Suspected early-release sites (a read-intent refcount interaction): scan DrainScanner (PostReadType::Release, tx_execution.cpp ~3651), the transient last-cce pin DecrReadIntent (template_cc_map.h:3884 / ~3891), or a DedupRead edge (tx_execution.cpp:6370).

Dead code noticed nearby

The "record-and-don't-free a still-locked cce" defense is not wired up: CcPageCleanGuard::MarkClean(idx, delay_free) ignores its delay_free argument, and LruEntry::ClearLocks(ccs, ng, invalidate_owner_term) never references invalidate_owner_term (no caller passes true) — vestigial since #168 ("Restructure for eloqconverge"). For the genuine stale-ex-leader post-read, the only active guard is the term check (PostReadCc::CheckLeaderTerm + ReadWriteSet::ClearDataReadSet read-lock-term validation), which fires only on a term change (failover) and does not cover the same-term kickout case above.

Relation to PR #491 (bthread-mutex CC-request fix)

Not a regression from #491. Verified by A/B (built fix @1448c93 vs pre-fix @f62f0fa): both pass scan.tcl alone 15× and both crash identically under concurrent-writer stress with the same NonBlockingLock assertion. The CI failure on the eloqkv PR (eloqkv-pr-ent/compile-test build #285, config 7 = 3-node cluster + eloqstore + kickout_data_for_test, scan.tcl first flushdb → I/O error reading reply) is an intermittent manifestation of this pre-existing race, not a new bug introduced by #491.

Reproduction (flaky / timing-sensitive)

• Debug build, WITH_DATA_STORE=ELOQDSS_ELOQSTORE, EXT_TX_PROC_ENABLED=ON, WITH_LOG_SERVICE=ON, OPEN_LOG_SERVICE=OFF, FORK_HM_PROCESS=ON.
• 3-node local cluster, shared eloqstore via minio, --core_number=2 --checkpointer_interval=1 --kickout_data_for_test=true --max_processing_time_microseconds=1000.
• Concurrent writers (SET churn) + scan.tcl loop → a data node SIGABRTs intermittently (usually within 1–2 scan iterations under light load; heavier load tends to hang instead).
• A deterministic repro / regression test is best done with CODE_FAULT_INJECTOR (force a kickout/recycle of a read-set cce right before its PostReadCc).

Proposed fix direction

Ensure a cce referenced by an in-flight PostReadCc (i.e. in a tx's data/scan read-set) retains its read-intent until the post-read releases it, so the kickout's IsFree() gate skips it. Audit the read-intent refcount accounting across the scan drain / transient last-cce pin / dedup paths. Optionally remove (or properly wire up) the dead delay_free / invalidate_owner_term machinery.

[liunyl]

I checked this against the current main tip (a674cac223b7ee58675f5f73ba5630197d79dc49). The lifetime hazard described here is still present; the latest changes do not touch this path.

Additional concrete race window

There is a sufficient same-term race involving scan early-release:

1. A scan reads CCE A and the transaction holds a read lock/read-intent on A.
2. ScanClose() removes an early-released tuple from the read set and sends a PostReadCc(A, Release).
3. ReleaseScanExtraLockOp::Forward() explicitly does not wait for remote work: it completes when LocalRefCnt() == 0.
4. The call also sends remote PostRead with need_remote_resp=false.
5. Therefore the same transaction may continue and read A again while the old release is still queued/in flight, adding A back to its read set.
6. The delayed old PostReadCc(A, Release) then executes ClearTx(txn). That is transaction-scoped, not acquisition-scoped: ReleaseReadIntent(txn) defaults to release_all=true, so it can clear the later acquisition too.
7. Once the last lock/intent disappears, RecycleKeyLock() detaches the pooled KeyGapLockAndExtraData and A becomes IsFree().
8. Kickout deletes A.
9. Commit validation later sends the PostRead for the newer read-set entry. Its CcEntryAddr still contains the pooled lock address; ExtractCce() can now return a freed entry or an unrelated entry after slot reuse (UAF/ABA).

This is a concrete path capable of producing the reported failure. It still needs a deterministic test to prove that this was the exact path taken by the captured crash, rather than another duplicate/early-release path.

Normal Redis SCAN currently leaves most of unlock_batch unused (its population is commented out), but the shared transaction engine also serves Mongo/SQL, and scan-close/drain/trailing-tuple paths remain relevant there.

Retransmission makes decrement-only insufficient

Changing early Release to DecrReadIntent alone is not robust. CcStreamSender has reconnect/EAGAIN resend queues, so the design should assume at-least-once delivery. Replaying the same logical decrement can underflow the ownership accounting or release a newer acquisition.

Waiting for one ACK alone is also incomplete unless duplicate logical releases are idempotent: an ACK from the first copy does not prove that another copy is not still queued.

Solution options discussed

1. Wait for release completion, with idempotent logical requests (recommended)

• Give every logical PostRead release a unique request ID.
• Deduplicate at the receiver: the same request ID executes at most once; duplicates return the prior result without dereferencing CcEntryAddr again.
• ReleaseScanExtraLockOp must wait for all logical release ACKs before the transaction can execute a later operation.
• On timeout, abort the transaction; do not allow it to continue and reacquire entries while release status is unknown.

The barrier prevents an old release from crossing a later acquisition. Request-ID idempotency prevents stream resend from executing the release twice.

Latency is approximately one slowest-node RTT plus CC-queue time, not one serial RTT per tuple, because releases are already issued in parallel. If response traffic becomes material, releases targeting the same shard can later be batched behind one logical request/ACK.

2. Retain every read lock/read-intent until commit

This avoids scan-close release ordering, but is less attractive for the shared Mongo/SQL scan path:

• long scans retain potentially large numbers of non-evictable CCEs;
• read locks block writers until commit;
• validation and release are currently combined in PostReadCc, so this requires splitting “validate before WAL” from “release after commit/abort” and preserving the read-set addresses through every success/error path.

It is simpler semantically but likely worse for long, common scans and is a broader transaction-pipeline change.

Deterministic regression tests

The minimum stable coverage should include:

1. A remote release ref is outstanding: ReleaseScanExtraLockOp must not let the tx advance.
2. After the ACK, the operation advances.
3. Replay the same release request ID multiple times: lock/read-intent state changes once.
4. Replay an already-completed request after kickout/reuse: the receiver returns the cached result and never dereferences the stale CCE address.
5. Optional fault-injection integration test: delay scan-close release, attempt a later read in the same tx, and verify that the later operation cannot start before release completion.

The key invariant is: a logical early release may execute once, and no later acquisition by the same transaction may start until its result is known.

[liunyl]

Final design decision: retain scan reads until transaction finalization

After reviewing the shared Mongo/SQL/EloqKV scan path, we selected option 2 rather than adding a synchronous scan-close ACK barrier.

Implementation: #526
EloqKV submodule update: eloqdata/eloqkv#536

Correctness invariant

No data ReadLock or ReadIntent acquired by a transaction may be released while that transaction can still execute another request. Every CCE address that still represents transaction-owned read state remains in the transaction read set until the existing commit/abort finalization path consumes it.

This removes the dangerous ordering entirely:

old scan-close PostRead release
    -> later same-tx acquisition
    -> delayed old ClearTx(txn)

There is no scan-close data PostRead after this change, so an old fire-and-forget release cannot cross a later acquisition.

Read-set representation

• Normal returned scan tuples that acquired read ownership remain semantic read-set entries with their real version.
• Scanner-only CCEs acquired for traversal, including drained tuples, trailing prefetch tuples, and applicable range last-tuple pins, are inserted into the same data read set with version 0.
• Version 0 means release-only: final PostRead releases ownership without adding OCC version validation.
• If the CCE is already in the read set, no second entry is needed.
• CCEs owned by the object-command set are excluded to avoid double cleanup.
• Trailing tuples are retained before scanner caches are reset or recycled.

Final release paths

• Commit uses the existing ValidateOperation fan-out. It sends PostRead for all data read-set entries and waits for local and remote completion.
• Abort uses the existing PostProcessOp cleanup fan-out and also waits for completion.
• Unique-secondary lookup early-release paths are removed for the same reason.
• No DecrReadIntent accounting is introduced, so remote resend/retry cannot make a decrement count diverge.

The fix deliberately reuses the transaction-final PostRead protocol instead of creating a new scan-close request-ID/dedup protocol.

Table-name lifetime

ReadWriteSet already stores non-owning table-name string_view values. Normal reads and scans rely on names backed by long-lived caller/schema/catalog storage.

The first implementation temporarily added a release_read_table_names_ owner set because scan-close processing was using ScanCloseTxRequest::table_name_, whose owning string belongs to a pooled fire-and-forget request and may be reused before transaction finalization.

The final implementation removes that owner set:

1. Scan open captures a non-owning TableName view in ScanState from the same stable backing storage used by regular read-set entries.
2. Scan-close/drain release-only entries use ScanState::table_name_, never the pooled ScanCloseTxRequest copy.
3. Existing ReadWriteSet::AddRead(cce, 0, table_name) is used directly.

This keeps a single read-set ownership model and does not duplicate a TableName per CCE or add a separate per-transaction name intern table.

Performance tradeoff

This avoids adding a remote RTT and CC-queue barrier to every common Mongo/SQL scan close. The cost is that locking scans retain read ownership and non-evictable CCEs until commit/abort; long or LIMIT-heavy scans may therefore block writers longer and grow the read set. Scanner-only version-0 entries do not increase OCC validation conflicts, while returned semantic tuples keep their existing validation behavior.

Regression coverage

The focused test verifies:

• scan-close ownership is still present after a later same-transaction barrier request;
• commit releases it;
• abort releases it through the same final cleanup model;
• the stable ScanState table-name view, rather than a pooled scan-close copy, backs release-only read-set entries.

The focused transaction consistency test passes 20 consecutive runs locally, and the parent EloqKV target builds against the final tx_service commit. Both PR CI suites are fully green on amd64 and arm64, including the EloqKV build matrix and compile-all-variants jobs.