Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix overflow when initializing a Sieve with T::MAX #47

Merged
merged 4 commits into from
Oct 9, 2024
Merged

Fix overflow when initializing a Sieve with T::MAX #47

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6e8e890
Fix overflow when initializing a Sieve with T::MAX
dvdplm Sep 30, 2024
f00c1f9
Ensure all code paths are taken
dvdplm Sep 30, 2024
d9798c8
Remove contentious test
dvdplm Oct 8, 2024
20e06b3
Revert to previous unwrapping
dvdplm Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 17 additions & 12 deletions src/hazmat/sieve.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ impl<T: Integer> Sieve<T> {
self.base = self
.base
.checked_add(&self.incr.into())
.expect("addition should not overflow by construction");
.expect("Does not overflow by construction");

self.incr = 0;

Expand Down Expand Up @@ -214,17 +214,16 @@ impl<T: Integer> Sieve<T> {
let result = if self.current_is_composite() {
None
} else {
// The overflow should never happen here since `incr`
// is never greater than `incr_limit`, and the latter is chosen such that
// it does not overflow when added to `base` (see `update_residues()`).
let mut num = self
.base
.checked_add(&self.incr.into())
.expect("addition should not overflow by construction");
if self.safe_primes {
num = num.wrapping_shl_vartime(1) | T::one_like(&self.base);
match self.base.checked_add(&self.incr.into()).into_option() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like here we're branching on something that will never happen unless the private attributes are messed with? Also, instead of returning None for a composite now the method returns None for a composite or if there's an overflow, which can be confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This overflow can actually happen without any malicious shenanigans (and is guarded against with the sieve_with_max_start test).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you about the odd control flow here though. It gets complex quick:

  • Create a sieve and set base to T::MAX (like the test).
  • Call next()
  • The first call to update_residues() will work fine because incr == 0 (returns true)
  • …but this first call will set last_round to true as a side effect.
  • Now we call maybe_next() which will also work fine because T::MAX is composite (returns None)
  • …but now incr and incr_limit are 2 as a side effect.
  • We call update_residues again, and given that incr is now 2 and less or equal to incr_limit, we match on the first check and return true (note: last_round is true, which would cause returning false)
  • So now we call maybe_next again but now the addition overflows so we return None and the main loop continues
  • Back in update_residues() for the third time, we finally hit the check for last_round, so we return false
  • …which causes the main while-loop in next() to exit and we return None
  • …and the iterator stops.

Pheew!!!

Maybe the Sieve constructor should check if the given start is worth the effort? E.g. check if T::MAX - start contains "enough" primes. Maybe using PNT and check that T::MAX/log(T::MAX) - start/log(start) is larger than some number (1?, 10?)? Or can you think of something smarter?

Copy link
Contributor Author

@dvdplm dvdplm Oct 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

…then again, there are a lot of primes, so even if they pick a start that is 99.999999% of T::MAX there are still a LOT of prime candidates left, so maybe we just check that start != T::MAX and that's good enough?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't particularly like the idea of heuristic checks and relying on probabilities in this specific case (the sieve), I think if it's possible to do exactly, we should try to do it.

Now we call maybe_next() which will also work fine because T::MAX is composite

Not necessarily, 2^128-1 and 2^8192-1 are primes. (Actually the former can be used as a test)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Actually the former can be used as a test)

You mean 2^127 -1? Is that the last prime before U128::MAX, probably not right? Seems like a bit of an arbitrary test, or maybe I'm misunderstanding what you're suggesting here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, my mistake. Any 2^n-1 is indeed composite if n is composite.

Some(mut num) => {
if self.safe_primes {
// Divide by 2 and ensure it's odd with an OR.
num = num.wrapping_shl_vartime(1) | T::one_like(&self.base);
}
Some(num)
}
None => None,
}
Some(num)
};

self.incr += 2;
Expand All @@ -233,7 +232,6 @@ impl<T: Integer> Sieve<T> {

fn next(&mut self) -> Option<T> {
// Corner cases handled here

if self.produces_nothing {
return None;
}
Expand Down Expand Up @@ -385,4 +383,11 @@ mod tests {
assert!(format!("{s:?}").starts_with("Sieve"));
assert_eq!(s.clone(), s);
}

#[test]
fn sieve_with_max_start() {
let start = U64::MAX;
let mut sieve = Sieve::new(&start, NonZeroU32::new(U64::BITS).unwrap(), false);
assert!(sieve.next().is_none());
}
}
Loading