CPU corruption injector: gdb register flip into one db_stress op (#14858)

[hx235]

Summary:

This PR is the injection layer of the CPU corruption injector, runs inside gdb and randomly corrupts a register by bit flip in exactly one db_stress op (i.e, write, foreground compaction and flush) per stress test run. Detection layer is at db_stress (#14852); orchestration layer is coming up.

How one run works

• The orchestration layer, coming up, randomly picks which stress test op instance (so corruption can land at different points in the LSM shape journey) and which target_fn of that op (so to cap instructions to step under a reasonable limit; injector.py in this PR randomly picks which instruction within the target_fn to inject (so corruption can land at different points of a target_fn).
• Attach: gdb starts with injector.py's parameters passed via -iex and the db_stress command after --args, so db_stress runs unmodified. Example:

gdb --batch --nx \
  -iex "py import sys; sys.argv=['injector.py','--op','write','--op_index','42','--entry_fn','rocksdb::MemTable::Add','--target_fn','rocksdb::MemTable::Add','--corruptions_per_op','1','--seed','7','--dir','<rundir>']" \
  -x tools/cpu_corruption_injector/injector.py \
  --args <db_stress> --threads=1 --verify_cpu_corruption_dir=<rundir>  ...

• Navigate: The orchestration layer will pick op_index. entry_fn is called exactly once per stress test run's op so the op_index-th op is its op_index-th call. injector_navigate.py breaks on entry_fn and set a gdb ignore-count of op_index-1 to fast-forward to op_index-th one. It also breaks at the first target_fn within that entry_fn.

• Warm up: injector_critical_instruction.py will choose "critical instruction" (those that move key/value bytes with general-purpose or vector registers or set a branch flag) uniformly within the chosen target_fn by the orchestration layer. In order to do that, it needs to approximate how many such instructions within the target_fn. Hence we have this warm-up phase. It single-steps the instruction within the first encoutering of target_fn to count and draw the critical instruction index, then corrupt that index at a later call.

• Corrupt: on a later call of target_fn, injector_critical_instruction.py single-step to the m-th critical instruction and bit-flip the register through injector_register_corruption.py. The way to corrupt register depends on what instruction it is. If the current call of target_fn's m-th instruction is not a critical instruction, we will try next target_fn till running out of target_fn.

• Record: injector_telemetry.py provides telemetry to capture the corruption for later analysis.

Test plan:

1. Isolated tests (real gdb-captured x/i fixtures): test_inject_critical_instruction

2. E2E test on navigation, inject, telemetry will be done in the later orchestration PR. Below is inject.json from such run

{
  "injection_result": "injected",
  "db_stress_crash_signal": null,
  "op": "write",
  "op_index": 279,
  "entry_fn": "rocksdb::MemTable::Add",
  "target_fn": "rocksdb::MemTable::Add",
  "critical_instruction_index": 37,
  "corruptions": [
    {
      "instruction": "mov    %rsi,0x8c8(%rbx)",
      "register": "rsi",
      "corruption_type": "bit_flip",
      "before": "0x7fffee4c64d8",
      "after": "0x7fffee4c64c8",
      "details": {
        "source": "rocksdb::Arena::AllocateAligned @ ./fbcode/internal_repo_rocksdb/repo/memory/arena.cc:135",
        "call_chain": [
          "rocksdb::Arena::AllocateAligned @ ./fbcode/internal_repo_rocksdb/repo/memory/arena.cc:135",
          "rocksdb::ConcurrentArena::AllocateAligned(unsigned long, unsigned long, rocksdb::Logger*)::{lambda()#1}::operator()() const @ fbcode/internal_repo_rocksdb/repo/memory/concurrent_arena.h:65",
          "rocksdb::ConcurrentArena::AllocateImpl<rocksdb::ConcurrentArena::AllocateAligned(unsigned long, unsigned long, rocksdb::Logger*)::{lambda()#1}>(unsigned long, bool, rocksdb::ConcurrentArena::AllocateAligned(unsigned long, unsigned long, rocksdb::Logger*)::{lambda()#1} const&) @ fbcode/internal_repo_rocksdb/repo/memory/concurrent_arena.h:145",
          "rocksdb::ConcurrentArena::AllocateAligned @ fbcode/internal_repo_rocksdb/repo/memory/concurrent_arena.h:63",
          "rocksdb::InlineSkipList<rocksdb::MemTableRep::KeyComparator const&>::AllocateNode @ fbcode/internal_repo_rocksdb/repo/memtable/inlineskiplist.h:868",
          "rocksdb::InlineSkipList<rocksdb::MemTableRep::KeyComparator const&>::AllocateKey @ fbcode/internal_repo_rocksdb/repo/memtable/inlineskiplist.h:855",
          "rocksdb::(anonymous namespace)::SkipListRep::Allocate @ ./fbcode/internal_repo_rocksdb/repo/memtable/skiplistrep.cc:36",
          "rocksdb::MemTable::Add @ ./fbcode/internal_repo_rocksdb/repo/db/memtable.cc:1157"
        ]
      }
    }
  ],
  "ops_seen": 279,
  "critical_instructions_seen": 38
}

Reviewed By: pdillinger

Differential Revision: D107999835

[meta-codesync]

@hx235 has exported this pull request. If you are a Meta employee, you can view the originating Diff in D107999835.

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 0.0s.

[github-actions]

Codex Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

🟡 Codex Code Review

Auto-triggered after CI reached the early-review threshold — reviewing commit babebbd

---

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not create PATH aliases: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.

---

ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

• Codex may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /codex-review [context] — Request a code review
• /codex-query <question> — Ask about the PR or codebase

[github-actions]

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit babebbd

---

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not create PATH aliases: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.

---

ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

• Codex may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /codex-review [context] — Request a code review
• /codex-query <question> — Ask about the PR or codebase

[github-actions]

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI reached the early-review threshold — reviewing commit babebbd

---

Summary

This PR adds a defensive CPU corruption injector (Python gdb scripts) and a db_stress verification layer (--verify_cpu_corruption_dir) that detects silent data corruption by reading back the entire keyspace after each write/flush/compaction op. The C++ changes also refactor TestCompactFiles and TestCompactRange from void to Status. The design is well-motivated, the separation between injection (Python/gdb) and detection (C++/db_stress) is clean, and the startup validation is thorough.

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

No high-severity findings.

🟡 MEDIUM

M1. ClassifyReadBack does not verify wide-column entity data — db_stress_test_base.cc:~955

• Issue: The new ClassifyReadBack uses db_->Get() and compares the returned value byte-for-byte against GenerateValue(expected.GetValueBase(), ...). When use_put_entity_one_in > 0, some keys are stored via PutEntity with wide columns. While db_->Get() returns the default column value (which matches GenerateValue), the non-default wide columns are not verified. A CPU corruption that corrupts only a non-default wide column would go undetected.
• Root cause: The tool uses Get() for simplicity. The default column carries the canonical value, so the primary corruption vector is covered.
• Suggested fix: Acceptable for the stated scope. Consider adding a comment noting the limitation, or using GetEntity for entity keys.

M2. Python: _in_allowed_module uses broad substring matching — injector_navigate.py:~260

• Issue: _in_allowed_module checks if tokens like "memcpy", "zstd", "hash_bytes" appear as substrings of the lowered function name. This could match unintended functions (e.g., my_memcpy_helper in a non-rocksdb library), causing the injector to single-step through irrelevant code (wasting time, not causing incorrect behavior).
• Root cause: Substring matching chosen for simplicity over exact symbol matching.
• Suggested fix: Acceptable trade-off. The false-positive case (stepping extra functions) is benign.

🟢 LOW / NIT

L1. Duplicated verification logic with TODO — db_stress_test_base.cc:~1038

• Issue: The full-keyspace read-back duplicates the canonical VerifyDb/VerifyOrSyncValue verifier (no_batched_ops_stress.cc:3293). The new ClassifyReadBack is simpler but could diverge over time.
• Suggested fix: Track the acknowledged TODO. Simplicity justified by constrained environment (threads=1, no fault injection).

L2. WriteDataCorruption uses raw C file I/O — db_stress_test_base.cc:~988

• Issue: Uses fopen/fputs/fclose instead of RAII wrappers.
• Suggested fix: Minor style issue in error-reporting path that runs at most once.

L3. DataCorruption struct uses Slice with borrowed lifetime — db_stress_test_base.cc:~934

• Issue: Safe in current code (consumed immediately), but fragile if captured across scopes.
• Suggested fix: A comment noting the borrowed lifetime would help.

L4. Python: parse_known_args silently ignores unknown arguments — injector.py:~100

• Issue: Typos in argument names silently discarded.
• Suggested fix: Consider parse_args or logging unknown args.

L5. Python: op_index silently clamped to 1 — injector.py:~106

• Issue: max(parsed.op_index, 1) silently clamps non-positive values.
• Suggested fix: Consider raising an error for non-positive values.

L6. No unit tests for C++ verification code

• Issue: C++ verification functions lack unit tests. E2E tests planned for orchestration PR.
• Suggested fix: Acceptable deferral per PR description.

Cross-Component Analysis

Context	Executes?	Assumptions hold?	Action
WritePreparedTxnDB	Possible	YES - Get reads latest committed	Safe
ReadOnly DB	NO	N/A	Safe
User-defined timestamps	YES	YES - supplies GetNowNanos() read timestamp	Safe
BlobDB	YES	YES - Get dereferences blobs	Safe
FIFO/Universal compaction	YES	YES	Safe
Concurrent memtable writes	NO - threads=1 enforced	N/A	Safe
test_batches_snapshots	NO - startup rejects	N/A	Safe
test_multi_ops_txns	NO - startup rejects	N/A	Safe

Assumption stress-test: All three core claims verified with no counterexample found. ExpectedValue::Exists() is safe under threads=1. All fault injection flags are checked. The void->Status refactoring is complete across all override sites.

Positive Observations

• Thorough startup validation rejecting all fault injection and multi-threaded configurations
• Clean Python/C++ separation with JSON contract
• Correct void->Status refactoring preserving existing control flow
• Well-documented Python modules with clear gdb lifecycle management
• Good defensive coding patterns throughout

The full review is also saved to review-findings.md.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit babebbd

---

Summary

This PR adds a well-structured CPU corruption injection framework for db_stress: a Python gdb-based injector that flips register bits in one op, and a C++ detection layer that does full-keyspace read-back verification. The design is sound, the startup validation is thorough, and the code is well-documented. The virtual function signature change from void to Status for TestCompactRange is a clean improvement.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. Missing --use_timed_put_one_in guard may cause false positive "lost" reports — db_stress_tool.cc

• Issue: The startup validation does not reject --use_timed_put_one_in > 0 combined with --verify_cpu_corruption_dir. Timed puts can expire via TTL-triggered compaction, causing keys to disappear from the DB even though the expected state model still thinks they exist. The full-keyspace read-back in MaybeVerifyCpuCorruption would classify such an expired key as a "lost" data corruption — a false positive.
• Root cause: The startup guard checks fault injection flags and incompatible test modes but does not account for TTL-based key expiration.
• Suggested fix: Add FLAGS_use_timed_put_one_in != 0 (and possibly FLAGS_compaction_ttl > 0) to the startup rejection block, or document that timed puts are safe because the single-op injection is too fast for TTL expiration.

M2. No validation against --compaction_ttl — db_stress_tool.cc

• Issue: Similar to M1, --compaction_ttl > 0 can cause keys to be dropped during compaction. If a compaction op is the injected op and the full-keyspace read-back runs after it, keys dropped by TTL compaction would look like "lost" corruption.
• Suggested fix: Same as M1 — either reject this combination at startup or document why it's safe.

M3. ClassifyReadBack does not handle Status::IOError gracefully — db_stress_test_base.cc:~945-950

• Issue: If Get() returns an unexpected status (e.g., IOError due to a CPU-corruption-induced bad file offset), the code calls port::ImmediateExit(1) with only a stderr message. A CPU-corrupted register could produce an IOError from the read-back itself. This exit path does not write a data_corruption.*.json file, so the orchestration layer would have to infer what happened from the exit code alone.
• Suggested fix: Consider classifying unexpected Get() errors as "detected-corruption" and writing the JSON file before exiting.

M4. WriteDataCorruption uses fopen/fputs/fclose without RAII — db_stress_test_base.cc:~985-995

• Issue: If there's an interruption between fopen and fclose, the file handle leaks. Minor risk since this is a diagnostic tool.
• Suggested fix: Consider std::ofstream or a cleanup guard for RAII consistency.

🟢 LOW / NIT

L1. DataCorruption Slice members are non-owning — db_stress_test_base.cc:~932-936

• Issue: The Slice members point into db_value and expected_scratch which are alive in the calling scope. Safe but fragile if the struct is ever stored beyond the current scope.
• Suggested fix: Consider a lifetime comment or owning std::string members.

L2. Missing test for testb/testw/testl/testq and cmpxchg rejection — test_injector_critical_instruction.py

• Issue: The _CMP_TEST_RE regex handles test[bwlq]?\b and uses \b to reject cmpxchg, but neither is tested.
• Suggested fix: Add test cases for testb (positive) and cmpxchg (negative rejection).

L3. _MAX_STEPS_PER_CALL = 20000 is not configurable — injector_navigate.py

• Issue: Hardcoded cap. Well-commented but not exposed as a CLI parameter.
• Suggested fix: Consider making it configurable if needed for large target functions.

Cross-Component Analysis

Context	Executes?	Assumptions hold?	Action?
WritePreparedTxnDB	YES	YES (threads=1)	Safe
User-defined timestamps	YES	YES (read ts set)	Safe
Merge ops	YES	YES (Get returns merged)	Safe
PutEntity	YES	YES (Get returns default col)	Safe
BlobDB	YES	YES (transparent deref)	Safe
FIFO/Universal compaction	YES	YES	Safe
Concurrent writers	NO (threads=1)	N/A	N/A
Timed put / TTL	POSSIBLE	May NOT hold	See M1/M2

Positive Observations

1. Thorough startup validation correctly rejects incompatible configurations
2. Well-documented Python modules with clear architecture comments
3. Clean virtual function signature change for TestCompactRange
4. Correct Slice lifetime management in DataCorruption usage
5. gdb signal handling (stop nopass print) prevents nested gdb conflicts
6. Well-scoped instruction decoder focuses on data-carrying operations only
7. Defensive Python error handling prevents gdb from leaving db_stress wedged

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase

[meta-codesync]

This pull request has been merged in 3745f2e.