Repository navigation
Make the UBSan CI steps pass and require Linux CI on main - #14
Merged
Merged
Conversation
The secure_zero_local helpers added in #5 counted down with `while (len-- > 0)`, which wraps len past zero on the final test. That is well-defined C, but CI's -fsanitize=unsigned-integer-overflow with -fno-sanitize-recover treats it as fatal. Use the non-wrapping loop bitcoin_pqc_secure_zero already uses. The UBSan runtime smoke step compiled the C objects with UBSan but never passed LDFLAGS to rustc, so the Rust test binaries failed to link on undefined __ubsan_handle_* symbols. Link the UBSan runtime through RUSTFLAGS the same way the UBSan differential step does. Add "Stable CI (linux)" as a required status check in the public-main-minimal ruleset so a disabled workflow can no longer let PRs merge with no CI run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Reviewed at b69ac8b: 0 findings shared privately with maintainers. |
The cmake crate reads these variables but build.rs never declared them, so cargo reused a C library built without them. On main the "UBSan runtime smoke" step therefore ran against an uninstrumented library and passed vacuously: only secp256k1-sys was rebuilt with the sanitizer flags, which is also why that step linked in CI but not in a fresh local build. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Reviewed at 59bcb77: 0 findings shared privately with maintainers. |
Once the C library is really rebuilt with UBSan, the cmake Debug build compiles it unoptimized, and the smoke step's 13 correctness tests plus 100 sign/verify roundtrips (about 23 minutes uninstrumented) ran for over 90 minutes with no step timeout. Compile the sanitized C code at -O1, as the ASan fuzz job already does, keeping debug asserts, and give the smoke step a 60-minute timeout. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The -O1 added to CFLAGS never reached the compiler: the cmake crate drops -O flags from CFLAGS and derives the CMake build type from the cargo opt-level, so the sanitized C stayed at the Debug build's -O0 and the smoke step timed out (correctness tests 1898 s, soak unfinished after 28 minutes). Set CARGO_PROFILE_DEV_OPT_LEVEL=1 instead, which makes the cmake crate build RelWithDebInfo (-O2 -g); the C sources have no assert() or NDEBUG use, so nothing is compiled out. Locally with gcc UBSan the step's tests drop from timing out to about 90 seconds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Reviewed at 6496d79: 0 findings shared privately with maintainers. |
This was referenced Oct 30, 2026
kiwidream
added a commit
that referenced
this pull request
Nov 2, 2026
* Keep PqcError 0.3.x-compatible by mapping OUT_OF_MEMORY to Other #10 added PqcError::OutOfMemory. PqcError is not #[non_exhaustive], so a new variant breaks every downstream exhaustive match and would force main's next release to 0.4.0. Remove the variant, its Display arm and its explicit mapping, so BITCOIN_PQC_ERROR_OUT_OF_MEMORY (-6) reaches Rust as PqcError::Other(-6) through the existing catch-all. The C API keeps the new code and #10's BAD_KEY/OUT_OF_MEMORY reporting, and the dedicated variant can return in 0.4.0. A unit test checks the -6 mapping with the bindings constant. An exhaustive match over exactly the 0.3.0 variant set in algorithm_tests fails to compile if a variant is added. cargo semver-checks reports no semver update required against v0.3.0 and v0.3.1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Burn 288 KiB of released stack in unoptimized builds Clang -O0 keygen, secret-key validation and signing write about 242 KiB of stack below the entry point, more than the 208 KiB unoptimized burn, so about 34 KiB of their frames stayed unburned (0.3.1 review note N1). GCC -O0 uses about 200 KiB. Every optimized GCC 15 and Clang 21 build at -O1 through -O3, -Og and -Os stays under 13 KiB, inside the 16 KiB burn. Raise the unoptimized burn to 288 KiB, about 46 KiB above Clang, and update the secure_zero.h measurements and the slh_dsa.h stack requirement. slh_dsa_stack_scrub_test now also requires the paint below the burned window to be intact, so a call that runs deeper than the burn fails the test instead of passing on the zeroed window alone. With Clang Debug the test fails at 208 KiB (unburned stack down to 241 KiB) and passes at 288 KiB. GCC Debug passes at both sizes. Sanitizer-instrumented -O0 builds stay uncovered: Clang -O0 UBSan uses about 390 KiB. The test probe now extends at least 64 KiB past the burn, so a build that overrides BITCOIN_PQC_BURN_STACK_BYTES can still run it; Clang -O0 UBSan passes with a 448 KiB burn. Optimized builds and all outputs are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Describe the 0.3.2 changes in the provenance doc The release checklist requires vendored crypto changes to be described in docs/vendored-crypto-provenance.md. The only vendored change since 0.3.1 is #4's backend observers in sphincsplus/ref/sha2.c and sha2.h, which only test-helper builds compile. Also record the repo-owned changes that alter C behaviour: #10's BAD_KEY and OUT_OF_MEMORY reporting and the larger unoptimized stack burn. The 0.3.1 entry, with the N2 wording fix, is already on main from #16. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Release qbit-libbitcoinpqc 0.3.2 Patch release from main. On top of 0.3.1 it carries #10's C error reporting (BAD_KEY for an inconsistent secret key in sign, and the new append-only BITCOIN_PQC_ERROR_OUT_OF_MEMORY), #4's backend-selection test helpers, #13, #14's build.rs and CI changes, and the 288 KiB unoptimized stack burn. The Rust PqcError keeps the 0.3.0 variant set, and the C headers only add one enum value and two facade macros, so 0.3.2 stays semver-compatible with 0.3.x. Bump the Rust package, both Cargo.lock entries, the CMake version variables, the README and docs/versioning-and-supply-chain.md to 0.3.2. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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
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.
Addresses #12. The issue stays open until the ruleset change below is applied to the live ruleset.
Status of #12
crossbeam-epochto 0.9.20, andSupply Chain Auditpasses on main's post-merge runs.secure_zero_localhelpers insphincsplus/ref/hash_sha2.c,sphincsplus/ref/sign.candsphincsplus/sha2-avx2/hash_sha2x8.cnow use the non-wrapping loopbitcoin_pqc_secure_zeroalready uses (while (len > 0) { *p++ = 0; len--; }).RUSTFLAGS, as the UBSan differential step already does. PreviouslyLDFLAGSnever reached rustc, so the link failed on undefined__ubsan_handle_*symbols.build.rsnever declaredCC/CFLAGS/LDFLAGSas rebuild triggers, so cargo reused the C library built earlier without UBSan. On main's post-merge run, onlysecp256k1-sysrebuilt in that step, and it passed against an uninstrumented library. That is also why it linked in CI but not in a fresh local build.build.rsnow declares the three variables. Checked locally with gcc: changingCFLAGSreruns the build script, the link then fails on__ubsan_handle_*without the runtime, and passes with it.-O0, and the smoke step took 107 minutes.-Oflags inCFLAGSare dropped by the cmake crate, so the UBSan steps setCARGO_PROFILE_DEV_OPT_LEVEL=1instead. The C library then builds as RelWithDebInfo (-O2 -g), and the C sources have noassert(), so no checks are compiled out. The smoke step also gets a 60-minute timeout. Locally with gcc UBSan, its tests run in about 90 seconds.Rust CIis active again and runs on main..github/rulesets/public-main-minimal.jsonnow requiresStable CI (linux). This file is not applied automatically. Once this PR's Linux job is green, an admin needs to apply it to the live ruleset (id 18088416). Applying it before the check is green would block every PR.Not changed: the clang
-O0UBSan failure ofslh_dsa_stack_scrub_test(item 3 in #12). It is instrumentation-only, and no CI job runsctest.Verification
cargo fmt --check,cargo clippy -- -D warnings,cargo testall green; ctest 5/5, includingslh_dsa_stack_scrub_test; source inventory check passes.Stable CI (linux)run.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.