chore: replace the unsound static-mut and ignored-Result patterns - #604
Open
blacks1ne wants to merge 1 commit into
Open
chore: replace the unsound static-mut and ignored-Result patterns#604blacks1ne wants to merge 1 commit into
blacks1ne wants to merge 1 commit into
Conversation
Ten soundness warnings, each fixed rather than silenced.
static_mut_refs (5). Two lazily-initialised process globals used the
'static mut + Once + assume_init' pattern, which needs unsafe at every access
and has no synchronisation on the read side:
- bls::singleton()'s SINGLETON becomes a OnceLock<SingletonKZGSetup>.
get_or_init returns &'static T directly, so the function keeps its exact
signature and loses its unsafe block entirely.
- bls256's G2_TAB becomes a OnceLock<[FP16; G2_TABLE]>. init() fills a local
and publishes it; the two verification paths take it via
.expect("bls256::init() must run before verification"). Previously a
missing init() meant silently pairing against an all-zero table; now it is
a loud panic. init() is called from exactly one place (inside
singleton()'s initialiser), so the ordering is unchanged.
unused_must_use (2). doubleratchet::ratchet_ephemeral_keys ignored both
hkdf.expand results. They cannot actually fail here — expand only errors when
the output exceeds 255*HashLen, and 96 is a constant well under SHA-512's
16320 — but the ratchet silently continued with a zeroed key buffer if it
ever did. The enclosing function already returns Result, so the error is
propagated with a note explaining why it is unreachable.
unused_assignments (3).
- lattice_ct::decode_pending_claim initialised 'p' to 0 and immediately
overwrote it with 32; it now starts at 32.
- clock::commit_shard_clock_frame's have_frame was initialised to false and
assigned on both branches; it is now a plain uninitialised binding, so the
compiler enforces that every path sets it. That flag guards the
latest-index pointer against an attacker-chosen frame_number, so a future
path that forgets to set it should not silently inherit 'false'.
- addressing::location_clone_is_independent mutated 'a' and never checked
it. The test now asserts on both halves, which is what it was for.
(cherry picked from commit f5522adcb2fbf1da39a0833ddd1daacfbe6e2d79)
blacks1ne
force-pushed
the
chore/warnings-soundness
branch
from
August 18, 2026 10:35
20a82b5 to
85c74fa
Compare
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.
10 warnings, fixed rather than silenced. This is the only PR in the series
besides #597 that changes runtime behaviour, so it is worth a closer read than
the rest.
static_mut_refs(5).bls::singleton'sSINGLETONandbls256'sG2_TABboth used
static mut+Once+assume_init, which needsunsafeat everyaccess and has no synchronisation on the read side. Both become
OnceLock.get_or_initreturns&'static T, sosingleton()keeps its exact signatureand loses its
unsafeblock entirely.bls256::init()is called from exactlyone place — inside
singleton()'s initialiser — so the ordering is unchanged; amissing
init()now panics loudly instead of silently pairing against anall-zero table. (
static_mut_refsbecomes a hard error in edition 2024, so thishas to happen eventually either way.)
unused_must_use(2).doubleratchet::ratchet_ephemeral_keysdiscarded bothhkdf.expandresults. They cannot fail here — 96 bytes is well under SHA-512's16320-byte limit — but if they ever did, the ratchet would have continued with a
zeroed key buffer. The function already returns
Result, so they are propagatedwith the bound spelled out in a comment.
unused_assignments(3). Includingclock.rs'shave_frame, which is nowan uninitialised binding so the compiler enforces that every path sets it. That
flag guards the latest-index pointer against an attacker-chosen
frame_number,so a future path that forgets to set it should not silently inherit
false.The full test suite passes on this — see the verification below — which is the
main evidence that the
OnceLockordering is right.Series
Part of the warning-cleanup series that starts with #597. The eight PRs are
disjoint and each stands on its own, but they are meant to be read in order —
please take #597 first: it is the only one of the eight that fixes a bug
rather than a warning, and it is the shortest.
932045a3the 16 being
classgroup's GMP FFI glue, deliberately left visible rather thansilenced. Update: those 16 are now fixed rather than documented, in fix: correct classgroup GMP FFI declarations and uninitialized values #605,
which corrects the declarations and the uninitialized values instead of
annotating them. With fix: correct classgroup GMP FFI declarations and uninitialized values #605 and the channelwasm commit in chore: drop unused imports and bindings in the crypto crates #599, the combined
tree checks with zero warnings.