Guard bedrock2 C output against mismatched target word width - #2405
Draft
JasonGross wants to merge 2 commits into
Draft
Guard bedrock2 C output against mismatched target word width#2405JasonGross wants to merge 2 commits into
JasonGross wants to merge 2 commits into
Conversation
The bedrock2 C backend types every word as br_word_t, an alias of
uintptr_t, and the prelude's _br_load/_br_store access sizeof(br_word_t)
bytes. The limb stride and buffer sizes baked into the generated code,
however, come from the synthesis word size (the "32"/"64" in the file
name). Compiling one of the fiat-bedrock2/src/*_32.c files on a 64-bit
target therefore reads and writes 4 bytes past the end of every caller
buffer and computes wrong field arithmetic, while the prelude's
static_assert(UINTPTR_MAX <= BR_WORD_MAX, ...) is vacuous because
BR_WORD_MAX is defined as UINTPTR_MAX. Nothing caught this: the
project's own test-bedrock2-files target compiled the _32 files on
x86-64 without complaint (scrutineer finding #2509).
Emit, from the fiat-crypto side of the backend (the ToString.header hook
in src/Bedrock/Field/Stringification/Stringification.v, which already
receives machine_wordsize), an additional guard after the bedrock2
prelude:
static_assert(BR_WORD_MAX == UINT32_MAX, "this file was synthesized
for 32-bit words; ...");
(and the UINT64_MAX counterpart for 64-bit files), so that a word-width
mismatch is a compile-time error instead of a silent memory-safety and
correctness bug. The vacuous assert lives in the bedrock2 submodule's
ToCString.prelude and is left untouched here.
Regenerate all 31 fiat-bedrock2/src/*.c files with rebuilt synthesis
binaries; the only change to each file is the new comment and
static_assert.
Make the test-bedrock2-files / only-test-bedrock2-files rule compile the
*_32.c files with -m32 (BEDROCK2_CFLAGS_32, overridable) and the *_64.c
files with the host default (BEDROCK2_CFLAGS_64), and install
gcc-multilib in the C workflow so that CI keeps compiling every file
while actually honouring the new guard. Document in the README that the
*_32.c files are for 32-bit targets only.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016Rbn2gww3MGhvrh52fNjpD
Extend test-bedrock2-files / only-test-bedrock2-files with a negative check: compile one *_32.c file with the 64-bit flags and one *_64.c file with the 32-bit flags, and require each to fail with the "synthesized for NN-bit words" static_assert message (failing for any other reason, such as a missing multilib, is reported separately). This confirms the guard introduced for scrutineer finding #2509 actually fires, rather than only checking that correctly-targeted builds succeed. The idea of a negative test is adopted from the codex/fix-2509 branch. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Rbn2gww3MGhvrh52fNjpD
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.
Fixes scrutineer finding #2509.
The bug
The bedrock2 C backend types every word as
br_word_t, an alias ofuintptr_t, and the prelude's_br_load/_br_storeaccesssizeof(br_word_t)bytes. The limb stride and buffer sizes baked into the generated code, however, come from the synthesis word size (the32/64in the file name). Compiling one of thefiat-bedrock2/src/*_32.cfiles on a 64-bit target therefore reads and writes 4 bytes past the end of every caller buffer (160 of the 183 public wrappers write out of bounds) and computes wrong field arithmetic even when buffers are over-allocated (e.g.mul(R, R) != R). The prelude'sstatic_assert(UINTPTR_MAX <= BR_WORD_MAX, ...)cannot catch this becauseBR_WORD_MAXis#defined asUINTPTR_MAX. Nothing else caught it either:make only-test-bedrock2-filescompiled the 14_32files on x86-64 CI without complaint, and no documentation said the_32files require a 32-bit target.The fix
src/Bedrock/Field/Stringification/Stringification.v): theToString.headerhook ofOutputBedrock2APIalready receivesmachine_wordsizebut discarded it. It now appends, after the bedrock2 prelude, a guard tied to the synthesis word size:UINT64_MAXcounterpart for 64-bit files. A word-width mismatch is now a compile-time error in both directions instead of a silent memory-safety and correctness bug. TheBR_WORD_MAX == UINTnn_MAXform matches how the prelude itself selects its_br_mulhuuimplementation.fiat-bedrock2/src/*.cregenerated with rebuilt synthesis binaries. The diff to each file is exactly the blank line, the comment and thestatic_assert(93 insertions, 0 deletions across 31 files).Makefile.examples):test-bedrock2-files/only-test-bedrock2-filesnow compile the*_64.cfiles with the host default (BEDROCK2_CFLAGS_64, empty) and the*_32.cfiles withBEDROCK2_CFLAGS_32(default-m32); both variables are overridable for cross compilers. A make-time check errors out if a bedrock2 file name does not end in_32/_64. The alternative of simply skipping the_32files would have removed the only compile coverage they have;-m32keeps CI compiling every file while honouring the new guard._32file with the 64-bit flags and one_64file with the 32-bit flags and requires each to fail with thesynthesized for NN-bit wordsmessage, so CI checks that the guard actually fires and not only that correctly-targeted builds succeed. The idea of a negative test is adopted from thecodex/fix-2509-bedrock2-word-widthbranch..github/workflows/c.yml): installgcc-multilibso-m32works onubuntu-latest.README.md): note in the bedrock2 section that*_32.cfiles are for 32-bit targets and*_64.cfiles for 64-bit targets, and that each file rejects a mismatched target at compile time.The vacuous
static_assert(UINTPTR_MAX <= BR_WORD_MAX, "pointer fits in int")is emitted from the bedrock2 submodule'sToCString.prelude, not from fiat-crypto, so it is left untouched here; it could be dropped upstream.Verification
All on this x86-64 host (gcc 12.2, clang available, gcc multilib installed).
src/Bedrock/Field/Stringification/Stringification.vandsrc/Bedrock/Standalone/StandaloneOCamlMain.vwithcoqcagainst the installedcoq-fiat-crypto-with-bedrocklibrary (overlay build; the only upstream file that differs from HEAD in the installed copy issrc/Bedrock/Field/Common/Types.v,#[export]vs#[global]on instance attributes, which cannot affect emitted text), extracted and builtbedrock2_unsaturated_solinas,bedrock2_word_by_word_montgomery,bedrock2_dettman_multiplicationandbedrock2_solinas_reductionwith the flags fromMakefile.standalone, then ranmake -f Makefile.examples bedrock2-fileswith the fourBEDROCK2_*variables pointed at those binaries.git diff -- fiat-bedrock2 | grep '^[+-]'shows only the three new lines per file.cc -c fiat-bedrock2/src/p256_64.c: succeeds.cc -m32 -c p256_64.c: fails withstatic assertion failed: "this file was synthesized for 64-bit words; ...".cc -c fiat-bedrock2/src/p256_32.c: fails withstatic assertion failed: "this file was synthesized for 32-bit words; ...".cc -m32 -c p256_32.c: succeeds. Same behaviour with clang.make -f Makefile.examples only-test-bedrock2-files SKIP_INCLUDE=1: exit 0, 31 object files produced (17_64at host width, 14_32under-m32), negative check passes. WithBEDROCK2_CFLAGS_32=-m64orBEDROCK2_CFLAGS_64=-m32the rule fails at the compile step with the static_assert message. The negative check's own failure branches were exercised from a scratch makefile: swapped flags report "compiled ... but should have been rejected", and a bogus flag reports "rejected ... for the wrong reason" with the compiler output.mul(R, R) == Rviafiat_p256_set_one/fiat_p256_mulon the regenerated files:p256_32.cunder-m32reportssizeof(br_word_t)=4 limbs=8 wrong=0;p256_64.creportssizeof(br_word_t)=8 limbs=4 wrong=0(the finding's reproducer reported 8 of 8 limbs wrong for the_32file on 64-bit).Not verified: the full
makeof the Coq development (only the changed file and its downstream cone were compiled); the Haskell and js_of_ocaml standalone builds, which shareStandaloneOCamlMain.v's dependency on the changed file but were not rebuilt; the CI run itself withgcc-multilib(the apt package name is the standard Debian/Ubuntu one and the same flag was exercised locally).🤖 Generated with Claude Code
https://claude.ai/code/session_016Rbn2gww3MGhvrh52fNjpD