Repository navigation
fix(parser): decode an NLRI block whole so a leading default route survives - #361
Merged
Merged
Conversation
…rvives parse_nlri_list read a leading 0x00 byte as an ADD-PATH path identifier and kept that reading for the rest of the block. A default route (RFC 4271 §4.3) is a valid non-ADD-PATH encoding of exactly that byte, so the four bytes after it were eaten as a path id, and the retry the memory optimization added in b3f52ad resumed at the failure position rather than the block start. Prefixes already decoded out of path-id bytes stayed in the list and the real ones were dropped. When both readings failed, the message became a MalformedNlri warning with an empty withdrawal list, so every withdrawal in it disappeared without an error. Decode the block whole under one reading, and fall back to the whole block under the other. ADD-PATH data wrapped in a non-ADD-PATH subtype still decodes with its path ids, which is the case the guess exists for; a plain block whose bytes also frame exactly as ADD-PATH still takes the guess, because nothing in the bytes decides between them. Verified against the three RIPE RIS files named in #360: an independent MRT decoder and this build now agree on all 1,039,666 records and on every prefix of the 1020-, 166- and 627-withdrawal messages. route-views.amsix 2020-06-17 18:30, the ADD-PATH-under-subtype-4 data from #31, is unchanged at 3,679,857 elements.
The Fixed entry said withdrawals are no longer lost when a block starts with the default route. A short block that frames exactly as ADD-PATH is still decoded that way, so 0.0.0.0/0 followed by one /24 keeps losing the /24. Scope the headline to a guess that cannot frame the block, name the limitation in the entry and in the doc comment, and pin it with a test so the behaviour cannot drift silently. Also restore a debug trace when the guess is taken and when it fails, which the old heuristic emitted, and correct the "two thirds" figure to the measured 74% of the amsix file.
A block that fails under both framings does not always yield a MalformedNlri finding: that is the classic IPv4 withdrawn/announced field path, while a failing MP_UNREACH_NLRI attribute is stored raw and raises OptionalAttributeError. Say which field produces which, and assert in the tie-break test that the plain framing also consumes the block, so the premise the decision rests on is executable.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are recorded for the NLRI decoding fix.
No open issues 🎉
What changed in this PR
Fixes NLRI decoding so a failed ADD-PATH guess retries the entire block, preserving default-route withdrawals affected by #360 while retaining compatibility with mislabeled ADD-PATH records from #31.
Changes:
- Decodes each NLRI block under one consistent framing and restarts from the beginning when the guess fails.
- Adds regression tests for IPv4/IPv6 default routes, withdrawal messages, and ADD-PATH compatibility.
- Documents the remaining ambiguity: blocks valid under both framings still prefer ADD-PATH.
| File | Description |
|---|---|
src/parser/utils.rs |
Implements whole-block decoding and fallback, with framing regression tests. |
src/parser/bgp/messages.rs |
Adds a message-level test preserving withdrawals that start with a default route. |
CHANGELOG.md |
Records the fix, observed impact, and remaining ambiguous-block limitation. |
Review effort: Balanced
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #361 +/- ##
==========================================
+ Coverage 92.02% 92.09% +0.06%
==========================================
Files 97 97
Lines 24759 24868 +109
==========================================
+ Hits 22785 22901 +116
+ Misses 1974 1967 -7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
digizeph
requested
a balanced review from Copilot
and removed request for
Copilot
November 6, 2026 17:16
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.
Problem
parse_nlri_listreads a leading0x00byte as an ADD-PATH path identifier when the record saysnon-ADD-PATH, keeps that framing for the rest of the block, and retries at the failure position
instead of the block start.
0x00is also the RFC 4271 §4.3 encoding of the default route, so awithdrawn list that begins with
0.0.0.0/0or::/0loses prefixes from the middle of the list, orloses every withdrawal in the message when both framings fail and
read_nlriturns the error into anempty list plus a
MalformedNlriwarning.Change
The block is decoded whole under one framing, and a failed ADD-PATH guess re-runs over the whole
block. The guess itself stays: it exists for producers that write ADD-PATH data under a
non-ADD-PATH subtype (issue #31).
A non-ADD-PATH block whose bytes also frame exactly as ADD-PATH is still read as ADD-PATH. Nothing in
the bytes decides between the two readings, and on the data that motivated the guess the ADD-PATH
reading is the correct one. A short block of that shape,
0.0.0.0/0followed by one prefix, keeps theold reading; that residual is named in the CHANGELOG entry and pinned by
test_parse_nlri_list_short_ambiguous_block_is_read_as_add_path.Evidence
An independent MRT decoder and released 0.23.0 decode the three RIPE RIS files in #360 (1,039,666
records). Exactly three (timestamp, peer) keys differ, and they are the three reported messages:
prefixes were lost and 7 invented (
aa00::/1,::/2, ...)With this change all three files match the raw decoder on every prefix, and the three messages
decode to 1020, 166 and 627 withdrawals.
route-views.amsix 2020-06-17 18:30 UTC, the ADD-PATH-under-subtype-4 data from issue #31, keeps
3,679,857 elements. Deleting the guess instead takes that file to 955,417 elements (-74%) with no
warning, which is why the guess is kept and only its failure path changes.
Tests
New cases pin the bug rather than the implementation: a crate outside this repository running them
against
origin/mainfails the IPv4 and IPv6 default-route-at-head cases (the IPv4 one returns0.0.0.0/0, 1.0.5.0/24, 1.0.6.0/24), while the lone-default-route and wrapped-ADD-PATH cases pass onboth trees. The tie-break test asserts that the plain framing also consumes the ambiguous block, so
the decision it locks is executable rather than implied.
Refs #360.