Closed d-Rickyy-b closed 1 year ago
Thanks for the detailed report. I need to read that RFC, but you're probably correct, we need to include some additional buffer if ICMP responses with an error code added are valid.
I also have this problem OS:Windows 11 22000.100
Joining the discussion with another user report in https://github.com/evcc-io/evcc/discussions/1916#discussion-3698021. Would it hurt to just increase the buffer size or should that be done on consumer side?
I get this problem sometimes on a Windows 10 VM
Just had another user report this in https://github.com/evcc-io/evcc/discussions/1561#discussioncomment-2017588.
@d-Rickyy-b do I read your analysis right that pinger.Size = 548
should fix this for Windows 10?
@SuperQ any chance for a permanent fix?
The issue I was having on Windows was due to too many packets I was trying send out. I was doing ping sweeping and port scanning at the same time. There is some internal Windows limitation on how many concurrent connections you can have. Once I go over this limit the error message starts popping up. So it might not be related to the tool itself but a Windows limitation. I tried increasing the limits for the TCP/IP configs in Windows registry but nothing really helped. May be running it in a VM adds additional limitations based on the host.
I had another workaround. Looks like this error message causes subsequent pings to fail as well even for hosts that are up. I have to retry the ping if I encounter this error in order for it to be successful on live hosts. Can we please fix the buffer size?
@stanimirivanovde PRs are open!
If what @d-Rickyy-b suggests is correct and ICMP error messages can't grow bigger than 575 bytes then I think we need to do a math.Max()
between 575 and the size of the ICMP request.
... I think we need to do a
math.Max()
between 575 and the size of the ICMP request.
First off: I am no networking expert. All I wrote/will write in this GitHub issue comes from RFCs and internet resources I read.
I checked again: ICMP error messages SHOULD contain as much of the original datagram as possible without the length of the ICMP datagram exceeding 576 bytes.
. But of course there could be systems replying with ICMP error messages that are >576 bytes in length (e.g. if the original datagram was that large). So ICMP error messages can grow bigger than 576 bytes. The RFC only says "should".
According to RFC760:
This field [Total Length] allows the length of a datagram to be up to 65,535 octets. Such long datagrams are impractical for most hosts and networks. All hosts must be prepared to accept datagrams of up to 576 octets (whether they arrive whole or in fragments).
And:
Every internet destination must be able to receive a datagram of 576 octets either in one piece or in fragments to be reassembled.
From my understanding I would suggest using a buffer of at least 576 bytes to receive the ICMP packet. In a popular rust library they use 2048 as buffer size (https://github.com/aisk/ping/blob/053409b86552b82d53e57be7864534474dfd0a7a/src/ping.rs#L52-L69). This seems to be, because there are no specified packets larger than 2048 bytes. But in theory an adversary could reply with forged ICMP responses of 65,535 bytes in size.
Another C++ implementation seems to use a buffer of size 65,536 (https://github.com/john-z-yang/ping/blob/b4e888627355b66d9fb0c72805cc2a7894e002fb/src/pinger.cc#L97-L104).
This tool (https://github.com/schweikert/fping/blob/ab1ed993badc1a6978b95c9d17a5e8b56fff4598/src/fping.c#L100) uses a recv_bufsize of 4096.
I really am not sure what the best buffer size is. All I can say is that the current implementation uses a too small buffer.
@SuperQ how was this completed? Update I see- moving to unmaintained. Thanks for the heads-up.
Yes, sorry, due to lack of admin rights on this repo, we can't maintain it anymore.
Hey there,
I just started using this library and ran into an issue on Windows 10. It seems that for certain ICMP error messages, the following error message will pop up in your terminal:
read ip4 0.0.0.0: wsarecvfrom: A message sent on a datagram socket was larger than the internal message buffer or some other network limit, or the buffer used to receive a datagram into was smaller than the datagram itself.
This seems to be related to the devices that are unreachable and a too small size of a buffer .
Steps to reproduce
go get -t "github.com/go-ping/ping"
)Expected behavior
The request works fine and
execPing
will return False because the host is not reachable.Actual behavior
The error written earlier will appear in the terminal and
execPing
will not return False but instead return an error.Assumptions
What I found out so far is that the buffer for receiving the ICMP response is calculated wrongly and hence is too small.
The buffer
bytes
https://github.com/go-ping/ping/blob/ff8be3320020b96856d3d48505bcf8926809752e/ping.go#L558
is calculated by
https://github.com/go-ping/ping/blob/e4e642a95741c494990b0c8126988d1fd3a5449c/utils_windows.go#L10-L16
and then being used to read the ICMP response into:
https://github.com/go-ping/ping/blob/ff8be3320020b96856d3d48505bcf8926809752e/ping.go#L564
With
pinger.Size
up to 547 I still get the above error message. As soon as I use apinger.Size
of 548 or bigger, I am not getting the error anymore.pinger.Size = 547
:In the above case the ICMP Request Data (which is contained within the response) is limited to just 520 bytes (even though we defined it to be 547 Bytes) because it's being cut off for the ICMP datagram size not to exceed the limit of 576 Bytes total (RFC 1812 - section 4.3.2.3). Now we use the earlier mentioned code to calculate the message/buffer size. The buffer size is calculated as
p.Size + 8 + ipv4.HeaderLen
= 547 + 8 + 20 = 575. That means that our buffer is too small for the data we try to receive.Now if we choose
pinger.Size = 548
we can see that our buffer is now big enough (548 + 8 + 20 = 576) to handle the same ICMP response. Any value >= 548 works just fine. Anything below that will lead to the issue described above.I was not able to reproduce the issue on Linux.
Suggestion
As we can see in the following picture, also the IP header gets parsed into the passed buffer.
For regular, non-error requests it's totally fine to use a buffer of the size of the request. IMHO for ICMP error messages the buffer should be of size:
p.Size + (8 + ipv4.HeaderLen) * 2
. Also opposed to non-error messages, it would be fine to use a maximum buffer size of 575 Bytes because error messages do not grow bigger than that, while non-error ICMP messages can theoretically reach 65507 Bytes in size.Sorry for the wall of text. I hope you get the point and will implement a solution for this issue. Thank you!