oxidecomputer / oxidecomputer/maghemite
BFD doesn't guarantee local discriminators are unique or non-zero
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 94
- Forks
- 6
- Avg merge
- 1d 8h
- Merged PRs (30d)
- 12
Description
The following is an issue identified by AI when I asked it to do an adversarial code review of #795. This was pre-existing in the sync/pthread impl, which is why it was not addressed as part of the async swap-out.
Problem
PeerInfo::with_random_discriminator() currently uses an unrestricted
random u32.
This permits:
- discriminator zero, which RFC 5880 forbids; and
- collisions with another active local BFD session.
The chance of zero is small for an individual session, but collisions
grow according to the birthday bound as the number of active sessions
increases. More importantly, the required invariant is not represented
or enforced anywhere.
An outgoing session with discriminator zero will send packets that a
compliant peer must discard.
Required behavior
RFC 5880 requires bfd.LocalDiscr to be:
- nonzero; and
- unique across all BFD sessions on the system.
It recommends selecting a random value subject to those constraints.
Proposed direction
- Represent a local discriminator as
NonZeroU32. - Allocate discriminators at the daemon level, where all active sessions
are visible. - Generate random candidates until one is both nonzero and unused.
- Release the discriminator when the session has completely shut down.
The packet wire representation can remain u32, but outgoing packet
construction should make an invalid value unrepresentable.
Testing
- Generated discriminators are never zero.
- Discriminators are unique across a large collection of concurrently
active test sessions. - A forced RNG collision causes retry rather than duplicate allocation.
- Removing a session permits safe eventual reuse after shutdown.
References
- RFC 5880 §6.3
- RFC 5880 §6.8.1
Context
This behavior predated the async implementation and was retained by #795
and #796.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start at PeerInfo::with_random_discriminator() and trace how the daemon creates and shuts down BFD sessions. Then inspect outgoing packet construction and the existing async and sync/pthread implementations. Done means nonzero, unique discriminators with retry-on-collision behavior, safe reuse after shutdown, and tests covering the listed cases.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- networking
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100