Conversation
|
Disclosure, per the "AI-assisted reports and PRs" section of CONTRIBUTING: this PR was drafted with AI assistance (Claude Code, Claude Opus 5). I should have said so in the original description — apologies for the omission. Everything in it was verified by running it rather than assumed:
One thing worth calling out, since it drove a design decision: a non-empty Happy to adjust anything, including the field naming, if you'd prefer a different shape. |
RFC 791 requires the originating host to compose the timestamp option with a data area large enough to hold every timestamp it expects back, so that each router on the path can append one. IPOption_Timestamp modelled only a single entry, so a target host had no room to record anything and would reply with the overflow flag set. Model the data area as a list instead: - flg 0 (timestamp_only) uses 'timestamps', a list of 32-bit timestamps. - flg 1/3 use 'pairs', a list of IPOption_Timestamp_Pair, each an internet address followed by its timestamp. Both are sized from the option length on dissection, mirroring IPOption_RR. The defaults are empty lists, also as in IPOption_RR, so 'pointer' now defaults to 5 (the RFC minimum, pointing at the first free octet) rather than to 9, which assumed one entry was already present. Note this replaces the 'internet_address' and 'timestamp' fields; existing callers move to 'pairs=[IPOption_Timestamp_Pair(...)]' or 'timestamps=[...]'. AI-Assisted: yes (Claude Sonnet 5)
f838267 to
24ee34b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5117 +/- ##
==========================================
- Coverage 80.63% 80.07% -0.56%
==========================================
Files 390 375 -15
Lines 96895 97680 +785
==========================================
+ Hits 78127 78219 +92
- Misses 18768 19461 +693
🚀 New features to boost your workflow:
|
98d0e99 to
24ee34b
Compare
| ConditionalField(IPField("internet_address", "0.0.0.0"), | ||
| lambda pkt: pkt.flg != 0), | ||
| IntField('timestamp', 0)] | ||
| ConditionalField( |
There was a problem hiding this comment.
Please use a MultipleTypesField
| IntField('timestamp', 0)] | ||
| ConditionalField( | ||
| FieldListField( | ||
| "timestamps", [], IntField("", 0), |
There was a problem hiding this comment.
Please don't change the field name and the original field type, except there are valid reasons to do so
| ConditionalField( | ||
| PacketListField( | ||
| "pairs", [], IPOption_Timestamp_Pair, | ||
| length_from=lambda pkt: max(0, (pkt.length or 4) - 4)), # noqa: E501 |
There was a problem hiding this comment.
Simplify the lambda expression
Address review: keep the original field name, timestamp, instead of the separate timestamps/pairs fields, select its type from flg with a single MultipleTypeField rather than two ConditionalFields, and simplify the length_from lambda. The field still holds a list, since RFC 791 has the originating host reserve room for every entry it expects back; each address/timestamp pair keeps the original internet_address and timestamp names and types. AI-Assisted: yes (Claude Opus 5)
|
Thanks for the review! Done in fc08d0b: switched to a MultipleTypeField, kept the original timestamp name (each entry keeps internet_address / timestamp with their original types), and simplified the lambda. The one intentional type change is that timestamp is now a list, since RFC 791 has the sender reserve room for several entries — that's the point of the PR. Happy to adjust if you'd prefer a different shape. @polybassa |
| lambda pkt: pkt.flg != 0), | ||
| IntField('timestamp', 0)] | ||
| MultipleTypeField( | ||
| [(FieldListField("timestamp", [], IntField("", 0), |
There was a problem hiding this comment.
Why did this became a FieldListField?
There was a problem hiding this comment.
Because the option can carry more than one timestamp. RFC 791 says the sender has to leave room for all the timestamps it wants routers to fill in, so with flg=0 you get a list of 32-bit values, not a single one. With just an IntField you could only ever read the first one. The size comes from length, same as IPOption_RR does for its addresses.
| [(FieldListField("timestamp", [], IntField("", 0), | ||
| length_from=lambda pkt: pkt.length - 4), | ||
| lambda pkt: pkt.flg == 0)], | ||
| PacketListField("timestamp", [], IPOption_Timestamp_Pair, |
There was a problem hiding this comment.
Please choose a field name that indicates that this field holds pairs of IP and Timestamps
There was a problem hiding this comment.
Renamed it to ip_timestamp_pairs in 52d5ac0. The flg=0 list is still called timestamp.
Heads up, I had to drop the MultipleTypeField for this. It wants the same name for every variant, so with two different names I went back to two ConditionalFields. Let me know if you'd rather have it another way.
Address review: the address/timestamp pair list gets a name of its own, ip_timestamp_pairs, so it no longer shares timestamp with the flg 0 list of bare timestamps. A MultipleTypeField needs one name for all of its variants, so the two lists are separate ConditionalFields selected by flg. AI-Assisted: yes
2 participants
What does this fix?
IPOption_Timestampcurrently supports only a single address/timestamp entry, which makes it impossible to build a valid multi-entry timestamp option. RFC 791 requires the sender to reserve enough space for all the timestamps it expects to receive back.This change makes the timestamp entries a list whose size is determined by the option length. With flg=0, the option uses timestamp, while flg=1 and flg=3 use ip_timestamp_pairs containing the address and timestamp.
The default
pointeris also changed from 9 to 5.This closes #4513.
One compatibility change is that the old
internet_addressandtimestampfields are removed. Existing code such asIPOption_Timestamp(internet_address=..., timestamp=...)will therefore no longer work, and the tests have been updated accordingly.AI-Assisted: yes (Claude Sonnet 5), as noted in the commit.