Fix warnings#1002
Merged
openshift-merge-bot[bot] merged 3 commits intorh-ecosystem-edge:mainfrom Mar 17, 2026
Merged
Conversation
WalkthroughRepository-wide maintenance: dependency bumps and vendored crate upgrades; removal of vendored Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@vendor/num-bigint/src/bigint/division.rs`:
- Around line 466-469: The checked_div_rem_euclid implementation lacks a
zero-divisor check and will panic when v is zero; update checked_div_rem_euclid
to mirror checked_div_euclid/checked_rem_euclid by returning None if
v.is_zero(), otherwise return Some(self.div_rem_euclid(v)); refer to the
checked_div_rem_euclid and div_rem_euclid functions and reuse the same zero
check pattern as in checked_div_euclid/checked_rem_euclid.
In `@vendor/rsa/src/algorithms/rsa.rs`:
- Around line 286-287: Fix the typo in the documentation comment that reads
"totiem" — replace it with "totient" in the comment starting "FIPS 186-4
**requires** the private exponent to be less than λ(n), which would make Euler's
totiem unreliable." in vendor/rsa/src/algorithms/rsa.rs so the sentence reads
"...which would make Euler's totient unreliable."
🧹 Nitpick comments (4)
src/cluster_crypto/json_crawl.rs (1)
292-299: Consider using.ok()instead of suppressing the lint.The
#[allow(clippy::manual_ok_err)]suppression works, but the clippy suggestion would actually simplify this code while preserving the comment context.♻️ Optional refactor
- #[allow(clippy::manual_ok_err)] - if let Ok(decoded) = String::from_utf8(decoded) { - Some(decoded) - } else { - // We don't search for crypto objects inside binaries - None - } + // We don't search for crypto objects inside binaries, so non-UTF8 returns None + String::from_utf8(decoded).ok()vendor/num-bigint/src/bigrand.rs (1)
52-87: Approve with note on unsafe code complexity.The
cfg_digit!macro dispatches between two implementations based on the target's native digit size. The 64-bit path usesunsafepointer casting fromVec<u64>to a&mut [u32]slice forgen_bits, which is sound because:
- Memory alignment is satisfied (u64 alignment ≥ u32 alignment)
- The
debug_assert!(native_len * 2 >= len)ensures the buffer is large enough- Big-endian targets correctly swap the u32 halves within each u64 (line 83)
The implementation is correct but subtle. Consider adding a brief inline comment explaining why the endian swap is needed (i.e.,
gen_bitsfills u32 slots in logical order, but on big-endian the memory layout of u64 requires swapping the halves).src/protobuf_gen/k8s.io.api.core.v1.rs (1)
7-7510: Consider a module-leveldead_codeallow to reduce regen diff churn (if possible).If your codegen setup allows it, a single
#![allow(dead_code)]at the module/file level (or in themod protobuf_gen { ... }wrapper) can avoid touching every generated type and keep future regen diffs smaller. If not feasible with your currentprost_buildpipeline, this per-type approach is still fine.vendor/num-bigint/src/biguint/iter.rs (1)
321-340: Testtest_iter_u32_digits_bedoesn't actually test big-endian iteration.This test is identical to
test_iter_u32_digits(lines 279-298) and only usesnext(), notnext_back(). Compare withtest_iter_u64_digits_be(lines 342-361) which correctly testsnext_back()for reverse iteration.Since this is vendored code from an upstream library, this is likely an upstream issue. If you're maintaining this vendored copy, consider fixing the test to actually verify big-endian (reverse) iteration using
next_back().
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockvendor/num-bigint-dig/Cargo.lockis excluded by!**/*.lockvendor/rsa/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (131)
Cargo.tomlbuild.rssrc/cluster_crypto.rssrc/cluster_crypto/crypto_objects.rssrc/cluster_crypto/json_crawl.rssrc/ocp_postprocess/ip_rename/etcd_rename.rssrc/protobuf_gen/github.com.openshift.api.oauth.v1.rssrc/protobuf_gen/github.com.openshift.api.route.v1.rssrc/protobuf_gen/k8s.io.api.admissionregistration.v1.rssrc/protobuf_gen/k8s.io.api.apps.v1.rssrc/protobuf_gen/k8s.io.api.batch.v1.rssrc/protobuf_gen/k8s.io.api.core.v1.rssrc/protobuf_gen/k8s.io.apimachinery.pkg.api.resource.rssrc/protobuf_gen/k8s.io.apimachinery.pkg.apis.meta.v1.rssrc/protobuf_gen/k8s.io.apimachinery.pkg.runtime.rssrc/protobuf_gen/k8s.io.apimachinery.pkg.util.intstr.rsvendor/byteorder/.cargo-checksum.jsonvendor/byteorder/.cargo_vcs_info.jsonvendor/byteorder/.github/workflows/ci.ymlvendor/byteorder/CHANGELOG.mdvendor/byteorder/COPYINGvendor/byteorder/Cargo.tomlvendor/byteorder/Cargo.toml.origvendor/byteorder/LICENSE-MITvendor/byteorder/README.mdvendor/byteorder/UNLICENSEvendor/byteorder/benches/bench.rsvendor/byteorder/rustfmt.tomlvendor/byteorder/src/io.rsvendor/byteorder/src/lib.rsvendor/num-bigint-dig/.cargo-checksum.jsonvendor/num-bigint-dig/.cargo_vcs_info.jsonvendor/num-bigint-dig/Cargo.tomlvendor/num-bigint-dig/Cargo.toml.origvendor/num-bigint-dig/src/algorithms/mod_inverse.rsvendor/num-bigint-dig/src/bigrand.rsvendor/num-bigint-dig/src/biguint.rsvendor/num-bigint-dig/src/prime.rsvendor/num-bigint-dig/tests/torture.rsvendor/num-bigint/.cargo-checksum.jsonvendor/num-bigint/.cargo_vcs_info.jsonvendor/num-bigint/Cargo.tomlvendor/num-bigint/Cargo.toml.origvendor/num-bigint/README.mdvendor/num-bigint/RELEASES.mdvendor/num-bigint/benches/bigint.rsvendor/num-bigint/build.rsvendor/num-bigint/src/bigint.rsvendor/num-bigint/src/bigint/addition.rsvendor/num-bigint/src/bigint/arbitrary.rsvendor/num-bigint/src/bigint/bits.rsvendor/num-bigint/src/bigint/convert.rsvendor/num-bigint/src/bigint/division.rsvendor/num-bigint/src/bigint/power.rsvendor/num-bigint/src/bigint/serde.rsvendor/num-bigint/src/bigint/subtraction.rsvendor/num-bigint/src/bigrand.rsvendor/num-bigint/src/biguint.rsvendor/num-bigint/src/biguint/addition.rsvendor/num-bigint/src/biguint/arbitrary.rsvendor/num-bigint/src/biguint/convert.rsvendor/num-bigint/src/biguint/division.rsvendor/num-bigint/src/biguint/iter.rsvendor/num-bigint/src/biguint/monty.rsvendor/num-bigint/src/biguint/multiplication.rsvendor/num-bigint/src/biguint/power.rsvendor/num-bigint/src/biguint/serde.rsvendor/num-bigint/src/biguint/shift.rsvendor/num-bigint/src/biguint/subtraction.rsvendor/num-bigint/src/lib.rsvendor/num-bigint/src/macros.rsvendor/num-bigint/tests/bigint.rsvendor/num-bigint/tests/bigint_bitwise.rsvendor/num-bigint/tests/biguint.rsvendor/num-bigint/tests/consts/mod.rsvendor/num-bigint/tests/roots.rsvendor/num-integer/.cargo-checksum.jsonvendor/num-integer/.cargo_vcs_info.jsonvendor/num-integer/Cargo.tomlvendor/num-integer/Cargo.toml.origvendor/num-integer/README.mdvendor/num-integer/RELEASES.mdvendor/num-integer/benches/average.rsvendor/num-integer/benches/gcd.rsvendor/num-integer/benches/roots.rsvendor/num-integer/build.rsvendor/num-integer/src/average.rsvendor/num-integer/src/lib.rsvendor/num-integer/src/roots.rsvendor/num-integer/tests/average.rsvendor/num-integer/tests/roots.rsvendor/num-traits/.cargo-checksum.jsonvendor/num-traits/.cargo_vcs_info.jsonvendor/num-traits/Cargo.tomlvendor/num-traits/Cargo.toml.origvendor/num-traits/README.mdvendor/num-traits/RELEASES.mdvendor/num-traits/build.rsvendor/num-traits/src/cast.rsvendor/num-traits/src/float.rsvendor/num-traits/src/identities.rsvendor/num-traits/src/int.rsvendor/num-traits/src/lib.rsvendor/num-traits/src/ops/bytes.rsvendor/num-traits/src/ops/euclid.rsvendor/num-traits/src/real.rsvendor/rsa/.cargo-checksum.jsonvendor/rsa/.cargo_vcs_info.jsonvendor/rsa/CHANGELOG.mdvendor/rsa/Cargo.tomlvendor/rsa/Cargo.toml.origvendor/rsa/README.mdvendor/rsa/src/algorithms/generate.rsvendor/rsa/src/algorithms/pkcs1v15.rsvendor/rsa/src/algorithms/rsa.rsvendor/rsa/src/errors.rsvendor/rsa/src/key.rsvendor/rsa/src/lib.rsvendor/rsa/src/oaep.rsvendor/rsa/src/pkcs1v15.rsvendor/rsa/src/pkcs1v15/signature.rsvendor/rsa/src/pss.rsvendor/rsa/src/pss/signature.rsvendor/rsa/tests/examples/pkcs8/rsa2048-sp800-56b-priv.dervendor/spki/.cargo-checksum.jsonvendor/spki/.cargo_vcs_info.jsonvendor/spki/CHANGELOG.mdvendor/spki/Cargo.tomlvendor/spki/Cargo.toml.origvendor/spki/src/spki.rsvendor/spki/tests/spki.rs
💤 Files with no reviewable changes (23)
- vendor/num-bigint/tests/biguint.rs
- vendor/byteorder/CHANGELOG.md
- vendor/num-bigint/build.rs
- vendor/byteorder/COPYING
- vendor/byteorder/README.md
- vendor/byteorder/.cargo-checksum.json
- vendor/byteorder/Cargo.toml.orig
- vendor/num-traits/src/cast.rs
- vendor/num-traits/src/int.rs
- vendor/num-bigint/tests/roots.rs
- vendor/byteorder/.github/workflows/ci.yml
- vendor/num-bigint/tests/bigint.rs
- vendor/num-integer/benches/gcd.rs
- vendor/byteorder/benches/bench.rs
- vendor/num-integer/build.rs
- vendor/byteorder/src/io.rs
- vendor/byteorder/UNLICENSE
- vendor/byteorder/LICENSE-MIT
- vendor/byteorder/rustfmt.toml
- vendor/num-bigint/tests/bigint_bitwise.rs
- vendor/num-traits/src/ops/bytes.rs
- vendor/byteorder/.cargo_vcs_info.json
- vendor/byteorder/Cargo.toml
🧰 Additional context used
🧬 Code graph analysis (12)
vendor/num-bigint/benches/bigint.rs (1)
vendor/num-integer/benches/roots.rs (1)
bench(16-46)
vendor/rsa/src/algorithms/generate.rs (1)
vendor/rsa/src/algorithms/rsa.rs (2)
compute_modulus(249-251)compute_private_exponent_euler_totient(256-278)
vendor/num-bigint/src/biguint/serde.rs (1)
vendor/num-bigint/src/biguint.rs (1)
biguint_from_vec(526-528)
vendor/num-bigint/src/biguint/arbitrary.rs (1)
vendor/num-bigint/src/biguint.rs (1)
biguint_from_vec(526-528)
vendor/num-bigint/src/bigrand.rs (1)
vendor/num-bigint/src/biguint.rs (1)
biguint_from_vec(526-528)
vendor/num-bigint/src/biguint/addition.rs (2)
vendor/num-bigint/src/lib.rs (1)
from_doublebigdigit(259-261)vendor/num-bigint-dig/src/biguint.rs (1)
u32_from_u128(2530-2537)
vendor/rsa/src/key.rs (1)
vendor/rsa/src/algorithms/rsa.rs (4)
compute_modulus(249-251)compute_private_exponent_carmicheal(289-304)compute_private_exponent_euler_totient(256-278)recover_primes(202-246)
vendor/num-bigint/src/biguint/convert.rs (2)
vendor/num-bigint/src/biguint/division.rs (1)
div_rem_digit(85-107)vendor/num-bigint/src/biguint/multiplication.rs (1)
mac_with_carry(15-26)
vendor/num-bigint/src/biguint/multiplication.rs (2)
vendor/num-bigint/src/lib.rs (1)
from_doublebigdigit(259-261)vendor/num-bigint-dig/src/biguint.rs (1)
u32_from_u128(2530-2537)
vendor/num-bigint/src/biguint/subtraction.rs (2)
vendor/num-bigint/src/lib.rs (1)
from_doublebigdigit(259-261)vendor/num-bigint-dig/src/biguint.rs (1)
u32_from_u128(2530-2537)
vendor/num-bigint/src/biguint.rs (1)
vendor/num-bigint-dig/src/biguint.rs (2)
u32_to_u128(2522-2524)u32_from_u128(2530-2537)
vendor/num-bigint/src/biguint/division.rs (2)
vendor/num-bigint/src/lib.rs (1)
to_doublebigdigit(265-267)vendor/num-bigint-dig/src/biguint.rs (1)
u32_to_u128(2522-2524)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Red Hat Konflux / recert-4-22-on-pull-request
- GitHub Check: build
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Comment on lines
+466
to
+469
|
|
||
| fn checked_div_rem_euclid(&self, v: &Self) -> Option<(Self, Self)> { | ||
| Some(self.div_rem_euclid(v)) | ||
| } |
There was a problem hiding this comment.
Missing zero-divisor check in checked_div_rem_euclid.
This implementation should return None when v is zero, consistent with checked_div_euclid and checked_rem_euclid above (lines 452-456, 460-464). Currently, it will panic on division by zero instead of returning None, defeating the purpose of the "checked" variant.
🐛 Proposed fix
fn checked_div_rem_euclid(&self, v: &Self) -> Option<(Self, Self)> {
+ if v.is_zero() {
+ return None;
+ }
Some(self.div_rem_euclid(v))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn checked_div_rem_euclid(&self, v: &Self) -> Option<(Self, Self)> { | |
| Some(self.div_rem_euclid(v)) | |
| } | |
| fn checked_div_rem_euclid(&self, v: &Self) -> Option<(Self, Self)> { | |
| if v.is_zero() { | |
| return None; | |
| } | |
| Some(self.div_rem_euclid(v)) | |
| } |
🤖 Prompt for AI Agents
In `@vendor/num-bigint/src/bigint/division.rs` around lines 466 - 469, The
checked_div_rem_euclid implementation lacks a zero-divisor check and will panic
when v is zero; update checked_div_rem_euclid to mirror
checked_div_euclid/checked_rem_euclid by returning None if v.is_zero(),
otherwise return Some(self.div_rem_euclid(v)); refer to the
checked_div_rem_euclid and div_rem_euclid functions and reuse the same zero
check pattern as in checked_div_euclid/checked_rem_euclid.
Comment on lines
+286
to
+287
| /// FIPS 186-4 **requires** the private exponent to be less than λ(n), which would | ||
| /// make Euler's totiem unreliable. |
There was a problem hiding this comment.
Typo in documentation comment.
"totiem" should be "totient".
📝 Proposed fix
-/// FIPS 186-4 **requires** the private exponent to be less than λ(n), which would
-/// make Euler's totiem unreliable.
+/// FIPS 186-4 **requires** the private exponent to be less than λ(n), which would
+/// make Euler's totient unreliable.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// FIPS 186-4 **requires** the private exponent to be less than λ(n), which would | |
| /// make Euler's totiem unreliable. | |
| /// FIPS 186-4 **requires** the private exponent to be less than λ(n), which would | |
| /// make Euler's totient unreliable. |
🤖 Prompt for AI Agents
In `@vendor/rsa/src/algorithms/rsa.rs` around lines 286 - 287, Fix the typo in the
documentation comment that reads "totiem" — replace it with "totient" in the
comment starting "FIPS 186-4 **requires** the private exponent to be less than
λ(n), which would make Euler's totiem unreliable." in
vendor/rsa/src/algorithms/rsa.rs so the sentence reads "...which would make
Euler's totient unreliable."
Member
Author
|
/override ? |
|
@omertuc: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Member
Author
|
/override pull-ci-rh-ecosystem-edge-recert-main-e2e-aws-ovn-single-node-recert-parallel |
|
@omertuc: Overrode contexts on behalf of omertuc: ci/prow/e2e-aws-ovn-single-node-recert-parallel DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Member
Author
|
/override pull-ci-rh-ecosystem-edge-recert-main-e2e-aws-ovn-single-node-recert-serial |
|
@omertuc: Overrode contexts on behalf of omertuc: ci/prow/e2e-aws-ovn-single-node-recert-serial DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Member
Author
|
/retest |
4 similar comments
Member
Author
|
/retest |
Member
Author
|
/retest |
Member
Author
|
/retest |
Member
Author
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
vendor/rsa/src/algorithms/rsa.rs (1)
286-287: Typo in documentation comment: "totiem" → "totient".Already flagged in a previous review.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/rsa/src/algorithms/rsa.rs` around lines 286 - 287, Fix the typo in the documentation comment in vendor/rsa/src/algorithms/rsa.rs: change "totiem" to "totient" in the comment that starts "FIPS 186-4 **requires** the private exponent to be less than λ(n), which would make Euler's totiem unreliable." so the comment correctly reads "totient"; update the comment associated with the RSA algorithm implementation (rsa.rs) to reflect the corrected spelling.vendor/rsa/Cargo.toml.orig (1)
65-66: Same[profile.dev]caveat asvendor/rsa/Cargo.tomllines 233-234 — this setting is ignored in dependency manifests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/rsa/Cargo.toml.orig` around lines 65 - 66, The [profile.dev] opt-level = 2 block in this dependency manifest is ignored (profile settings don't apply in dependency Cargo.toml); remove the [profile.dev] / opt-level = 2 stanza from this file (vendor/rsa/Cargo.toml.orig) or relocate the profile override to the workspace or root Cargo.toml so that the change takes effect, ensuring you only edit/remove the [profile.dev] and its opt-level entry.vendor/num-bigint/src/bigint/division.rs (1)
466-469: Missing zero-divisor check inchecked_div_rem_euclid.This delegates directly to
div_rem_euclidwithout checking for zero, so it will panic instead of returningNone. The sibling methodschecked_div_euclid(line 452) andchecked_rem_euclid(line 460) both guard against this.🐛 Proposed fix
fn checked_div_rem_euclid(&self, v: &Self) -> Option<(Self, Self)> { + if v.is_zero() { + return None; + } Some(self.div_rem_euclid(v)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/num-bigint/src/bigint/division.rs` around lines 466 - 469, The method checked_div_rem_euclid currently calls div_rem_euclid unguarded and will panic on a zero divisor; update checked_div_rem_euclid to mirror checked_div_euclid/checked_rem_euclid by checking the divisor first (e.g., test v.is_zero() or equivalent) and return None when the divisor is zero, otherwise call and return div_rem_euclid(v).
🧹 Nitpick comments (7)
vendor/num-traits/src/identities.rs (1)
213-227: Consider extendingwrapping_identitiesto cover the new const paths.The existing test exercises
zero::<Wrapping<T>>()andone::<Wrapping<T>>()but leavesConstZero::ZEROandConstOne::ONEonWrappinguntested.🧪 Suggested test additions
fn test_wrapping_identities { macro_rules! test_wrapping_identities { ($($t:ty)+) => { $( assert_eq!(zero::<$t>(), zero::<Wrapping<$t>>().0); assert_eq!(one::<$t>(), one::<Wrapping<$t>>().0); assert_eq!((0 as $t).is_zero(), Wrapping(0 as $t).is_zero()); assert_eq!((1 as $t).is_zero(), Wrapping(1 as $t).is_zero()); + assert_eq!(Wrapping(<$t as ConstZero>::ZERO), <Wrapping<$t> as ConstZero>::ZERO); + assert_eq!(Wrapping(<$t as ConstOne>::ONE), <Wrapping<$t> as ConstOne>::ONE); )+ }; } test_wrapping_identities!(isize i8 i16 i32 i64 usize u8 u16 u32 u64); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/num-traits/src/identities.rs` around lines 213 - 227, The test wrapping_identities currently checks runtime zero::<T>()/one::<T>() vs Wrapping::<T> values but doesn't exercise the new const paths; update the test to also assert that <T as ConstZero>::ZERO (or ConstZero::ZERO for type T) and <T as ConstOne>::ONE on Wrapping<T> match the existing zero::<T>() and one::<T>() results and that their .is_zero() behavior matches: for each type T in the macro add assertions comparing zero::<T>() == Wrapping::<T>::ZERO.0 (or ConstZero::ZERO.0), one::<T>() == Wrapping::<T>::ONE.0 (or ConstOne::ONE.0), and that Wrapping::<T>::ZERO.is_zero() and Wrapping::<T>::ONE.is_zero() match (0 as T).is_zero()/ (1 as T).is_zero(); keep using the same macro test_wrapping_identities and Wrapping type names to locate where to add these lines.vendor/num-bigint/src/biguint/iter.rs (2)
89-133:nthnot implemented in the 64-bitU32Digitsbranch.The 32-bit branch (lines 41–43) implements
nthin O(1) by delegating toslice::Iter::nth. The 64-bit branch falls back to the defaultIterator::nth, which callsnext()repeatedly — O(n) instead of O(1).A straightforward custom
nthfor the 64-bit branch can skip wholeu64elements at a time:♻️ Suggested `nth` for the 64-bit `U32Digits` branch
#[inline] fn count(self) -> usize { self.len() } + + #[inline] + fn nth(&mut self, n: usize) -> Option<u32> { + // Each u64 holds 2 u32 digits. Account for the current lo/hi position. + let remaining = self.len(); + if n >= remaining { + // Exhaust the iterator. + self.data = &[]; + return None; + } + // Absolute digit index from the start of `data`. + let abs = n + usize::from(!self.next_is_lo); + let word_idx = abs / 2; + let is_lo = abs % 2 == 0; + self.data = &self.data[word_idx..]; + self.next_is_lo = is_lo; + self.next() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/num-bigint/src/biguint/iter.rs` around lines 89 - 133, The 64-bit U32Digits iterator lacks a custom nth implementation so Iterator::nth falls back to repeated next() calls (O(n)); implement Iterator::nth for U32Digits to run O(1) by skipping whole u64 entries when possible: use self.len()/index math to compute how many u64 words to skip and whether to return the low or high half of a word, update self.data and self.next_is_lo accordingly, and handle the last_hi_is_zero edge-case consistent with next() and last() behavior; locate the impl Iterator for U32Digits and add a matching fn nth(&mut self, n: usize) -> Option<u32> that performs these steps.
195-217: Addnthto the 32-bitU64Digitsbranch for O(1) performance.The 64-bit branch (lines 247–249) provides
nthviaslice::Iter::nth. The 32-bit branch falls back to the default, callingnext()n times. Since the inner type isChunks<'a, u32>, a customnthcan skip directly in O(1) time.♻️ Suggested `nth` for the 32-bit `U64Digits` branch
#[inline] fn count(self) -> usize { self.len() } + + #[inline] + fn nth(&mut self, n: usize) -> Option<u64> { + self.it.nth(n).map(super::u32_chunk_to_u64) + }
Chunks::nthcomputes the offset in O(1) using arithmetic and direct slicing—it has been optimized since Rust 1.0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/num-bigint/src/biguint/iter.rs` around lines 195 - 217, The 32-bit U64Digits<'_> iterator currently lacks a custom nth and falls back to O(n) default; add an O(1) nth implementation to the impl Iterator for U64Digits<'_> that mirrors the 64-bit branch by defining fn nth(&mut self, n: usize) -> Option<u64> which calls self.it.nth(n) and maps the result with super::u32_chunk_to_u64 (i.e., return self.it.nth(n).map(super::u32_chunk_to_u64)); this keeps performance consistent with the 64-bit slice::Iter::nth implementation.src/cluster_crypto/crypto_objects.rs (1)
19-25: Consider boxing the large variant(s) rather than silencing the lint.
#[allow(clippy::large_enum_variant)]is a valid approach for this PR's scope. As an optional follow-up, boxing the oversized variant(s) eliminates the wasted memory for every smaller-variant instance and removes the need for the suppression attribute entirely.♻️ Example: boxing the two heavy variants
+#[allow(clippy::large_enum_variant)] pub(crate) enum CryptoObject { - PrivateKey(PrivateKey, PublicKey), - Certificate(Certificate), + PrivateKey(Box<PrivateKey>, Box<PublicKey>), + Certificate(Box<Certificate>), PublicKey(PublicKey), Jwt(jwt::Jwt), }Remove the
#[allow]once the variant sizes are balanced.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cluster_crypto/crypto_objects.rs` around lines 19 - 25, The enum CryptoObject currently silences clippy::large_enum_variant but wastes memory because heavy variants (PrivateKey(PrivateKey, PublicKey), Certificate(Certificate) and possibly Jwt(jwt::Jwt)) make every instance large; change the heavy variants to hold boxed values (e.g., PrivateKey(Box<PrivateKey>, Box<PublicKey> or Box<(PrivateKey, PublicKey)>), Certificate(Box<Certificate>), and Jwt(Box<jwt::Jwt>)) so smaller variants are compact, then remove the #[allow(clippy::large_enum_variant)] attribute; update any match arms and constructors using the CryptoObject::PrivateKey, CryptoObject::Certificate, and CryptoObject::Jwt variants accordingly.vendor/rsa/src/key.rs (1)
815-820:dequality assertion implicitly couples to the specific totient method used to generate the fixture
assert_eq!(key.d(), ref_key.d())is only guaranteed to hold when the reference DER key was generated with the same private-exponent derivation method (from_primesuses Euler totient,from_p_quses Carmichael/SP800-56B). Carmichael's totient produces a smaller but equally validd. If the fixtures are ever regenerated by a tool using a different convention, these assertions will silently fail. A functional correctness check (encrypt/decrypt round-trip) would be more robust and still validate the constructed key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/rsa/src/key.rs` around lines 815 - 820, The test currently asserts private-exponent equality with assert_eq!(key.d(), ref_key.d()), which incorrectly couples the test to the totient convention (from_primes vs from_p_q) and can fail when fixtures are regenerated; replace this direct d() equality check with a functional correctness test: perform an encrypt/decrypt (or sign/verify) round-trip using the constructed key and ref_key (e.g., use key.public_key().encrypt(...) then key.decrypt(...) or sign with key and verify with ref_key.public_key()), and assert the plaintext/message or signature verifies matches, leaving the equality checks for key.n(), dp(), dq() intact; mention from_primes and from_p_q in the change so reviewers know why d() equality was removed.vendor/rsa/Cargo.toml (1)
233-234:[profile.dev]in a dependency manifest has no effectCargo only processes
[profile.*]sections from the workspace root'sCargo.toml; profile sections in dependency/vendored crate manifests are silently ignored. This is upstream configuration and not harmful, butopt-level = 2here has no bearing on how this crate is compiled in the project.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/rsa/Cargo.toml` around lines 233 - 234, The [profile.dev] section with opt-level = 2 in this vendored Cargo.toml has no effect; remove the [profile.dev] block (the "[profile.dev]" header and the "opt-level = 2" entry) from vendor/rsa/Cargo.toml, or if you intended to change build profiles for the whole project, move that setting to the workspace root Cargo.toml under a top-level [profile.dev] instead.vendor/num-bigint/src/biguint/subtraction.rs (1)
170-190: Minor inconsistency:len() == 0vsis_empty().The 32-bit branch (line 173) uses
other.data.len() == 0while the 64-bit branch (line 183) usesother.data.is_empty(). Clippy typically flags the former.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/num-bigint/src/biguint/subtraction.rs` around lines 170 - 190, The two cfg_digit branches for the sub method use inconsistent emptiness checks: one uses other.data.len() == 0 while the other uses other.data.is_empty(); update the branch that uses other.data.len() == 0 to call other.data.is_empty() instead so both cfg_digit variants in fn sub (and the call sites sub2rev/&[self]) use the idiomatic is_empty() check, keeping behavior identical but resolving the clippy inconsistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vendor/num-bigint/src/biguint/division.rs`:
- Around line 681-684: The method checked_div_rem_euclid in BigUint currently
just calls div_rem_euclid which will panic on a zero divisor; update
checked_div_rem_euclid to first check whether the divisor v is zero and return
None if so, otherwise call div_rem_euclid and wrap the result in Some, mirroring
the behavior of the sibling checked_div and checked_div_rem implementations and
avoiding a panic from div_rem.
In `@vendor/rsa/src/key.rs`:
- Around line 281-319: Both constructors can panic when a prime is zero because
compute_private_exponent_* subtracts one from primes before validate() runs;
update from_p_q and from_primes to pre-check primes are > BigUint::one() (and
still enforce p != q in from_p_q and primes.len() >= 2 and pairwise-unequal in
from_primes) and return Err(Error::InvalidPrime) for any prime <= one so
compute_private_exponent_carmicheal/compute_private_exponent_euler_totient are
never called with zero.
---
Duplicate comments:
In `@vendor/num-bigint/src/bigint/division.rs`:
- Around line 466-469: The method checked_div_rem_euclid currently calls
div_rem_euclid unguarded and will panic on a zero divisor; update
checked_div_rem_euclid to mirror checked_div_euclid/checked_rem_euclid by
checking the divisor first (e.g., test v.is_zero() or equivalent) and return
None when the divisor is zero, otherwise call and return div_rem_euclid(v).
In `@vendor/rsa/Cargo.toml.orig`:
- Around line 65-66: The [profile.dev] opt-level = 2 block in this dependency
manifest is ignored (profile settings don't apply in dependency Cargo.toml);
remove the [profile.dev] / opt-level = 2 stanza from this file
(vendor/rsa/Cargo.toml.orig) or relocate the profile override to the workspace
or root Cargo.toml so that the change takes effect, ensuring you only
edit/remove the [profile.dev] and its opt-level entry.
In `@vendor/rsa/src/algorithms/rsa.rs`:
- Around line 286-287: Fix the typo in the documentation comment in
vendor/rsa/src/algorithms/rsa.rs: change "totiem" to "totient" in the comment
that starts "FIPS 186-4 **requires** the private exponent to be less than λ(n),
which would make Euler's totiem unreliable." so the comment correctly reads
"totient"; update the comment associated with the RSA algorithm implementation
(rsa.rs) to reflect the corrected spelling.
---
Nitpick comments:
In `@src/cluster_crypto/crypto_objects.rs`:
- Around line 19-25: The enum CryptoObject currently silences
clippy::large_enum_variant but wastes memory because heavy variants
(PrivateKey(PrivateKey, PublicKey), Certificate(Certificate) and possibly
Jwt(jwt::Jwt)) make every instance large; change the heavy variants to hold
boxed values (e.g., PrivateKey(Box<PrivateKey>, Box<PublicKey> or
Box<(PrivateKey, PublicKey)>), Certificate(Box<Certificate>), and
Jwt(Box<jwt::Jwt>)) so smaller variants are compact, then remove the
#[allow(clippy::large_enum_variant)] attribute; update any match arms and
constructors using the CryptoObject::PrivateKey, CryptoObject::Certificate, and
CryptoObject::Jwt variants accordingly.
In `@vendor/num-bigint/src/biguint/iter.rs`:
- Around line 89-133: The 64-bit U32Digits iterator lacks a custom nth
implementation so Iterator::nth falls back to repeated next() calls (O(n));
implement Iterator::nth for U32Digits to run O(1) by skipping whole u64 entries
when possible: use self.len()/index math to compute how many u64 words to skip
and whether to return the low or high half of a word, update self.data and
self.next_is_lo accordingly, and handle the last_hi_is_zero edge-case consistent
with next() and last() behavior; locate the impl Iterator for U32Digits and add
a matching fn nth(&mut self, n: usize) -> Option<u32> that performs these steps.
- Around line 195-217: The 32-bit U64Digits<'_> iterator currently lacks a
custom nth and falls back to O(n) default; add an O(1) nth implementation to the
impl Iterator for U64Digits<'_> that mirrors the 64-bit branch by defining fn
nth(&mut self, n: usize) -> Option<u64> which calls self.it.nth(n) and maps the
result with super::u32_chunk_to_u64 (i.e., return
self.it.nth(n).map(super::u32_chunk_to_u64)); this keeps performance consistent
with the 64-bit slice::Iter::nth implementation.
In `@vendor/num-bigint/src/biguint/subtraction.rs`:
- Around line 170-190: The two cfg_digit branches for the sub method use
inconsistent emptiness checks: one uses other.data.len() == 0 while the other
uses other.data.is_empty(); update the branch that uses other.data.len() == 0 to
call other.data.is_empty() instead so both cfg_digit variants in fn sub (and the
call sites sub2rev/&[self]) use the idiomatic is_empty() check, keeping behavior
identical but resolving the clippy inconsistency.
In `@vendor/num-traits/src/identities.rs`:
- Around line 213-227: The test wrapping_identities currently checks runtime
zero::<T>()/one::<T>() vs Wrapping::<T> values but doesn't exercise the new
const paths; update the test to also assert that <T as ConstZero>::ZERO (or
ConstZero::ZERO for type T) and <T as ConstOne>::ONE on Wrapping<T> match the
existing zero::<T>() and one::<T>() results and that their .is_zero() behavior
matches: for each type T in the macro add assertions comparing zero::<T>() ==
Wrapping::<T>::ZERO.0 (or ConstZero::ZERO.0), one::<T>() == Wrapping::<T>::ONE.0
(or ConstOne::ONE.0), and that Wrapping::<T>::ZERO.is_zero() and
Wrapping::<T>::ONE.is_zero() match (0 as T).is_zero()/ (1 as T).is_zero(); keep
using the same macro test_wrapping_identities and Wrapping type names to locate
where to add these lines.
In `@vendor/rsa/Cargo.toml`:
- Around line 233-234: The [profile.dev] section with opt-level = 2 in this
vendored Cargo.toml has no effect; remove the [profile.dev] block (the
"[profile.dev]" header and the "opt-level = 2" entry) from
vendor/rsa/Cargo.toml, or if you intended to change build profiles for the whole
project, move that setting to the workspace root Cargo.toml under a top-level
[profile.dev] instead.
In `@vendor/rsa/src/key.rs`:
- Around line 815-820: The test currently asserts private-exponent equality with
assert_eq!(key.d(), ref_key.d()), which incorrectly couples the test to the
totient convention (from_primes vs from_p_q) and can fail when fixtures are
regenerated; replace this direct d() equality check with a functional
correctness test: perform an encrypt/decrypt (or sign/verify) round-trip using
the constructed key and ref_key (e.g., use key.public_key().encrypt(...) then
key.decrypt(...) or sign with key and verify with ref_key.public_key()), and
assert the plaintext/message or signature verifies matches, leaving the equality
checks for key.n(), dp(), dq() intact; mention from_primes and from_p_q in the
change so reviewers know why d() equality was removed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockvendor/num-bigint-dig/Cargo.lockis excluded by!**/*.lockvendor/rsa/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (131)
Cargo.tomlbuild.rssrc/cluster_crypto.rssrc/cluster_crypto/crypto_objects.rssrc/cluster_crypto/json_crawl.rssrc/ocp_postprocess/ip_rename/etcd_rename.rssrc/protobuf_gen/github.com.openshift.api.oauth.v1.rssrc/protobuf_gen/github.com.openshift.api.route.v1.rssrc/protobuf_gen/k8s.io.api.admissionregistration.v1.rssrc/protobuf_gen/k8s.io.api.apps.v1.rssrc/protobuf_gen/k8s.io.api.batch.v1.rssrc/protobuf_gen/k8s.io.api.core.v1.rssrc/protobuf_gen/k8s.io.apimachinery.pkg.api.resource.rssrc/protobuf_gen/k8s.io.apimachinery.pkg.apis.meta.v1.rssrc/protobuf_gen/k8s.io.apimachinery.pkg.runtime.rssrc/protobuf_gen/k8s.io.apimachinery.pkg.util.intstr.rsvendor/byteorder/.cargo-checksum.jsonvendor/byteorder/.cargo_vcs_info.jsonvendor/byteorder/.github/workflows/ci.ymlvendor/byteorder/CHANGELOG.mdvendor/byteorder/COPYINGvendor/byteorder/Cargo.tomlvendor/byteorder/Cargo.toml.origvendor/byteorder/LICENSE-MITvendor/byteorder/README.mdvendor/byteorder/UNLICENSEvendor/byteorder/benches/bench.rsvendor/byteorder/rustfmt.tomlvendor/byteorder/src/io.rsvendor/byteorder/src/lib.rsvendor/num-bigint-dig/.cargo-checksum.jsonvendor/num-bigint-dig/.cargo_vcs_info.jsonvendor/num-bigint-dig/Cargo.tomlvendor/num-bigint-dig/Cargo.toml.origvendor/num-bigint-dig/src/algorithms/mod_inverse.rsvendor/num-bigint-dig/src/bigrand.rsvendor/num-bigint-dig/src/biguint.rsvendor/num-bigint-dig/src/prime.rsvendor/num-bigint-dig/tests/torture.rsvendor/num-bigint/.cargo-checksum.jsonvendor/num-bigint/.cargo_vcs_info.jsonvendor/num-bigint/Cargo.tomlvendor/num-bigint/Cargo.toml.origvendor/num-bigint/README.mdvendor/num-bigint/RELEASES.mdvendor/num-bigint/benches/bigint.rsvendor/num-bigint/build.rsvendor/num-bigint/src/bigint.rsvendor/num-bigint/src/bigint/addition.rsvendor/num-bigint/src/bigint/arbitrary.rsvendor/num-bigint/src/bigint/bits.rsvendor/num-bigint/src/bigint/convert.rsvendor/num-bigint/src/bigint/division.rsvendor/num-bigint/src/bigint/power.rsvendor/num-bigint/src/bigint/serde.rsvendor/num-bigint/src/bigint/subtraction.rsvendor/num-bigint/src/bigrand.rsvendor/num-bigint/src/biguint.rsvendor/num-bigint/src/biguint/addition.rsvendor/num-bigint/src/biguint/arbitrary.rsvendor/num-bigint/src/biguint/convert.rsvendor/num-bigint/src/biguint/division.rsvendor/num-bigint/src/biguint/iter.rsvendor/num-bigint/src/biguint/monty.rsvendor/num-bigint/src/biguint/multiplication.rsvendor/num-bigint/src/biguint/power.rsvendor/num-bigint/src/biguint/serde.rsvendor/num-bigint/src/biguint/shift.rsvendor/num-bigint/src/biguint/subtraction.rsvendor/num-bigint/src/lib.rsvendor/num-bigint/src/macros.rsvendor/num-bigint/tests/bigint.rsvendor/num-bigint/tests/bigint_bitwise.rsvendor/num-bigint/tests/biguint.rsvendor/num-bigint/tests/consts/mod.rsvendor/num-bigint/tests/roots.rsvendor/num-integer/.cargo-checksum.jsonvendor/num-integer/.cargo_vcs_info.jsonvendor/num-integer/Cargo.tomlvendor/num-integer/Cargo.toml.origvendor/num-integer/README.mdvendor/num-integer/RELEASES.mdvendor/num-integer/benches/average.rsvendor/num-integer/benches/gcd.rsvendor/num-integer/benches/roots.rsvendor/num-integer/build.rsvendor/num-integer/src/average.rsvendor/num-integer/src/lib.rsvendor/num-integer/src/roots.rsvendor/num-integer/tests/average.rsvendor/num-integer/tests/roots.rsvendor/num-traits/.cargo-checksum.jsonvendor/num-traits/.cargo_vcs_info.jsonvendor/num-traits/Cargo.tomlvendor/num-traits/Cargo.toml.origvendor/num-traits/README.mdvendor/num-traits/RELEASES.mdvendor/num-traits/build.rsvendor/num-traits/src/cast.rsvendor/num-traits/src/float.rsvendor/num-traits/src/identities.rsvendor/num-traits/src/int.rsvendor/num-traits/src/lib.rsvendor/num-traits/src/ops/bytes.rsvendor/num-traits/src/ops/euclid.rsvendor/num-traits/src/real.rsvendor/rsa/.cargo-checksum.jsonvendor/rsa/.cargo_vcs_info.jsonvendor/rsa/CHANGELOG.mdvendor/rsa/Cargo.tomlvendor/rsa/Cargo.toml.origvendor/rsa/README.mdvendor/rsa/src/algorithms/generate.rsvendor/rsa/src/algorithms/pkcs1v15.rsvendor/rsa/src/algorithms/rsa.rsvendor/rsa/src/errors.rsvendor/rsa/src/key.rsvendor/rsa/src/lib.rsvendor/rsa/src/oaep.rsvendor/rsa/src/pkcs1v15.rsvendor/rsa/src/pkcs1v15/signature.rsvendor/rsa/src/pss.rsvendor/rsa/src/pss/signature.rsvendor/rsa/tests/examples/pkcs8/rsa2048-sp800-56b-priv.dervendor/spki/.cargo-checksum.jsonvendor/spki/.cargo_vcs_info.jsonvendor/spki/CHANGELOG.mdvendor/spki/Cargo.tomlvendor/spki/Cargo.toml.origvendor/spki/src/spki.rsvendor/spki/tests/spki.rs
💤 Files with no reviewable changes (23)
- vendor/num-bigint/tests/bigint_bitwise.rs
- vendor/num-bigint/tests/biguint.rs
- vendor/num-traits/src/int.rs
- vendor/byteorder/Cargo.toml.orig
- vendor/byteorder/UNLICENSE
- vendor/num-traits/src/cast.rs
- vendor/byteorder/LICENSE-MIT
- vendor/byteorder/.cargo-checksum.json
- vendor/byteorder/README.md
- vendor/num-bigint/build.rs
- vendor/num-bigint/tests/roots.rs
- vendor/byteorder/Cargo.toml
- vendor/byteorder/COPYING
- vendor/byteorder/CHANGELOG.md
- vendor/num-traits/src/ops/bytes.rs
- vendor/num-integer/benches/gcd.rs
- vendor/num-integer/build.rs
- vendor/byteorder/src/io.rs
- vendor/num-bigint/tests/bigint.rs
- vendor/byteorder/rustfmt.toml
- vendor/byteorder/.github/workflows/ci.yml
- vendor/byteorder/benches/bench.rs
- vendor/byteorder/.cargo_vcs_info.json
✅ Files skipped from review due to trivial changes (1)
- vendor/num-bigint/.cargo_vcs_info.json
🚧 Files skipped from review as they are similar to previous changes (37)
- vendor/num-traits/src/real.rs
- vendor/rsa/src/oaep.rs
- vendor/num-bigint/src/bigint/arbitrary.rs
- vendor/num-bigint-dig/.cargo-checksum.json
- vendor/num-bigint/src/bigint/addition.rs
- vendor/spki/.cargo_vcs_info.json
- vendor/num-bigint-dig/.cargo_vcs_info.json
- vendor/num-bigint/src/bigint/power.rs
- vendor/rsa/src/pkcs1v15.rs
- vendor/num-traits/src/lib.rs
- src/protobuf_gen/github.com.openshift.api.oauth.v1.rs
- src/protobuf_gen/k8s.io.apimachinery.pkg.api.resource.rs
- src/cluster_crypto/json_crawl.rs
- vendor/num-traits/.cargo-checksum.json
- src/ocp_postprocess/ip_rename/etcd_rename.rs
- vendor/num-integer/tests/average.rs
- vendor/num-bigint-dig/src/algorithms/mod_inverse.rs
- vendor/num-integer/Cargo.toml
- vendor/num-bigint-dig/src/prime.rs
- vendor/spki/tests/spki.rs
- vendor/num-bigint-dig/src/bigrand.rs
- vendor/num-integer/.cargo-checksum.json
- vendor/num-traits/README.md
- vendor/num-bigint/src/bigint/subtraction.rs
- vendor/rsa/.cargo_vcs_info.json
- vendor/num-bigint/benches/bigint.rs
- src/protobuf_gen/k8s.io.api.batch.v1.rs
- vendor/num-bigint/src/bigint/bits.rs
- vendor/num-traits/src/ops/euclid.rs
- vendor/num-bigint/src/bigint/serde.rs
- vendor/rsa/src/algorithms/pkcs1v15.rs
- vendor/rsa/.cargo-checksum.json
- vendor/num-traits/.cargo_vcs_info.json
- vendor/num-bigint/README.md
- vendor/num-bigint/src/macros.rs
- vendor/num-integer/.cargo_vcs_info.json
- vendor/num-bigint-dig/Cargo.toml
Comment on lines
+681
to
+684
|
|
||
| fn checked_div_rem_euclid(&self, v: &Self) -> Option<(Self, Self)> { | ||
| Some(self.div_rem_euclid(v)) | ||
| } |
There was a problem hiding this comment.
Missing zero-divisor check in checked_div_rem_euclid for BigUint.
Same issue as the BigInt variant: delegates to div_rem_euclid (which calls div_rem → panics on zero) without guarding for a zero divisor. The sibling methods at lines 667-680 both return None for zero.
🐛 Proposed fix
fn checked_div_rem_euclid(&self, v: &Self) -> Option<(Self, Self)> {
+ if v.is_zero() {
+ return None;
+ }
Some(self.div_rem_euclid(v))
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/num-bigint/src/biguint/division.rs` around lines 681 - 684, The method
checked_div_rem_euclid in BigUint currently just calls div_rem_euclid which will
panic on a zero divisor; update checked_div_rem_euclid to first check whether
the divisor v is zero and return None if so, otherwise call div_rem_euclid and
wrap the result in Some, mirroring the behavior of the sibling checked_div and
checked_div_rem implementations and avoiding a panic from div_rem.
Comment on lines
+281
to
+319
| /// Constructs an RSA key pair from its two primes p and q. | ||
| /// | ||
| /// This will rebuild the private exponent and the modulus. | ||
| /// | ||
| /// Private exponent will be rebuilt using the method defined in | ||
| /// [NIST 800-56B Section 6.2.1](https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-56Br2.pdf#page=47). | ||
| pub fn from_p_q(p: BigUint, q: BigUint, public_exponent: BigUint) -> Result<RsaPrivateKey> { | ||
| if p == q { | ||
| return Err(Error::InvalidPrime); | ||
| } | ||
|
|
||
| let n = compute_modulus(&[p.clone(), q.clone()]); | ||
| let d = compute_private_exponent_carmicheal(&p, &q, &public_exponent)?; | ||
|
|
||
| Self::from_components(n, public_exponent, d, vec![p, q]) | ||
| } | ||
|
|
||
| /// Constructs an RSA key pair from its primes. | ||
| /// | ||
| /// This will rebuild the private exponent and the modulus. | ||
| pub fn from_primes(primes: Vec<BigUint>, public_exponent: BigUint) -> Result<RsaPrivateKey> { | ||
| if primes.len() < 2 { | ||
| return Err(Error::NprimesTooSmall); | ||
| } | ||
|
|
||
| // Makes sure that primes is pairwise unequal. | ||
| for (i, prime1) in primes.iter().enumerate() { | ||
| for prime2 in primes.iter().take(i) { | ||
| if prime1 == prime2 { | ||
| return Err(Error::InvalidPrime); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let n = compute_modulus(&primes); | ||
| let d = compute_private_exponent_euler_totient(&primes, &public_exponent)?; | ||
|
|
||
| Self::from_components(n, public_exponent, d, primes) | ||
| } |
There was a problem hiding this comment.
from_p_q and from_primes can panic on zero primes before validate() is reached
Both new constructors call into compute_private_exponent_carmicheal/compute_private_exponent_euler_totient, which compute p - BigUint::one() without a zero-guard. For BigUint, subtracting below zero panics. The validate() call inside from_components handles this gracefully (*prime <= BigUint::one() → Err(InvalidPrime)), but it fires only after the compute functions — so passing p = 0 or any zero prime panics rather than returning an error, inconsistent with the rest of the API.
🛡️ Suggested guard in from_p_q
pub fn from_p_q(p: BigUint, q: BigUint, public_exponent: BigUint) -> Result<RsaPrivateKey> {
if p == q {
return Err(Error::InvalidPrime);
}
+ if p <= BigUint::one() || q <= BigUint::one() {
+ return Err(Error::InvalidPrime);
+ }🛡️ Suggested guard in from_primes
for (i, prime1) in primes.iter().enumerate() {
for prime2 in primes.iter().take(i) {
if prime1 == prime2 {
return Err(Error::InvalidPrime);
}
}
}
+ for prime in &primes {
+ if *prime <= BigUint::one() {
+ return Err(Error::InvalidPrime);
+ }
+ }Note: this is code from the upstream rsa 0.9.10 crate. If preferred, the fix can be tracked as an upstream issue rather than a local patch.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Constructs an RSA key pair from its two primes p and q. | |
| /// | |
| /// This will rebuild the private exponent and the modulus. | |
| /// | |
| /// Private exponent will be rebuilt using the method defined in | |
| /// [NIST 800-56B Section 6.2.1](https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-56Br2.pdf#page=47). | |
| pub fn from_p_q(p: BigUint, q: BigUint, public_exponent: BigUint) -> Result<RsaPrivateKey> { | |
| if p == q { | |
| return Err(Error::InvalidPrime); | |
| } | |
| let n = compute_modulus(&[p.clone(), q.clone()]); | |
| let d = compute_private_exponent_carmicheal(&p, &q, &public_exponent)?; | |
| Self::from_components(n, public_exponent, d, vec![p, q]) | |
| } | |
| /// Constructs an RSA key pair from its primes. | |
| /// | |
| /// This will rebuild the private exponent and the modulus. | |
| pub fn from_primes(primes: Vec<BigUint>, public_exponent: BigUint) -> Result<RsaPrivateKey> { | |
| if primes.len() < 2 { | |
| return Err(Error::NprimesTooSmall); | |
| } | |
| // Makes sure that primes is pairwise unequal. | |
| for (i, prime1) in primes.iter().enumerate() { | |
| for prime2 in primes.iter().take(i) { | |
| if prime1 == prime2 { | |
| return Err(Error::InvalidPrime); | |
| } | |
| } | |
| } | |
| let n = compute_modulus(&primes); | |
| let d = compute_private_exponent_euler_totient(&primes, &public_exponent)?; | |
| Self::from_components(n, public_exponent, d, primes) | |
| } | |
| /// Constructs an RSA key pair from its two primes p and q. | |
| /// | |
| /// This will rebuild the private exponent and the modulus. | |
| /// | |
| /// Private exponent will be rebuilt using the method defined in | |
| /// [NIST 800-56B Section 6.2.1](https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-56Br2.pdf#page=47). | |
| pub fn from_p_q(p: BigUint, q: BigUint, public_exponent: BigUint) -> Result<RsaPrivateKey> { | |
| if p == q { | |
| return Err(Error::InvalidPrime); | |
| } | |
| if p <= BigUint::one() || q <= BigUint::one() { | |
| return Err(Error::InvalidPrime); | |
| } | |
| let n = compute_modulus(&[p.clone(), q.clone()]); | |
| let d = compute_private_exponent_carmicheal(&p, &q, &public_exponent)?; | |
| Self::from_components(n, public_exponent, d, vec![p, q]) | |
| } | |
| /// Constructs an RSA key pair from its primes. | |
| /// | |
| /// This will rebuild the private exponent and the modulus. | |
| pub fn from_primes(primes: Vec<BigUint>, public_exponent: BigUint) -> Result<RsaPrivateKey> { | |
| if primes.len() < 2 { | |
| return Err(Error::NprimesTooSmall); | |
| } | |
| // Makes sure that primes is pairwise unequal. | |
| for (i, prime1) in primes.iter().enumerate() { | |
| for prime2 in primes.iter().take(i) { | |
| if prime1 == prime2 { | |
| return Err(Error::InvalidPrime); | |
| } | |
| } | |
| } | |
| for prime in &primes { | |
| if *prime <= BigUint::one() { | |
| return Err(Error::InvalidPrime); | |
| } | |
| } | |
| let n = compute_modulus(&primes); | |
| let d = compute_private_exponent_euler_totient(&primes, &public_exponent)?; | |
| Self::from_components(n, public_exponent, d, primes) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/rsa/src/key.rs` around lines 281 - 319, Both constructors can panic
when a prime is zero because compute_private_exponent_* subtracts one from
primes before validate() runs; update from_p_q and from_primes to pre-check
primes are > BigUint::one() (and still enforce p != q in from_p_q and
primes.len() >= 2 and pairwise-unequal in from_primes) and return
Err(Error::InvalidPrime) for any prime <= one so
compute_private_exponent_carmicheal/compute_private_exponent_euler_totient are
never called with zero.
Member
Author
|
/retest |
2 similar comments
Member
Author
|
/retest |
Member
Author
|
/retest |
Our prost generated code was generating unused warnings because we're
not using all of the generated structs. This change adds an attribute:
`prost_build.type_attribute(".", "#[allow(dead_code)]");`
to each struct in order to suppress those warnings.
This will fix the following warning: ``` warning: the following packages contain code that will be rejected by a future version of Rust: num-bigint-dig v0.8.4 ``` We have `num-bigint-dig` as an indirect dependency from the `rsa` crate, upgrading the `rsa` crate to the latest version gets rid of this warning.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
vendor/rsa/src/key.rs (1)
281-318:⚠️ Potential issue | 🔴 CriticalReject
<= 1primes before rebuildingd.
from_p_qandfrom_primesstill call the exponent-rebuild helpers beforefrom_componentscan reject bad primes. Those helpers dop - 1/prime - 1, andnum-bigint-dig0.8.6 asserts on subtraction underflow, so a zero prime can still panic here instead of returningError::InvalidPrime. Please short-circuit<= 1in both constructors. (docs.rs)🛡️ Proposed guard in both constructors
pub fn from_p_q(p: BigUint, q: BigUint, public_exponent: BigUint) -> Result<RsaPrivateKey> { - if p == q { + if p <= BigUint::one() || q <= BigUint::one() || p == q { return Err(Error::InvalidPrime); } let n = compute_modulus(&[p.clone(), q.clone()]); let d = compute_private_exponent_carmicheal(&p, &q, &public_exponent)?; @@ pub fn from_primes(primes: Vec<BigUint>, public_exponent: BigUint) -> Result<RsaPrivateKey> { if primes.len() < 2 { return Err(Error::NprimesTooSmall); } + if primes.iter().any(|prime| prime <= &BigUint::one()) { + return Err(Error::InvalidPrime); + } // Makes sure that primes is pairwise unequal. for (i, prime1) in primes.iter().enumerate() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/rsa/src/key.rs` around lines 281 - 318, Both constructors must validate that primes are greater than 1 before attempting to rebuild the private exponent to avoid subtraction underflow in compute_private_exponent_carmichael/compute_private_exponent_euler_totient; in from_p_q add a guard after the p==q check to return Error::InvalidPrime if p <= 1 or q <= 1, and in from_primes short-circuit by returning Error::InvalidPrime if any prime in primes is <= 1 (in addition to the existing len < 2 check and pairwise-unequal checks) before calling compute_private_exponent_* and Self::from_components.vendor/num-bigint/src/bigint/division.rs (1)
466-469:⚠️ Potential issue | 🔴 CriticalMissing zero-divisor check in
checked_div_rem_euclid.This implementation should return
Nonewhenvis zero, consistent withchecked_div_euclidandchecked_rem_euclidabove (lines 452-456, 460-464). Currently, it will panic on division by zero instead of returningNone, defeating the purpose of the "checked" variant.,
🐛 Proposed fix
fn checked_div_rem_euclid(&self, v: &Self) -> Option<(Self, Self)> { + if v.is_zero() { + return None; + } Some(self.div_rem_euclid(v)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vendor/num-bigint/src/bigint/division.rs` around lines 466 - 469, The checked_div_rem_euclid implementation must mirror the other checked variants by returning None for a zero divisor; update checked_div_rem_euclid to first test whether v is zero (same zero-check approach used in checked_div_euclid/checked_rem_euclid) and return None if so, otherwise call div_rem_euclid(v) and wrap the result in Some — reference the checked_div_rem_euclid function and the div_rem_euclid helper when making the change.
🧹 Nitpick comments (1)
src/protobuf_gen/k8s.io.api.admissionregistration.v1.rs (1)
2-2: Acceptable for generated code, but consider generator-level fix.Adding
#[allow(dead_code)]to generated protobuf structs is a pragmatic way to suppress warnings. However, if these structs are genuinely unused, consider whether the protobuf generation configuration could be adjusted to only generate needed types, or apply the attribute in the code generator itself for maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/protobuf_gen/k8s.io.api.admissionregistration.v1.rs` at line 2, The generated protobuf file adds #[allow(dead_code)] to suppress warnings; instead of patching individual generated files, update the protobuf code generator or its template so the attribute is emitted for the entire module or target structs (apply #[allow(dead_code)] at the module level or in the generator template) or adjust generation options to only emit the needed types; locate the usage of the attribute in the generated module name k8s.io.api.admissionregistration.v1 and modify the generator/template that produces that file to emit the attribute (or narrow generation) so you don't hand-edit generated code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vendor/num-traits/src/float.rs`:
- Line 824: The concrete impls currently forward to Self::clamp (remove the
explicit Self::clamp overrides) which causes f32/f64 primitive impls to use a
release-time panic path; delete the explicit clamp forwards in the impl
FloatCore for f32 and impl FloatCore for f64 and remove the clamp forwarding
lines in the float_impl_std! and float_impl_libm! macro expansions so those
types fall back to the trait default clamp implementation (which uses
crate::clamp with debug-only assertions); ensure no other explicit Self::clamp
overrides remain so the trait default behavior is consistent across generic and
concrete uses.
---
Duplicate comments:
In `@vendor/num-bigint/src/bigint/division.rs`:
- Around line 466-469: The checked_div_rem_euclid implementation must mirror the
other checked variants by returning None for a zero divisor; update
checked_div_rem_euclid to first test whether v is zero (same zero-check approach
used in checked_div_euclid/checked_rem_euclid) and return None if so, otherwise
call div_rem_euclid(v) and wrap the result in Some — reference the
checked_div_rem_euclid function and the div_rem_euclid helper when making the
change.
In `@vendor/rsa/src/key.rs`:
- Around line 281-318: Both constructors must validate that primes are greater
than 1 before attempting to rebuild the private exponent to avoid subtraction
underflow in
compute_private_exponent_carmichael/compute_private_exponent_euler_totient; in
from_p_q add a guard after the p==q check to return Error::InvalidPrime if p <=
1 or q <= 1, and in from_primes short-circuit by returning Error::InvalidPrime
if any prime in primes is <= 1 (in addition to the existing len < 2 check and
pairwise-unequal checks) before calling compute_private_exponent_* and
Self::from_components.
---
Nitpick comments:
In `@src/protobuf_gen/k8s.io.api.admissionregistration.v1.rs`:
- Line 2: The generated protobuf file adds #[allow(dead_code)] to suppress
warnings; instead of patching individual generated files, update the protobuf
code generator or its template so the attribute is emitted for the entire module
or target structs (apply #[allow(dead_code)] at the module level or in the
generator template) or adjust generation options to only emit the needed types;
locate the usage of the attribute in the generated module name
k8s.io.api.admissionregistration.v1 and modify the generator/template that
produces that file to emit the attribute (or narrow generation) so you don't
hand-edit generated code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 67872382-47f4-4324-ba85-cd594d2e13b0
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockvendor/num-bigint-dig/Cargo.lockis excluded by!**/*.lockvendor/rsa/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (131)
Cargo.tomlbuild.rssrc/cluster_crypto.rssrc/cluster_crypto/crypto_objects.rssrc/cluster_crypto/json_crawl.rssrc/ocp_postprocess/ip_rename/etcd_rename.rssrc/protobuf_gen/github.com.openshift.api.oauth.v1.rssrc/protobuf_gen/github.com.openshift.api.route.v1.rssrc/protobuf_gen/k8s.io.api.admissionregistration.v1.rssrc/protobuf_gen/k8s.io.api.apps.v1.rssrc/protobuf_gen/k8s.io.api.batch.v1.rssrc/protobuf_gen/k8s.io.api.core.v1.rssrc/protobuf_gen/k8s.io.apimachinery.pkg.api.resource.rssrc/protobuf_gen/k8s.io.apimachinery.pkg.apis.meta.v1.rssrc/protobuf_gen/k8s.io.apimachinery.pkg.runtime.rssrc/protobuf_gen/k8s.io.apimachinery.pkg.util.intstr.rsvendor/byteorder/.cargo-checksum.jsonvendor/byteorder/.cargo_vcs_info.jsonvendor/byteorder/.github/workflows/ci.ymlvendor/byteorder/CHANGELOG.mdvendor/byteorder/COPYINGvendor/byteorder/Cargo.tomlvendor/byteorder/Cargo.toml.origvendor/byteorder/LICENSE-MITvendor/byteorder/README.mdvendor/byteorder/UNLICENSEvendor/byteorder/benches/bench.rsvendor/byteorder/rustfmt.tomlvendor/byteorder/src/io.rsvendor/byteorder/src/lib.rsvendor/num-bigint-dig/.cargo-checksum.jsonvendor/num-bigint-dig/.cargo_vcs_info.jsonvendor/num-bigint-dig/Cargo.tomlvendor/num-bigint-dig/Cargo.toml.origvendor/num-bigint-dig/src/algorithms/mod_inverse.rsvendor/num-bigint-dig/src/bigrand.rsvendor/num-bigint-dig/src/biguint.rsvendor/num-bigint-dig/src/prime.rsvendor/num-bigint-dig/tests/torture.rsvendor/num-bigint/.cargo-checksum.jsonvendor/num-bigint/.cargo_vcs_info.jsonvendor/num-bigint/Cargo.tomlvendor/num-bigint/Cargo.toml.origvendor/num-bigint/README.mdvendor/num-bigint/RELEASES.mdvendor/num-bigint/benches/bigint.rsvendor/num-bigint/build.rsvendor/num-bigint/src/bigint.rsvendor/num-bigint/src/bigint/addition.rsvendor/num-bigint/src/bigint/arbitrary.rsvendor/num-bigint/src/bigint/bits.rsvendor/num-bigint/src/bigint/convert.rsvendor/num-bigint/src/bigint/division.rsvendor/num-bigint/src/bigint/power.rsvendor/num-bigint/src/bigint/serde.rsvendor/num-bigint/src/bigint/subtraction.rsvendor/num-bigint/src/bigrand.rsvendor/num-bigint/src/biguint.rsvendor/num-bigint/src/biguint/addition.rsvendor/num-bigint/src/biguint/arbitrary.rsvendor/num-bigint/src/biguint/convert.rsvendor/num-bigint/src/biguint/division.rsvendor/num-bigint/src/biguint/iter.rsvendor/num-bigint/src/biguint/monty.rsvendor/num-bigint/src/biguint/multiplication.rsvendor/num-bigint/src/biguint/power.rsvendor/num-bigint/src/biguint/serde.rsvendor/num-bigint/src/biguint/shift.rsvendor/num-bigint/src/biguint/subtraction.rsvendor/num-bigint/src/lib.rsvendor/num-bigint/src/macros.rsvendor/num-bigint/tests/bigint.rsvendor/num-bigint/tests/bigint_bitwise.rsvendor/num-bigint/tests/biguint.rsvendor/num-bigint/tests/consts/mod.rsvendor/num-bigint/tests/roots.rsvendor/num-integer/.cargo-checksum.jsonvendor/num-integer/.cargo_vcs_info.jsonvendor/num-integer/Cargo.tomlvendor/num-integer/Cargo.toml.origvendor/num-integer/README.mdvendor/num-integer/RELEASES.mdvendor/num-integer/benches/average.rsvendor/num-integer/benches/gcd.rsvendor/num-integer/benches/roots.rsvendor/num-integer/build.rsvendor/num-integer/src/average.rsvendor/num-integer/src/lib.rsvendor/num-integer/src/roots.rsvendor/num-integer/tests/average.rsvendor/num-integer/tests/roots.rsvendor/num-traits/.cargo-checksum.jsonvendor/num-traits/.cargo_vcs_info.jsonvendor/num-traits/Cargo.tomlvendor/num-traits/Cargo.toml.origvendor/num-traits/README.mdvendor/num-traits/RELEASES.mdvendor/num-traits/build.rsvendor/num-traits/src/cast.rsvendor/num-traits/src/float.rsvendor/num-traits/src/identities.rsvendor/num-traits/src/int.rsvendor/num-traits/src/lib.rsvendor/num-traits/src/ops/bytes.rsvendor/num-traits/src/ops/euclid.rsvendor/num-traits/src/real.rsvendor/rsa/.cargo-checksum.jsonvendor/rsa/.cargo_vcs_info.jsonvendor/rsa/CHANGELOG.mdvendor/rsa/Cargo.tomlvendor/rsa/Cargo.toml.origvendor/rsa/README.mdvendor/rsa/src/algorithms/generate.rsvendor/rsa/src/algorithms/pkcs1v15.rsvendor/rsa/src/algorithms/rsa.rsvendor/rsa/src/errors.rsvendor/rsa/src/key.rsvendor/rsa/src/lib.rsvendor/rsa/src/oaep.rsvendor/rsa/src/pkcs1v15.rsvendor/rsa/src/pkcs1v15/signature.rsvendor/rsa/src/pss.rsvendor/rsa/src/pss/signature.rsvendor/rsa/tests/examples/pkcs8/rsa2048-sp800-56b-priv.dervendor/spki/.cargo-checksum.jsonvendor/spki/.cargo_vcs_info.jsonvendor/spki/CHANGELOG.mdvendor/spki/Cargo.tomlvendor/spki/Cargo.toml.origvendor/spki/src/spki.rsvendor/spki/tests/spki.rs
💤 Files with no reviewable changes (23)
- vendor/num-traits/src/int.rs
- vendor/byteorder/CHANGELOG.md
- vendor/num-traits/src/ops/bytes.rs
- vendor/byteorder/.github/workflows/ci.yml
- vendor/byteorder/.cargo_vcs_info.json
- vendor/num-bigint/tests/roots.rs
- vendor/byteorder/rustfmt.toml
- vendor/byteorder/benches/bench.rs
- vendor/byteorder/COPYING
- vendor/num-bigint/build.rs
- vendor/num-bigint/tests/bigint_bitwise.rs
- vendor/num-integer/build.rs
- vendor/num-bigint/tests/bigint.rs
- vendor/num-bigint/tests/biguint.rs
- vendor/byteorder/.cargo-checksum.json
- vendor/num-integer/benches/gcd.rs
- vendor/byteorder/src/io.rs
- vendor/byteorder/UNLICENSE
- vendor/byteorder/LICENSE-MIT
- vendor/num-traits/src/cast.rs
- vendor/byteorder/README.md
- vendor/byteorder/Cargo.toml
- vendor/byteorder/Cargo.toml.orig
✅ Files skipped from review due to trivial changes (3)
- vendor/num-bigint/.cargo-checksum.json
- vendor/num-bigint-dig/src/prime.rs
- vendor/num-traits/.cargo-checksum.json
🚧 Files skipped from review as they are similar to previous changes (38)
- vendor/num-integer/benches/roots.rs
- src/cluster_crypto/json_crawl.rs
- src/cluster_crypto/crypto_objects.rs
- vendor/num-traits/src/real.rs
- vendor/num-bigint-dig/.cargo_vcs_info.json
- vendor/num-bigint-dig/src/bigrand.rs
- vendor/num-integer/benches/average.rs
- src/protobuf_gen/k8s.io.apimachinery.pkg.api.resource.rs
- vendor/rsa/CHANGELOG.md
- vendor/rsa/src/oaep.rs
- vendor/num-bigint-dig/tests/torture.rs
- vendor/rsa/src/algorithms/pkcs1v15.rs
- vendor/num-integer/src/average.rs
- vendor/num-integer/.cargo_vcs_info.json
- vendor/num-bigint-dig/src/biguint.rs
- vendor/rsa/src/pkcs1v15.rs
- vendor/num-bigint/src/macros.rs
- vendor/num-traits/src/identities.rs
- vendor/rsa/.cargo-checksum.json
- vendor/spki/.cargo-checksum.json
- vendor/num-bigint/src/biguint/power.rs
- vendor/num-integer/tests/roots.rs
- vendor/num-bigint/src/bigint/serde.rs
- vendor/num-bigint/src/biguint/arbitrary.rs
- vendor/rsa/src/pkcs1v15/signature.rs
- build.rs
- vendor/num-bigint/Cargo.toml
- vendor/num-traits/.cargo_vcs_info.json
- vendor/num-bigint-dig/src/algorithms/mod_inverse.rs
- src/protobuf_gen/k8s.io.apimachinery.pkg.util.intstr.rs
- vendor/num-integer/Cargo.toml
- vendor/spki/Cargo.toml.orig
- vendor/rsa/src/algorithms/rsa.rs
- vendor/rsa/src/pss.rs
- vendor/num-bigint/src/bigint/power.rs
- vendor/spki/Cargo.toml
- vendor/spki/tests/spki.rs
- vendor/num-bigint/tests/consts/mod.rs
| Self::is_finite(self) -> bool; | ||
| Self::is_normal(self) -> bool; | ||
| Self::is_subnormal(self) -> bool; | ||
| Self::clamp(self, min: Self, max: Self) -> Self; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In stable Rust, what are the exact panic conditions for f32::clamp/f64::clamp(includingmin > max and NaN bounds), and do those panics occur in release builds?
💡 Result:
Exact panic conditions (stable Rust)
For both f32::clamp and f64::clamp, the function panics iff:
min > max, orminis NaN, ormaxis NaN.
(Because the implementation checksmin <= max; with IEEE float comparisons, if either bound is NaN thenmin <= maxis false, triggering the panic.) [1][2][3]
It does not panic when self is NaN; in that case it just returns NaN. [1][2]
Do these panics happen in release builds?
Yes. The check is implemented via const_assert!(min <= max, ...), which expands to a normal runtime if !(condition) { ... panic ... } (not a debug_assert!), so it is present in both debug and release builds. [3][4]
Sources:
[1] f32::clamp docs (panic contract) (doc.rust-lang.org)
[2] f64::clamp docs (panic contract) (doc.rust-lang.org)
[3] core::num::{f32,f64}::clamp source uses const_assert!(min <= max, ...) (doc.rust-lang.org)
[4] core::panic::const_assert definition shows it panics via an if !condition at runtime (doc.rust-lang.org)
Citations:
- 1: https://doc.rust-lang.org/stable/std/primitive.f32.html?utm_source=openai
- 2: https://doc.rust-lang.org/beta/core/primitive.f64.html?utm_source=openai
- 3: https://doc.rust-lang.org/src/core/num/f32.rs.html?utm_source=openai
- 4: https://doc.rust-lang.org/beta/src/core/panic.rs.html
🏁 Script executed:
# Read the trait default clamp method with context (around line 681)
sed -n '675,690p' vendor/num-traits/src/float.rsRepository: rh-ecosystem-edge/recert
Length of output: 496
🏁 Script executed:
# Read the f32 impl clamp forwarding (around line 824)
sed -n '820,830p' vendor/num-traits/src/float.rsRepository: rh-ecosystem-edge/recert
Length of output: 548
🏁 Script executed:
# Search for crate::clamp definition to understand its contract
rg 'fn clamp' --no-heading -B2 -A8 vendor/num-traits/src/Repository: rh-ecosystem-edge/recert
Length of output: 5437
Remove Self::clamp forwards in concrete trait impls to prevent release-build panics that violate the trait contract.
The FloatCore and Float trait defaults delegate to crate::clamp, which only panics in debug builds (via debug_assert!). However, lines 824, 886, 1947, and 2031 forward to Self::clamp, which on f32/f64 primitives uses a runtime panic (const_assert!) that triggers in both debug and release builds on invalid bounds (min > max, NaN bounds).
This creates a silent contract violation: release builds using the concrete f32/f64 impls will panic on invalid clamp parameters, while code using the trait default (via generics) will silently clamp without panicking in release mode.
Remove the four Self::clamp forwards so all implementations consistently use the trait default (debug-only panic) or document the release-build panic explicitly in the trait contract.
Affected lines to remove forwards
- Line 824 (impl FloatCore for f32)
- Line 886 (impl FloatCore for f64)
- Line 1947 (float_impl_std! macro)
- Line 2031 (float_impl_libm! macro)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/num-traits/src/float.rs` at line 824, The concrete impls currently
forward to Self::clamp (remove the explicit Self::clamp overrides) which causes
f32/f64 primitive impls to use a release-time panic path; delete the explicit
clamp forwards in the impl FloatCore for f32 and impl FloatCore for f64 and
remove the clamp forwarding lines in the float_impl_std! and float_impl_libm!
macro expansions so those types fall back to the trait default clamp
implementation (which uses crate::clamp with debug-only assertions); ensure no
other explicit Self::clamp overrides remain so the trait default behavior is
consistent across generic and concrete uses.
danmanor
approved these changes
Mar 17, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danmanor, omertuc The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Member
Author
|
/override ? |
|
@omertuc: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Member
Author
|
/override pull-ci-rh-ecosystem-edge-recert-main-e2e-aws-ovn-single-node-recert-parallel |
Member
Author
|
/override pull-ci-rh-ecosystem-edge-recert-main-e2e-aws-ovn-single-node-recert-serial |
|
@omertuc: Overrode contexts on behalf of omertuc: ci/prow/e2e-aws-ovn-single-node-recert-parallel DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@omertuc: Overrode contexts on behalf of omertuc: ci/prow/e2e-aws-ovn-single-node-recert-serial DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-merge-bot
bot
merged commit b198320
into
rh-ecosystem-edge:main
16 checks passed
|
@omertuc: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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
2 participants
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.
See commit messages
Summary by CodeRabbit
New Features
Bug Fixes
Chores