Skip to content

Fix u64 overflow in Hypergeometric::new for populations near u64::MAX - #60

Open
SAY-5 wants to merge 1 commit into
rust-random:masterfrom
SAY-5:fix-hypergeometric-population-overflow
Open

SAY-5 wants to merge 1 commit into
rust-random:masterfrom
SAY-5:fix-hypergeometric-population-overflow

Conversation

@SAY-5

@SAY-5 SAY-5 commented Jul 27, 2026

Copy link
Copy Markdown
  • Added a CHANGELOG.md entry

Summary

Fixes #59. Hypergeometric::new computed n + 2 in u64, which overflows once total_population_size reaches u64::MAX - 1.

Motivation

For a population near u64::MAX (with valid K <= N and n <= N), the constructor should build a usable sampler. Instead, the n + 2 term overflowed: in debug/overflow-checked builds new panicked, and in release the wrap made the mode m infinite, which selected the H2PE branch with NaN constants so sample later panicked with EmptyRange.

Details

Compute the + 2 in f64 (n as f64 + 2.0) so the divisor no longer overflows u64. The mode is then computed correctly (~0 for the reproducer), the inverse-transform branch is selected, and sampling returns values in the expected support. Added a regression test constructing Hypergeometric::new(u64::MAX - 1, 3, 2) and drawing samples.

Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
@benjamin-lieser

Copy link
Copy Markdown
Member

Does n1 + 1 not have the same problem? This case is degenerated and will always produce K, maybe we should catch it earlier.
Also in f64 the +1.0 and the +2.0 operation saturate for big population sizes, this can cause the mode to be 1 even though it should be 0.
I am not sure if this is a problem though.

@SAY-5

SAY-5 commented Jul 27, 2026

Copy link
Copy Markdown
Author

n1 + 1 and k + 1 are safe: the swap above guarantees n1 <= n2 so n1 <= n/2, and k = min(sample_size, n - sample_size) <= n/2, so only the n + 2 term could actually overflow. The f64 saturation is inherent to computing m in f64 once n exceeds 2^53 (k as f64 and n1 as f64 lose precision the same way) and predates this change; as far as I can tell an off-by-one mode only shifts the HIN/H2PE selection right at the threshold, but happy to clamp or document it if you'd prefer.

This branch has not been deployed

No deployments
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.

Hypergeometric::new overflows computing n + 2 at population u64::MAX - 1

2 participants