Repository navigation
fix(protocols): rebuild noise and cut-recording gaps found reviewing #2703 #2711
Description
Found while reviewing the #2703 fix and confirmed by a Sonnet cross-review. All three are identical on main and low priority.
- Outer rebuilds repeat a payload's messages. Rebuilding an outer layer over a cut payload, such as IGMPv3, re-parses the payload and repeats its parse-time messages. Ethernet logs four (CRITICAL and WARNING, twice) where IPv6 logs two. A rebuild should report nothing the parse already reported.
- An IGMPv3 Report cut on a whole field, built directly, reads INVALID. At 8 and 20 octets no short read is recorded, so a direct parse reads INVALID and warns. Through IPv4 the same cuts read TRUNCATED.
- Needs a decision: cuts with no record. A GRE message cut inside its payload, or an ICMP inside a TSO frame cut by the capture, records nothing, so a standalone rebuild warns INVALID exactly as the parse does. Should a payload cut be recorded on the layer, so that it reads TRUNCATED?
Activity
JarryShaw commentedon Nov 3, 2026
A worker is on items 1 and 2, stacked on #2703's rebuild-path change.
Needs deciding, item 3: a GRE message cut inside its payload, or an ICMP inside a TSO frame cut by the capture, leaves no short-read record on the layer. Its standalone rebuild therefore warns INVALID, exactly as its parse did.
Recommendation: 1. The checksum covers octets that were never captured, so INVALID is a false alarm. #2668 and #2689 already treat such cuts as TRUNCATED when a frame length is present, and 1 makes the standalone layer agree. Going ahead with 1 after items 1 and 2, unless you pick otherwise.
JarryShaw commentedon Nov 3, 2026
take 1
JarryShaw commentedon Nov 3, 2026
Ruling taken: option 1 for item 3. A layer whose captured octets fall short of the length it declares now records the cut, so both its parse and its standalone rebuild read TRUNCATED, not INVALID. This covers a GRE message cut inside its payload, and an ICMP inside a TSO frame cut by the capture.
How this fits with the rest: items 1 and 2 are unchanged, and the worker on them takes item 3 in the same commit, rebased onto
mainnow that #2712 has merged.needs: decisionis removed and the issue stayswip.JarryShaw commentedon Nov 3, 2026
The worker has all three items in one commit, and a Sonnet review found a false-positive cut that is being fixed. One point needs your call, because it rewrites your #2588 wording.
Needs deciding: an error that
beholderabsorbs while a payload is rebuilt inside_make_payload. Today it is logged twice: once by the rebuild and once by the outer layer's own parse of the same octets.ProtocolErrorgoes quiet, so other classes such asFieldValueErrorkeep logging twice on a rebuild.Recommendation: 1. Each error is still reported exactly once, from the parse that owns it. Going ahead with 1 unless you pick 2.
JarryShaw commentedon Nov 3, 2026
take 1
JarryShaw commentedon Nov 3, 2026
Ruling taken: option 1. While a payload is rebuilt inside
_make_payload,beholderabsorbs an error of any class quietly. The outer layer's own parse of the same octets still reports it once. The "Handled Refusals" page records this as the one exception to #2588'sProtocolError-only rule.How this fits with the rest: the worker already kept that change, so nothing changes in flight. The false-positive cut and the IGMPv3 count ceiling are still being fixed in the same commit.
needs: decisionis removed and the issue stayswip.JarryShaw commentedon Nov 3, 2026
Round 4 is in at
c9e0c968a. The SCTP regression is fixed: a layer whose last field is a list records no frame cut. A standalone IGMPv3 Report with an overstated count no longer raises. One point needs your call.Needs deciding: a standalone, header-only IGMPv3 Report whose count exceeds the records present. Where the zero-fill budget covers the counted records, it reads TRUNCATED with zero-filled records. Where it does not, it reads as on
main: VALID, no records. The budget grows with what the context has already read, so the same octets can give either result, depending on what was parsed before. Neither raises.Recommendation: 1. It is a narrow edge, it never raises, and it always round-trips. 2 is real new work. Going ahead with 1 unless you pick 2.
JarryShaw commentedon Nov 3, 2026
The PR is #2738. The budget question above is still open; the PR follows option 1 in the meantime.
JarryShaw commentedon Nov 3, 2026
would prefer 2
JarryShaw commentedon Nov 3, 2026
Ruling: option 2. A standalone, header-only IGMPv3 Report must read the same way whatever the context parsed before it. So records past those actually present are never zero-filled, and the cut is recorded by a new mechanism in the list field, not through the zero-fill budget.
This replaces what #2738 does now, which is option 1, so #2738 is back to needs changes. Round 5 has gone to the same worker. Nothing else in #2738 changes.
fix(protocols): report a rebuilt frame's messages once and record cut…