Skip to content

fix: correct classgroup GMP FFI declarations and uninitialized values - #605

Open
blacks1ne wants to merge 1 commit into
QuilibriumNetwork:v2.1.0.25from
blacks1ne:fix/classgroup-gmp-ffi-soundness
Open

fix: correct classgroup GMP FFI declarations and uninitialized values#605
blacks1ne wants to merge 1 commit into
QuilibriumNetwork:v2.1.0.25from
blacks1ne:fix/classgroup-gmp-ffi-soundness

Conversation

@blacks1ne

Copy link
Copy Markdown
Contributor

These are the sixteen warnings the rest of the warning-cleanup series
deliberately left alone. I said they would come as an issue with analysis
rather than a PR, because a plausible-looking fix in GMP FFI glue produces
silent memory corruption rather than a compile error, and because the crate is
vendored.

Both reasons turned out not to survive checking, so here is the PR instead.

__gmpz_cmpabs is declared wrongly in both extern blocks

src/gmp/mpz.rs and src/gmp_classgroup/ffi.rs each open their own
extern "C" block, and three symbols appear in both with different signatures.
rustc only warns, but it is entitled to assume either one.

symbol gmp/mpz.rs gmp_classgroup/ffi.rs GMP 6.3.0 gmp.h
__gmpz_cmpabs (by: mpz_ptr, l: mpz_srcptr) — no return (rop: *const Mpz, op: *const Mpz) -> usize int mpz_cmpabs (mpz_srcptr, mpz_srcptr)
__gmpz_sizeinbase (op: mpz_srcptr, base: c_int) -> size_t (op: &Mpz, base: c_int) -> size_t size_t mpz_sizeinbase (mpz_srcptr, int)
__gmpz_export (..., op: mpz_srcptr)no return (..., op: &Mpz) -> *mut c_void void *mpz_export (..., mpz_srcptr)

mpz_cmpabs returns a negative int when |op1| < |op2|. Declared as
returning usize, that case cannot be represented. I checked what it actually
produces, with both declarations against the same GMP the build image links:

case                     correct (c_int)   broken (usize)
|5| vs |7|  (expect < 0)              -1             4294967295
|7| vs |5|  (expect > 0)               1                      1
|5| vs |5|  (expect = 0)               0                      0
|-7| vs |5| (expect > 0)               1                      1
|5| vs |-7| (expect < 0)              -1             4294967295

So every "less than" answer comes back as a large positive number — the exact
inputs where the comparison matters. It reads 4294967295 rather than
18446744073709551615 only because the upper half of the return register
happened to be zero on this run; nothing guarantees that.

The declaration in gmp/mpz.rs is wrong differently: no return value at all,
and a mutable first operand.

This is latent rather than live — the sole wrapper is dead code, and grep
finds no caller of mpz_cmpabs anywhere in the workspace. That is what makes
it cheap to fix now and expensive to fix later: correcting a dead function's
signature costs nothing today, and the first caller to use it would silently
get wrong answers.

__gmpz_sizeinbase and __gmpz_export are the benign pair — Mpz is
#[repr(transparent)] over #[repr(C)] mpz_struct, so &Mpz and
*const mpz_struct have identical ABI. But gmp/mpz.rs omits __gmpz_export's
void * return, and export_obj asserts on that return, so the unified
declaration has to be the one that keeps it.

All three now have a single declaration in gmp/mpz.rs, imported by
gmp_classgroup/ffi.rs and reached through the Mpz::inner() accessor that
already existed for exactly this purpose. mpz_cmpabs returns Ordering.

Six mem::uninitialized() calls

Deprecated since Rust 1.39 and immediate UB for any type with a validity
invariant. Five of them build an mpz_struct, whose *mut c_void is a scalar —
producing an uninitialised one is UB the moment the value exists, before
__gmpz_init can overwrite it. The window is one line and GMP does initialise
the struct, so this has probably never misbehaved in a shipped build; that is
not the same as being correct, and the classic symptom is a miscompile that
appears under a new LLVM.

All five become MaybeUninit. from_str_radix is the one that needed care:

let r = __gmpz_init_set_str(mpz.as_mut_ptr(), s.as_ptr(), base as c_int);
let mut mpz = mpz.assume_init();   // <- before the branch, deliberately
if r == 0 { Ok(Mpz { mpz }) } else { __gmpz_clear(&mut mpz); ... }

The error branch calls __gmpz_clear, legal only on an initialised mpz_t.
GMP 6.3.0's manual, Simultaneous Integer Init & Assign:

If the string is a correct base BASE number, the function returns 0; if an
error occurs it returns -1. ROP is initialized even if an error occurs.
(I.e., you have to call 'mpz_clear' for it.)

So assume_init() belongs before the branch and the existing __gmpz_clear is
required, not incidental. Moving assume_init() into the success arm — which
looks tidier — would leak on every malformed input.

The sixth is a usize word count that __gmpz_export always writes, and which
GMP sets to 0 for a zero input. It is now plainly 0 rather than uninitialised.

Error::description

Deprecated since Rust 1.42. The message moves into the Display impl, which
previously delegated to it.

Tests

Every change has a test that fails without it.

  • cmpabs_orders_by_magnitude — all three orderings. Cannot compile against
    an unsigned return type, and I confirmed that: restoring the old signature
    fails the build with E0308 rather than passing quietly.
  • cmpabs_ignores_sign|-9| == |9|, and -9 < -2 while |-9| > |-2|.
  • cmpabs_handles_multi_limb_values — magnitudes past one limb, where a
    truncated or half-read return register would show up.
  • export_obj_round_trips — export/import over positive, negative and
    multi-limb values. The negative cases matter here: export_obj internally
    exports a zero for them, which is the path where GMP writes a count of 0
    and where the previously-uninitialised local was read.
  • export_obj_reports_required_size — undersized buffer reports the size
    needed instead of writing past it.
  • test_constructors_are_initializednew, new_reserve, one are read,
    mutated, grown past their reservation and dropped, not merely compared once.
  • test_clone_is_independent — mutating a clone must not disturb the source,
    i.e. the limb pointer was really deep-copied.
  • test_from_str_radix_error_path — 1000 failed parses, then a successful
    one, so a leak on the error path is large enough for a leak checker and the
    allocator is asserted healthy afterwards. Also covers a digit invalid for its
    base, not just junk.
  • test_parse_error_message — pins "invalid integer" to Display, which
    is what silently changes if description is removed carelessly.

Verification

In the AMD64 build image (Dockerfile.source, test-context), which is where
this crate can compile at all — it needs GMP/FLINT/MPFR:

cargo check -p classgroup --lib --bins --tests --examples   # 16 warnings -> 0
cargo nextest run -p classgroup                              # 69 tests, 69 passed

That takes the workspace to zero warnings across the whole cleanup series.

GMP prototypes and manual quotes above are from GMP 6.3.0 —
/usr/local/include/gmp.h in that image, which is the source-built GMP the
crate actually links against, not the distro 6.2.1 also present.

Note on #600, and a correction

#600 adds a [lints.rust] table to crates/classgroup/Cargo.toml whose comment
justified allow-rather-than-patch by saying it keeps the tree byte-comparable
with poanetwork/vdf. That was wrong and I have corrected it on that branch.
classgroup has already diverged — src/vdf.cpp and build.rs have both been
substantially rewritten in-tree — so wholesale re-vendoring was never on the
table, and "it is vendored, do not touch it" was not a real reason to leave a
genuine defect in place. That is what changed my mind about filing this as an
issue.

The load-bearing half of that comment is unchanged and still true: deprecated = "allow" there does not reach src/gmp/, because that module's own
#![warn(deprecated)] beats the level cargo passes on the command line. The key
covers build.rs and must not be deleted as dead config. I deleted it once on
that assumption and had to put it back.

The two PRs touch different files in the crate and do not conflict in either
merge order.

`__gmpz_cmpabs` was declared in two extern blocks with two different, both
wrong, signatures. GMP's is `int mpz_cmpabs (mpz_srcptr, mpz_srcptr)`, which
returns a negative value when |op1| < |op2|:

  gmp/mpz.rs             fn __gmpz_cmpabs(by: mpz_ptr, l: mpz_srcptr);
  gmp_classgroup/ffi.rs  fn __gmpz_cmpabs(rop: *const Mpz, op: *const Mpz) -> usize;

The first declares no return and takes a mutable first operand; the second
returns `usize`, which cannot represent a negative result. Comparing |5| with
|7| through it yields 4294967295 instead of -1 — and the upper half of the
return register is not guaranteed zero, so the garbage is not even stable.
The sole wrapper was dead code, so this was latent rather than live.

`__gmpz_sizeinbase` and `__gmpz_export` were also declared twice. Those two
are ABI-identical (`Mpz` is `#[repr(transparent)]` over `#[repr(C)]`
`mpz_struct`), but `gmp/mpz.rs` omitted `__gmpz_export`'s `void *` return,
which `export_obj` asserts on. All three now have one declaration in
`gmp/mpz.rs`, imported by `gmp_classgroup/ffi.rs` and reached through the
existing `Mpz::inner()` accessor. `mpz_cmpabs` returns `Ordering`.

Six `mem::uninitialized()` calls become `MaybeUninit`. Five construct an
`mpz_struct`, whose `*mut c_void` makes producing an uninitialised value UB
before GMP can overwrite it. `from_str_radix` keeps `assume_init()` ahead of
its success/failure branch: GMP documents that `mpz_init_set_str` initialises
`rop` even on a parse error, so the existing `__gmpz_clear` on that path is
required, not incidental. The sixth is a `usize` count that `__gmpz_export`
always writes, now plainly `0`.

`Error::description` is deprecated; its message moves into `Display`, which
previously delegated to it.

Adds non-regression tests for each: `mpz_cmpabs` across all three orderings
and both signs, including multi-limb values; export/import round trips over
positive, negative and zero-exporting inputs; constructor initialisation and
clone independence; the `from_str_radix` error path; and the parse error's
message. The cmpabs tests cannot compile against an unsigned return type.

Verified in the AMD64 build image: classgroup goes from 16 warnings to 0,
69/69 tests pass.
@blacks1ne
blacks1ne force-pushed the fix/classgroup-gmp-ffi-soundness branch from 980f4a3 to 1dbb2ad Compare August 18, 2026 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant