Rayhaan JaufeerallyandClaude Fable 5.1
a8aba52562
A review of the wire codec and the routing table, with the fixes. Bugs in the codec: - A NOTIFICATION with a code or subcode we have no name for (subcode 0, "unspecific", is common) failed to decode and was answered as a bad message. Every pair now reads, as BgpError::Other. - Optional parameters in the extended form of RFC 9072 were read and written with one-octet parameter lengths; they are two. - An ORF capability listing more than one address family made the whole OPEN malformed. - Bytes after an OPEN's optional parameters were accepted. - Extended communities were split into fields by rules that were wrong for the common types. They are kept as their eight octets. - Several length fields were computed in u16 and could wrap on encode. Simplifications in the codec: - Lengths are filled in after the value is written, for messages, attributes, capabilities and the UPDATE's two fields. The parallel wire_len functions, which had to be kept in step by hand, are gone. Message::to_wire no longer encodes into a second buffer. - Type codes for messages, attributes and capabilities are each defined once. The strum discriminant enums and their hand-written conversions are removed. - One function splits an attribute off a buffer; the UPDATE decoder, the plain attribute parser and the table's key reader all use it. - The UPDATE decoder's 185-line loop is an AttributeReader with one method that decides what becomes of an attribute. Duplicate detection is a bitset, not a HashSet allocated per UPDATE. - ParserContext::four_octet_asn is a bool. The "context not set" error path, which treated a programming error as a malformed route, is gone. - Codec functions take a context only if they read it, and return a Result only if they can fail. - MP_REACH_NLRI and MP_UNREACH_NLRI hold an AfiSafi. - AddressFamilyId is IpVersion, which is what it was. - Deserialize is no longer derived on codec types: nothing used it, and it bypassed IpPrefix's invariants. IpPrefix::to_wire, which was broken and unused, is removed. Bugs in the table: - The index of a row's best route was sixteen bits. A peer using ADD-PATH could send more routes for a prefix than that counts, and the index wrapped. It is 32 bits, at no cost in row size. - Shards and attribute-store stripes were picked from the top bits of the hash, which are the bits hashbrown keeps as the tag it compares first. With 64 shards each shard's table had 2 of 128 tag values. They are now picked from bits the table does not use. - remove_peer held the peer list's lock for the whole scan of the table, so no session could establish while a peer was removed. - When more prefixes went at once than were left, the forced trim dropped a fixed few entries, reset every reader, and re-ran the tidy on every later write. It drops the older half. - Readers were woken by every change, including a repeated announcement and changes their journal does not record. - Reader::poll with a budget of zero never made progress. Simplifications in the table: - The two journals of a shard are one Journals value written through three calls; the five places that kept them in step by hand are gone. Whether an entry is still live is decided in one place. - The shortcuts of route selection moved to select.rs, next to the invariant they depend on, which is now written down. - announce and withdraw take routes with or without path identifiers; the _paths variants are removed. - The selection key is read off the wire form only; the second implementation over parsed attributes is removed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Fuzz targets
Coverage-guided fuzzing of everything that reads input we do not control.
The property tests (cargo test, e.g. crates/packet/tests/properties.rs)
check the same things on generated input and run on every build; these
targets go further by letting libFuzzer follow code coverage for as long as
you leave them running.
| Target | Input |
|---|---|
message |
Raw bytes as they come off a TCP connection |
message_body |
A message body behind a correct header (reaches the body parsers) |
prefix_text |
A prefix as text |
config |
The configuration file |
For the two message targets a finding is either a panic, or a message the decoder accepted that cannot be written and read back as itself.
Running
libFuzzer needs a nightly toolchain and cargo-fuzz:
rustup toolchain install nightly
cargo install cargo-fuzz
Then, from this directory:
cargo +nightly fuzz run message_body corpus/message_body seeds/message_body -- -max_total_time=300
The first directory is the working corpus, which the fuzzer adds to and
which is not checked in. seeds/ holds a few valid inputs to start from.
Leave out -max_total_time to run until interrupted.
When it finds something
The input is saved under artifacts/<target>/. Reproduce it with
cargo +nightly fuzz run message_body artifacts/message_body/crash-...
Fix the bug, and add a unit test with the input next to the code it broke,
so that cargo test keeps it fixed without anyone having to run the fuzzer.