-
Notifications
You must be signed in to change notification settings - Fork 42
Exception when trying to parse an IPv6 address #53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Unpack expects a byte sequence, not an int.
pcapng/structs.py
Outdated
| return ( | ||
| unpack_ipv6(value[:16]), | ||
| struct.unpack(self.endianness + "B", value[16]), | ||
| struct.unpack(self.endianness + "B", value[16:17]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't sound right, among other things because "B" cannot have endianness (it's a single byte) and it shouldn't unpack two bytes, but just one.
I would suggest changing this to just be value[16] without the unpack then.
This should be options valid for the interface description block: https://ietf-opsawg-wg.github.io/draft-ietf-opsawg-pcap/draft-ietf-opsawg-pcapng.html#name-interface-description-block (because it matches
python-pcapng/pcapng/blocks.py
Line 277 in 3e13e64
| Option(5, "if_IPv6addr", "ipv6+prefix", multiple=True), |
And that option is documented as:
The if_IPv6addr option is an IPv6 network address and corresponding prefix length for the interface. The first 16 octets are the IP address and the next octet is the prefix length. This option can be repeated multiple times within the same Interface Description Block when multiple IPv6 addresses are assigned to the interface.
since there's a single octect there's nothing to unpack at all there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
Yes, sure, I am not the one who wrote the unpack. I just thought I'd change as little as possible, you should be able to return value[16]. What was faulty was to call unpack with an integer and not a byte array and that caused an exception. As long as there is not exception any more and the integer value is return properly, it's OK.
Oh by the way, you are wrong if you think value[16:17] creates a 2-byte array, it only creates a 1-byte array. value[16:16] creates a 0-byte array. (I know I myself also sometimes get confused about basic python although I use it quite a lot). See: https://docs.python.org/3/library/stdtypes.html#typesseq
4. The slice of s from i to j is defined as the sequence of items with index k such that i <= k < j.
Cheers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gha I clearly need to have more coffee before reviewing code, sorry yes it's obvious now that I re-read them.
But yeah please change this to just return the octet, using the unpack on it will just throw the next person reading the code off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you see my new commit?
Unpack expects a byte sequence, not an int.