Audit fixes: F-10, F-13, F-14, F-30 (OMP races, tx_init defense, locale-independent reference validation)#155
Merged
adequatelimited merged 8 commits intomasterfrom Apr 13, 2026
Merged
Conversation
In validate_tfile_pow_fp(), ecode is shared across OMP threads: writes happen inside OMP_CRITICAL_ blocks (correctly serialized) but reads are outside any critical section (the while-loop condition at line 853 and the early-continue at line 866). Under the C11 memory model this is a data race and undefined behavior. On x86 it works by accident due to Total Store Order — stores become visible quickly and the compiler is unlikely to cache the value in a register across an OMP_CRITICAL_ boundary. On weakly- ordered architectures (ARM64, RISC-V, MIPS) the write may not be visible to a reader for an unbounded time. ThreadSanitizer also flags this as a data race. Declaring ecode volatile prevents the compiler from caching it in a register and forces each read to go to memory. Combined with the memory barriers implicit in OMP critical-section entry/exit, this gives correct shared-flag semantics on all real hardware. errnum is written inside critical sections and read only AFTER the OMP_PARALLEL_ block ends. The implicit barrier at parallel- region exit provides the necessary synchronization, so errnum does not need volatile. Closes #101
fix(tfile): declare ecode volatile in validate_tfile_pow_fp()
scan_quorum() declared a single shared `int result` used for two
semantically different purposes in the same OMP parallel loop:
1. Weight comparison inside OMP_CRITICAL_() (writes/reads under lock)
2. Peer-contribution counter OUTSIDE any critical section (writes
and reads across threads with no synchronization)
The conflation produced a real data race between threads: Thread A in
the critical section writing result = cmp256(...) while Thread B
outside any critical section executing result = 0 / result++. TSan
flagged the access pattern empirically on a live mainnet scan.
Split the variable into two thread-private variables declared at
function top and added to the OMP private() clause:
- weight_cmp: used only in the critical section for cmp256 result
- contrib: used only in the peer-counting loop outside the critical
section
Style: matches the existing codebase convention for OpenMP thread-local
state (see `node`, `peer`, `len`, `ipstr` in the same function's
private() clause). Block-scope declarations inside the OMP region work
equally well semantically but the function-top + private() form is
what the rest of this file uses.
Also applied the same style to `peer` and `ecode` in catchup() (sync.c)
while in the neighborhood — both were declared inside the OMP parallel
block body in C99 style. Moved to function top + private() clause for
consistency. No behavior change in catchup(); purely convention cleanup.
Closes #110
fix(network,sync): eliminate OMP data races on shared working variables
tx__init() previously left dsaoff and tlroff uninitialized when the TXDAT_TYPE or TXDSA_TYPE switch case did not match a known value, then proceeded to use those uninitialized values in pointer arithmetic and size-check computations. A peer-controlled OP_TX packet with a type byte other than 0x00 produced undefined behavior at every step from the offset computation through the TXENTRY pointer initialization. Build with -ftrivial-auto-var-init=pattern and a temporary trace showed the offsets reading as 0xfefefefefefefefe (the GCC pattern fill for uninitialized stack locals) on every bad-type case, and the resulting tx->dsa / tx->tlr / tx_sz values were garbage. tx_read's post-tx__init size check happened to reject the test cases with a small buffer, but a larger crafted buffer could pass that check and propagate garbage pointers into tx_val() or process_tx(). Fix: - tx__init() return type changed from void to int (VEOK/VERROR) - Added default: return VERROR to both type switches so uninitialized offsets are never read - Added bounds checks: dsaoff < sizeof(tx->buffer) and tlroff + sizeof(TXTLR) <= sizeof(tx->buffer) so declared counts that would place pointers outside the buffer are rejected - tx_read() now checks the return value and propagates VERROR with set_errno(EMCM_TXINVAL) Also added src/test/tx-read-unknown-type.c that exercises tx_read() with five crafted buffers (valid, unknown TXDAT_TYPE, unknown TXDSA_TYPE, both unknown, oversized MDST_COUNT) and asserts all five yield VERROR. Follows the existing src/test/_assert.h pattern. Mirror protection (verified by static walk of process_tx and confirmed at runtime): bad TXs cannot reach mirror_tx(); the vulnerability is strictly a local-crash concern, not a propagation concern. Closes #90
fix(tx): reject unsupported TXDAT_TYPE / TXDSA_TYPE in tx__init()
mdst_val__reference() used <ctype.h>'s isdigit() / isupper() on a
const char* directly. Two bugs with that:
1. Locale sensitivity. The ctype classifiers consult the process
locale's character table. A node in en_US.UTF-8 may classify a
byte differently than one in the C locale -- a consensus split on
transaction validity.
2. Undefined behavior. The C standard requires the ctype argument be
representable as unsigned char or equal EOF. Plain char is signed
on most platforms, so bytes with the high bit set get sign-extended
to negative int. Any byte >= 0x80 in the reference field is UB.
Fix: two file-local static inline helpers at the top of tx.c that take
an unsigned char and perform a pure ASCII range check:
static inline int ascii_isdigit(unsigned char c) { return c >= '0' && c <= '9'; }
static inline int ascii_isupper(unsigned char c) { return c >= 'A' && c <= 'Z'; }
mdst_val__reference() now reads reference[j] into an unsigned char c
once per iteration and uses the helpers. <ctype.h> include removed --
no remaining users.
Behavior for valid ASCII input is unchanged; non-ASCII bytes that were
previously UB are now deterministically rejected in all locales.
The helpers carry a TODO marker to promote them to extlib.h if other
units later need locale-free ASCII classification.
Closes #104
…pper fix(tx): replace locale-dependent ctype calls with ASCII range checks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Promotes four audit fixes from
audit-fixestomaster:src/tfile.cecodevolatile invalidate_tfile_pow_fp()to fix OMP cross-thread data race on the loop-exit flagsrc/network.c,src/sync.cresultinscan_quorum()into thread-privateweight_cmp+contrib; hygiene cleanup ofpeer/ecodeincatchup()src/tx.c,src/test/tx-read-unknown-type.ctx__init()before any uninitialized read; unit test addedsrc/tx.c<ctype.h>isdigit/isupperwith locale-free ASCII range helpersScope audit
All changes localized to the files each audit finding targets. No scope creep.
Artifact sweep
Verified the diff contains NO:
return VEOKat the top ofvalidate_pow())F-.. TRACE/LOCAL TEST ONLY/DO NOT COMMITinstrumentation-ftrivial-auto-var-initflags or other test-only toolingCo-Authored-By: Claudeattribution trailers (already prevented by.claude/settings.local.json)src/tfile.c:validate_pow()begins with the originalconst word32 peach_trigger[2]declaration, unchanged — no early-return bypass.Build
make NO_CUDA=1 mochimopasses cleanly on default-Werror -Wextra -Wpedanticwith gcc-13 (Ubuntu 24.04 WSL). No-Wno-errorworkaround needed.Validation (from the individual PRs)
validate_tfile_pow_fpafter thevolatilefix. Live sync with-T970000exercised the OMP loop under real PoW load on ~5000 trailers; returned VEOK, sync reached catchup cleanly.result/weight_cmp/contrib.src/test/tx-read-unknown-type.ccovers 5 crafted cases. Pre-fix run with-ftrivial-auto-var-init=patterncaptured0xfefefefefefefefepoison in uninitialized offsets; post-fix shows clean VERROR on the default branch. Live sync confirmed thousands of legitimate mainnet TXs still parse correctly (no regression).LC_ALL=CandLC_ALL=en_US.UTF-8— zero reference rejections in either. Real mainnet captures include ASCII digit strings and an uppercase string (LPPAYMENT) — both accepted identically in both locales, proving locale independence.Test plan