From bb3799848b28875990f7626732ecbe257c952e9e Mon Sep 17 00:00:00 2001 From: "Alex J. Malozemoff" Date: Tue, 23 Aug 2022 13:45:28 -0700 Subject: [PATCH 1/3] Use `checked_shr` instead of `>>` in `Field::random` In some edge cases, `REPR_SHAVE_BITS` could be 64, causing an overflow if we use `>>`. So use `checked_shr` instead. --- ff_derive/src/lib.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ff_derive/src/lib.rs b/ff_derive/src/lib.rs index 4c670cf..0edab29 100644 --- a/ff_derive/src/lib.rs +++ b/ff_derive/src/lib.rs @@ -1233,7 +1233,9 @@ fn prime_field_impl( }; // Mask away the unused most-significant bits. - tmp.0.as_mut()[#top_limb_index] &= 0xffffffffffffffff >> REPR_SHAVE_BITS; + // Note: In some edge cases, `REPR_SHAVE_BITS` could be 64, in which case + // `0xfff... >> REPR_SHAVE_BITS` overflows. So use `checked_shr` instead. + tmp.0.as_mut()[#top_limb_index] &= 0xffffffffffffffffu64.checked_shr(REPR_SHAVE_BITS).unwrap_or(0); if tmp.is_valid() { return tmp From 22bc4997e3d6cecb981d1b79518644246bae1612 Mon Sep 17 00:00:00 2001 From: "Alex J. Malozemoff" Date: Tue, 13 Sep 2022 13:28:10 -0700 Subject: [PATCH 2/3] Update ff_derive/src/lib.rs Co-authored-by: str4d --- ff_derive/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ff_derive/src/lib.rs b/ff_derive/src/lib.rs index 0edab29..15f213e 100644 --- a/ff_derive/src/lib.rs +++ b/ff_derive/src/lib.rs @@ -1235,6 +1235,8 @@ fn prime_field_impl( // Mask away the unused most-significant bits. // Note: In some edge cases, `REPR_SHAVE_BITS` could be 64, in which case // `0xfff... >> REPR_SHAVE_BITS` overflows. So use `checked_shr` instead. + // This is always sufficient because we will have at most one spare limb + // to accommodate values of up to twice the modulus. tmp.0.as_mut()[#top_limb_index] &= 0xffffffffffffffffu64.checked_shr(REPR_SHAVE_BITS).unwrap_or(0); if tmp.is_valid() { From 732b862b63cabbb14297a84fd9a99816f8d7c5ac Mon Sep 17 00:00:00 2001 From: "Alex J. Malozemoff" Date: Tue, 13 Sep 2022 13:33:55 -0700 Subject: [PATCH 3/3] add test case --- Cargo.toml | 3 +++ tests/derive.rs | 16 ++++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index 3d41713..b112f6a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -20,6 +20,9 @@ ff_derive = { version = "0.12", path = "ff_derive", optional = true } rand_core = { version = "0.6", default-features = false } subtle = { version = "2.2.1", default-features = false, features = ["i128"] } +[dev-dependencies] +rand = "0.8" + [features] default = ["bits", "std"] alloc = [] diff --git a/tests/derive.rs b/tests/derive.rs index 545d074..bfa2cd2 100644 --- a/tests/derive.rs +++ b/tests/derive.rs @@ -21,6 +21,22 @@ mod fermat { struct Fermat65537Field([u64; 1]); } +mod full_limbs { + #[derive(PrimeField)] + #[PrimeFieldModulus = "39402006196394479212279040100143613805079739270465446667948293404245721771496870329047266088258938001861606973112319"] + #[PrimeFieldGenerator = "19"] + #[PrimeFieldReprEndianness = "little"] + struct F384p([u64; 7]); + + #[test] + fn random_masking_does_not_overflow() { + use ff::Field; + use rand::rngs::OsRng; + + let _ = F384p::random(OsRng); + } +} + #[test] fn batch_inversion() { use ff::{BatchInverter, Field};