Support multiple entries in IPOption_Timestamp - #5117
Open
itzzdev09 wants to merge 1 commit into
Open
Conversation
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=[...]'.
Author
|
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. |
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.
Fixes #4513
Problem
RFC 791 says the originating host "must compose this option with a large enough timestamp data area to hold all the timestamp information expected", so that each router along the path can append an entry.
IPOption_Timestampmodelled exactly one entry — a single conditionalinternet_addressplus a singletimestamp. There was no way to reserve room for more, so a target host had nowhere to record its timestamp and would reply with the overflow flag set.Change
The data area is now a list, sized from the option length on dissection, following the pattern
IPOption_RRalready uses forrouters:flg=0(timestamp_only) →timestamps, a list of 32-bit timestamps.flg=1/flg=3→pairs, a list ofIPOption_Timestamp_Pair(an internet address followed by its timestamp).Reserving space for four replies now works:
and dissection round-trips:
Notes for review
Two deliberate decisions worth flagging:
This replaces the
internet_address/timestampfields. Callers move topairs=[IPOption_Timestamp_Pair(internet_address=..., timestamp=...)]ortimestamps=[...]. I couldn't see a way to keep the scalar names while supporting a list of interleaved address/timestamp pairs; happy to add compatibility properties if you'd like.Defaults are empty lists, matching
IPOption_RR, sopointernow defaults to5— the RFC minimum, pointing at the first free octet — instead of9, which assumed one entry was already present. A non-empty default is not safe here: scapy shallow-copies list defaults (value.copy()inPacket.do_init_cached_fields), so aPacketinside a default list would be shared across instances. I verified that with a mutation test while developing this.The two existing assertions in
inet.utsare updated to the new spelling and keep their original expected bytes; the checksum test keeps its 12-byte option (and so its existing checksum) by passing one explicit empty pair.Test plan
UTscapy -t test/scapy/layers/inet.uts→ 67 passed, 0 failed (same as before the change).flgshapes and dissection back into the lists.flake8 scapy/layers/inet.pywith the project config: 15 pre-existing issues before and after, none introduced.