Skip to content

Conversation

@reallesee
Copy link
Contributor

Fixed a bug in BloomFilter's Arbitrary implementation where hashers could be zero.
When hashers=0, the indices() method returns an empty iterator, which causes contains() to always return true regardless of whether the item was actually inserted. This completely breaks the bloom filter semantics (100% false positive rate).

The public constructor already requires NonZeroU8, so arbitrary should respect the same invariant. Added .max(1) to ensure at least one hasher is always generated.

@danlaine
Copy link
Collaborator

danlaine commented Dec 11, 2025

I think it might make more sense to make the field hashers: NonZeroU8?

@reallesee
Copy link
Contributor Author

The public API already requires NonZeroU8, so the invariant is enforced at construction. This just fixes arbitrary to respect the same constraint. Changing the internal field would be a larger refactor for no practical benefit.

patrick-ogrady
patrick-ogrady previously approved these changes Dec 18, 2025
@patrick-ogrady
Copy link
Contributor

With this change, you'll need to call regenerate-conformance (in the justfile).

@reallesee reallesee force-pushed the fix-bloomfilter-arbitrary-zero-hashers branch from 83d6b88 to 768875b Compare December 20, 2025 13:17
@reallesee
Copy link
Contributor Author

With this change, you'll need to call regenerate-conformance (in the justfile).

Did it, thanks

@codecov
Copy link

codecov bot commented Dec 20, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.63%. Comparing base (008e58e) to head (768875b).

@@            Coverage Diff             @@
##             main    #2460      +/-   ##
==========================================
- Coverage   92.64%   92.63%   -0.01%     
==========================================
  Files         353      353              
  Lines      102482   102482              
==========================================
- Hits        94940    94939       -1     
- Misses       7542     7543       +1     
Files with missing lines Coverage Δ
cryptography/src/bloomfilter.rs 100.00% <ø> (ø)

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 008e58e...768875b. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@reallesee
Copy link
Contributor Author

@patrick-ogrady CI needs breaking-format label, can you add it?

@andresilva
Copy link
Collaborator

#2729 includes this as well (didn't see this PR before).

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.

4 participants