-
Notifications
You must be signed in to change notification settings - Fork 21.6k
fix(bintrie): prevent OOB on 31-byte stems at depth 248 #33339
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(bintrie): prevent OOB on 31-byte stems at depth 248 #33339
Conversation
|
Add caller-side guards const St1emSize8 = 248 // example: number of bits for a 32-byte key func insertAtStem(stem []byte, depth int, ... ) error { Reject invalid path generation func (n *HashedNode) InsertValuesAtStem(stem []byte, depth int, vals ...Value) error { Internal node read guard (If not already present) func (in *InternalNode) readStemBit(stem []byte, depth int) (int, error) { Tests you should add
Caller responsibilities and options
Deployment checklist
If you share a snippet where this surfaces, I can mark exactly where to add the guard and suggest the tightest fix for your codebase style. |
|
Guard Placement
📌 Why this is the tightest fix
✅ Checklist for your codebase style
👉 If you show me your actual function signatures (like how readStemBit and InsertValuesAtStem are structured in your repo), I can drop in the exact guard code styled to match your conventions — whether you prefer returning error, panicking, or using bool flags. |
|
Since you’re already using error returns (not panics), the tightest fix is to add explicit guards in the two critical functions and define reusable error constants.
`go } ✅ Why this is the tightest fix
|
|
let’s make this airtight with fuzz tests around the boundary depths (245–251). These will automatically catch regressions if someone later changes the code and accidentally reintroduces out‑of‑bounds reads. 🧪 Fuzz Test Strategy
Example Fuzz Test in Go `go } Benefits
👉 This way, you don’t just fix the bug — you lock the boundary behavior into your test suite so it can’t sneak back in. |
|
let’s lock down InsertValuesAtStem with fuzz tests so you’re covered on both the read and insert paths. 🧪 Fuzz Test Strategy for InsertValuesAtStem
Example Fuzz Test `go } ✅ Why this locks it down
👉 With these fuzz tests in place, you’ll have automatic guardrails against out‑of‑bounds reads in bintrie. |
|
combine both fuzz tests into a single fuzz suite so you can run them together without duplicating harness code. This keeps your test suite lean and ensures both readStemBit and InsertValuesAtStem are always validated side by side. Combined Fuzz Suite `go } ✅ Why this is the leanest approach
With this combined fuzz suite, you’ll have one test that automatically validates both critical guard points. If either function regresses, fuzzing will catch it. |
|
wire your combined fuzz suite into go test -fuzz so it runs continuously in CI with proper timeouts and coverage reporting. This way, you’ll catch regressions automatically and keep your bintrie guards rock‑solid.
`yaml on: jobs:
✅ With this setup:
|
|
add corpus files so your fuzz suite always reuses tricky edge cases like 29‑byte stems at depth 247. This makes fuzzing more effective because Go’s fuzz engine will start from your saved inputs every run, not just random ones.
Add files like: seed29byte247.txt seed31byteboundary.txt seed32byteboundary.txt seed32bytebeyond.txt
Go will:
✅ Benefits
|
|
add logging hooks so you can see which corpus inputs are being exercised during CI runs. This makes fuzzing more transparent and helps confirm that your seeded edge cases (like 29‑byte stems at depth 247) are actually being tested.
`go }
This will print lines like:
`yaml
This ensures logs appear in CI output, so you can confirm corpus inputs are exercised. ✅ Benefits
|
|
CI fail on low coverage Below is a compact, idiomatic approach you can drop into your GitHub Actions workflow to fail the job when coverage falls below a configurable threshold. It runs go test with coverage, extracts the total coverage, compares it to a threshold, and exits nonzero if coverage is too low. Shell script snippet to check coverage `bash !/usr/bin/env bash Configurable threshold as a percentage, e.g., 80.0 Run tests and produce coverage Extract total coverage percentage Compare floats Usage GitHub Actions step example `yaml on: jobs: Variations and enhancements
Recommended policy
|
This change enforces stem-depth guards in bintrie to avoid out-of-bounds reads when operating on 31-byte stems at depth 248. Internal nodes now reject depth >= StemSize8 before reading a stem bit, and HashedNode.InsertValuesAtStem rejects generating a path for the same boundary. The keyToPath function remains unchanged because it intentionally supports depth == StemSize8 for full 32-byte keys as covered by tests; the bug was in callers passing 31-byte stems at that depth.