-
Notifications
You must be signed in to change notification settings - Fork 0
Description
Summary
SlotRecyclingLib.bitmask(offset, width) is part of the documented public API, but it does not currently validate its inputs the way its documentation implies.
Today:
width >= 256can revert via checked arithmetic underflow in((1 << width) - 1).offset + width > 256can silently truncate the intended mask once the left shift is applied.- Consumers can use the helper to build a
clearMaskthat looks correct at the call site but does not actually clear all intended non-vacancy bits.
The core library only validates that free() clears the vacancy region. It does not validate the rest of a consumer-supplied clearMask, so bitmask() should have a precise and deliberate input contract instead of inheriting incidental shift behavior.
Why this matters
This is not a direct corruption bug in the core allocate / free flow when consumers pass valid masks, but it is an API sharp edge in a documented helper that downstream integrations are expected to copy from the README.
The failure modes are poor:
- some invalid inputs panic with generic arithmetic errors
- other invalid inputs produce a mask that does not match the caller's apparent intent
- neither outcome is described in the API docs
Because bitmask() is called out in the README and the semver policy treats library function signatures as public API, consumers may reasonably assume it has a stable and well-defined input domain.
Proposed solution
Keep bitmask() as a generic helper for building masks, but make its contract explicit and validated.
Recommended contract:
- valid inputs satisfy
offset <= 256,width <= 256, andoffset + width <= 256 width == 0returns0width == 256returnstype(uint256).max(which impliesoffset == 0under the sum rule)- all other invalid inputs revert deliberately instead of panicking or truncating
Implementation direction:
- add explicit bounds checks before performing shifts
- special-case the
width == 256path so full-width masks are supported intentionally - prefer a dedicated error such as
BadBitmask(uint256 offset, uint256 width)rather than reusingBadRecycleConfig, since this helper is not config-specific
Documentation direction:
- document
bitmask()as a generic range helper - document
create()as intentionally stricter: byte-aligned,8..248, and config-specific - make the difference between the two APIs explicit in NatSpec and README
Why this approach
This resolves the ambiguity without unnecessarily shrinking the helper:
- Better than mirroring
create()exactly:bitmask()is used to buildclearMask, which does not need the byte-alignment and8..248restrictions that vacancy configs need. - Better than docs-only: the current behavior includes silent truncation and generic panic paths, which are not good public API semantics.
- Better ergonomics: full-width masks are a natural use case for a generic helper and already appear conceptually in the test suite via
type(uint256).max.
Alternatives considered
1. Align bitmask() exactly with create()
Pros:
- fewer distinct rules to explain
Cons:
- over-constrains a generic helper
- disallows sensible non-config uses such as a deliberate full-width mask
- does not match how the helper is described in the README
2. Narrow the documented contract without changing code
Pros:
- smallest implementation change
Cons:
- blesses silent truncation and generic panic behavior as public semantics
- keeps the helper easy to misuse
- weak fit for a repo that is explicitly trying to freeze and document public API behavior
Acceptance criteria
- Define and document the supported input contract for
bitmask(). - Make invalid inputs fail in a deliberate and documented way instead of relying on incidental shift behavior.
- Support the full-width case intentionally if the helper remains generic.
- Add focused tests for:
- normal in-range masks
width == 0width == 256offset + width == 256- invalid
offset + width > 256 - invalid huge inputs that currently panic or truncate
- Update NatSpec and README so the distinction between
create()andbitmask()is explicit. - If a new error is introduced, update the compatibility fixture and semver/release notes accordingly.
Semver note
If this change introduces a new public error such as BadBitmask, it should be treated as a minor release under STABILITY.md, not a patch release.
Non-goals
- Redesigning the core recycling mechanism.
- Adding validation that attempts to prove an arbitrary
clearMaskmatches a consumer's intended data layout.