Skip to content

Update icmp.h#101

Merged
ofalk merged 1 commit intoofalk:develfrom
Lomasterrrr:patch-2
Feb 13, 2025
Merged

Update icmp.h#101
ofalk merged 1 commit intoofalk:develfrom
Lomasterrrr:patch-2

Conversation

@Lomasterrrr
Copy link

here too the fields are incorrectly sized, you can understand even by the htons function in the icmp_pack_hdr_mask macro.

here too the fields are incorrectly sized, you can understand even by the htons function in the icmp_pack_hdr_mask macro.
@ofalk ofalk self-assigned this Jun 18, 2024
@ofalk
Copy link
Owner

ofalk commented Jun 18, 2024

Likewise to #101 - I think it's uint32 because of ICMPv6, but maybe I'm missing something.

@ofalk ofalk changed the base branch from master to devel February 13, 2025 16:06
@ofalk
Copy link
Owner

ofalk commented Feb 13, 2025

Likewise to #101 - I think it's uint32 because of ICMPv6, but maybe I'm missing something.

Yes, likewise, 16bit are enough.

@ofalk ofalk merged commit 91b6711 into ofalk:devel Feb 13, 2025
@Lomasterrrr
Copy link
Author

In ICMPv6 there is no query mask. And in icmp4 its id and seq fields are 16 bits. In RFC 590 it is not written directly, but by the ASCII art of the header, you can see that the size of these fields is 16 bits.

To prove it I can attach screenshots from wireshark, where you can see that this message has everything as I say,

https://i.imgur.com/NVYUKJi.png
https://i.imgur.com/5VUcXBS.png

ofalk added a commit that referenced this pull request Jul 13, 2025
* Encoding fixes
* CI Config (a few iterations) w/ Ubuntu and Windows target
* Typo fixes
* Fixes in icmp.h (#100 #101) - wrong size of fields for icmp4 timestamp message / 16bits are enough for Id/Seq.
* Fix payload size of Netlink message (#99) - The `len` parameter of `NLMSG_LENGTH`  was size of Netlink header instead of RtNetlink header.
* python/dnet.pyx: fix incompatible-function-pointer-types for modern compiler (#104)
* Cythonize with latest Cython (3.1)
* Fix #109 PF_PACKET support detection to failure
* tree-wide: regenerate autotools files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants