Increase maximum -s value to 65507

[pevik]

Correct definitions: maximum theoretical IPv4 packet size and minimum IPv4 header size (previously probably IPv6 size was used).

Because fping does not know ahead whether address is IPv4 or IPv6 assume IPv4. Previously fping allowed only 65488, but real maximum for IPv4 on Linux is 65507 (IPv6 would allow 65527).

[gsnw-sebast]

The pull request is correct for IPv4.

In general, an attempt could be made to differentiate between IPv4 and IPv6.
For IPv6, the values should be as follows.

#define MAX_IP_PACKET 4294967295
#define SIZE_IP_HDR 40
#define SIZE_ICMP_HDR 8

[auerswal]

Thanks for the pull request. It still needs some work before it can be accepted.

1. There is an existing off-by-one error in the size check and error message. Currently, that does not matter much, because the assumed IP header size of 40 bytes is bigger than the off-by-one error. With your corrected IPv4 minimum header size, the reported error wrongly states that the value must be less than 65507, but 65507 is OK with your pull request. Please adjust the error message to be correct, e.g., "…must not be larger than…".
2. There is a test that checks the error message when a too large value is used with -b. That test fails with your pull request because the reported size limit changes. Please adjust that test, you can find it in ci/test-03-forbidden.pl.
3. The test mentioned above currently uses an ICMP payload size of 65509 which is two bytes above the (IPv4) maximum. While you are working on this code, could you also adjust this to use 65508?

Thanks in advance and best regards,
Erik

[auerswal]

Please drop this change. 64K is widely understood as 2^16 and thus nearly correct (off-by-one error, 2^16-1 for IPv4). Using 65K is further off, both interpretations 651000 and 651024.

[auerswal]

Please drop this comment, I find it confusing.

The ICMPv6 payload for a normal RFC 8200 IPv6 packet could be (40+2^16-1-8) bytes, because IPv6 uses a payload length field that excludes the fixed header length of 40 bytes. The calculation in the comment does not show this clearly.

fping does not use separate payload sizes for IPv4 and IPv6 targets. Thus it cannot easily allow different sizes. It could theoretically check if it uses IPv6 (e.g., option -6 is active), but this is not done.

I'd say that this comment would be better as an issue to allow larger payloads for ICMPv6, but I definitely do not like it as a code comment.

[pevik]

#define MAX_IP_PACKET 4294967295

OK, u32. See iputils/iputils#550 (comment) that anything > 65535 produces EMSGSIZE on the Linux kernel. Is it different on FreeBSD?

FYI (it's explained in the commit message) I use IPv4 because the limit is smaller than IPv6 (fping does not print errno on failure, therefore I followed the current approach to allow smaller value.

[coveralls]

coverage: 87.676% (+0.2%) from 87.526%
when pulling 41c25eb on pevik:fix-s-max-value
into 54f452c on schweikert:develop.

[auerswal]

Thanks, I'd say that is OK. I plan on merging this in its current form in a week, unless some objection emerges.

[auerswal]

Thanks for adjusting the test according to the changed maximum size definition. That suffices, I can adjust the incorrect error message (an existing issue, not introduced by this pull request) myself.

[pevik]

Ah, I got that and fixed:

-$cmd8->stderr_is_eq("fping: data size 65508 not valid, must be lower than 65507\n");
+$cmd8->stderr_is_eq("fping: data size 65508 not valid, must not be larger than 65507\n");

[auerswal]

Thanks for adjusting the error message, too! This looks good, there is just a little nit left:
Please use -b in the commit message instead of -s, since fping uses -b for payload size.

[auerswal]

Please adjust the commit message, because it has two problems that could lead to confusion in the future:

1. The affected option is -b, not -s. This is problematic because fping has both options -b and -s, but they do totally different things. Please correct this in the first line of the commit message.
2. Current fping does print an error message when sending fails. This includes specifying a too large payload size. Please remove the incorrect statement that it does not from the commit message.

Regarding the error message with an oversized payload:

$ git diff
diff --git a/src/fping.c b/src/fping.c
index 696a017..6933eee 100644
--- a/src/fping.c
+++ b/src/fping.c
@@ -135,7 +135,7 @@ extern int h_errno;
 /*** Ping packet defines ***/
 
 #define MAX_IP_PACKET 65536 /* (theoretical) max IP packet size */
-#define SIZE_IP_HDR 40
+#define SIZE_IP_HDR 0
 #define SIZE_ICMP_HDR 8 /* from ip_icmp.h */
 #define MAX_PING_DATA (MAX_IP_PACKET - SIZE_IP_HDR - SIZE_ICMP_HDR)
 
$ ./src/fping -b 65528 127.0.0.1
127.0.0.1: error while sending ping: Message too long
127.0.0.1 is unreachable

[pevik]

1. is obviously correct. I have changed also 2., but let me explain.

2. Current fping does print an error message when sending fails. This includes specifying a too large payload size. Please remove the incorrect statement that it does not from the commit message.

...
-#define SIZE_IP_HDR 40
+#define SIZE_IP_HDR 0
 #define SIZE_ICMP_HDR 8 /* from ip_icmp.h */
 #define MAX_PING_DATA (MAX_IP_PACKET - SIZE_IP_HDR - SIZE_ICMP_HDR)
 
$ ./src/fping -b 65528 127.0.0.1
127.0.0.1: error while sending ping: Message too long
127.0.0.1 is unreachable

Try this (with allowing more size:

$ ./src/fping -c1 -b 65510 127.0.0.1; echo $?

127.0.0.1 : xmt/rcv/%loss = 1/0/100%
1

 $ ./src/fping -c1 -b 65510 ::1; echo $?
::1 : [0], 128 bytes, 0.103 ms (0.103 avg, 0% loss)

::1 : xmt/rcv/%loss = 1/1/0%, min/avg/max = 0.103/0.103/0.103
0

It just fails on IPv4 with no explanation. That is the difference with ping with iputils, where I can set the higher value (which is working for IPv6). If there were meaningful message on IPv4 on size between 65527 and 65508, I would suggest to set higher payload 65527, as I'm going to set for iputils (iputils/iputils#550).

[pevik]

The problem is nicely visible with these commands which show first working -b value. Try to run them with your diff you posted.

IPv4: first good -b 65488, before "fping: data size 65489 not valid, must be lower than 65488

i=65535 a='127.0.0.1'; while true; do
    cmd="./src/fping -c1 -b $i $a"
    echo "== $cmd =="
    $cmd && break
    i=$((i-1))
    echo
done

== ./src/fping -c1 -b 65535 127.0.0.1 ==
./src/fping: data size 65535 not valid, must not be larger than 65527
...
== ./src/fping -c1 -b 65528 127.0.0.1 ==
./src/fping: data size 65528 not valid, must not be larger than 65527

== ./src/fping -c1 -b 65527 127.0.0.1 ==

127.0.0.1 : xmt/rcv/%loss = 1/0/100%
...
== ./src/fping -c1 -b 65508 127.0.0.1 ==

127.0.0.1 : xmt/rcv/%loss = 1/0/100%

== ./src/fping -c1 -b 65507 127.0.0.1 ==
127.0.0.1 : [0], 128 bytes, 0.088 ms (0.088 avg, 0% loss)

127.0.0.1 : xmt/rcv/%loss = 1/1/0%, min/avg/max = 0.088/0.088/0.088

But it works well on IPv6, although current versioin doesn't allow the highest size.

i=65535 a='::1'; while true; do 
    cmd="./src/fping -c1 -b $i $a"
    echo "== $cmd =="
    $cmd && break
    i=$((i-1))
    echo
done

== ./src/fping -c1 -b 65535 ::1 ==
./src/fping: data size 65535 not valid, must not be larger than 65527
...
== ./src/fping -c1 -b 65528 ::1 ==
./src/fping: data size 65528 not valid, must not be larger than 65527

== ./src/fping -c1 -b 65527 ::1 ==
::1 : [0], 128 bytes, 0.129 ms (0.129 avg, 0% loss)

::1 : xmt/rcv/%loss = 1/1/0%, min/avg/max = 0.129/0.129/0.129

[auerswal]

Thanks for adjusting the commit message!

[auerswal]

Thanks, the commit message is OK now.

[auerswal]

Thanks for the explanations, I now understand the issue regarding error messages. Using one of -a, -u, -c, -C, -l, -x, and -X results in a missing error message on send failures. At a first glance this happens because the verbose_flag variable is set to 0 in this case. I concur that this makes accepting a payload size only valid for IPv6, but too large for IPv4, problematic.

[pevik]

I suppose this could not easily be fixed, thus let's ignore this possibility. Thanks for merging and your time.