Bff fix#5294
Open
SirTyson wants to merge 2 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes additional out-of-bounds risks in the binary fuse filter’s retry/seed-rotation logic and strengthens deserialization validation, along with expanding unit tests to cover these rare paths deterministically.
Changes:
- Reworked seed rotation into a bounded, carry-aware increment routine and ensured populate error retries don’t fall through into peeling with cleared scratch state.
- Added validation for
SerializedBinaryFuseFilterfields during deserialization and tests for malformed serialized filters. - Expanded tests to force repeated populate/peeling retry paths and verify correctness / failure behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/util/test/BinaryFuseTests.cpp | Expands retry-path and serialization hardening tests; adds forced-retry instrumentation usage. |
| src/test/CovMark.h | Adds new coverage marks to assert seed-carry and max-retries-exhausted paths in tests. |
| lib/binaryfusefilter.h | Fixes seed rotation to avoid out-of-bounds indexing; adds serialized-filter validation and retry instrumentation. |
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
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.
Description
Fixes an additional out of bounds vector access bug that occasionally occurs in the binary fuse filter.
In the original fix here, I fixed an out of bounds issue and removed the parent try-catch loop around binary fuse filter construction. Unfortunately, there were two more out of bounds bugs that I did not catch, that have since been throwing after I removed the catch statement.
Given that these failures are probabilistic, I added a code coverage test and some instrumentation to deterministically trigger these failures in the original PR. However, I only triggered them once, and these out of bounds issues were only triggered when hitting the unlikely path multiple times in a loop. I have changed the unit test to now trigger the rare condition up to the maximum number of allowed times in the affected loops and fixed the bugs. I also added a little more hardening and unit tests around serialization while I was looking at it.
Checklist
clang-formatv8.0.0 (viamake formator the Visual Studio extension)