ping tests: Add tests for IP header options

[jlduran]

Summary

The function pr_pack() prints out a packet, if the IP packet contains options, these are printed as well.

Test the functionality fixed in 70960bb86a3ba5b6f5c4652e613e6313a7ed1ac1.

Test Plan

DISCLAIMER: My preferred route would be to implement D38431, however for reasons detailed in that review, we'll resort to fixing what we currently have.

[jlduran]

@markj already made a few suggestions by email. Let's move the discussion here.

[markj]

sbin/ping/ping.c

1381 ↗

(On Diff #116888)

This (int) cast also isn't needed.

1455 ↗

(On Diff #116888)

I think we need to adjust hlen here and in the default case to avoid an infinite loop.

[jlduran]

sbin/ping/ping.c

1455 ↗

(On Diff #116888)

Nice! will add a test case for NOP as well.

[jlduran]

Address comments:

• Add single NOP case
• Remove unneeded int cast
• There is no need to decrease hlen, as cp is advancing to the next octet, (single NOP case added as proof). However, let's be pedantic (and true to the original implementation), and decrease it.

[jlduran]

sbin/ping/ping.c

1391 ↗

(On Diff #117013)

I believe, to really stay true to the original implementation, we should also decrease hlen by 1 accordingly, here and in the other routing options. However, I'd like to avoid magic numbers, but I’m afraid the constants from ip.h are meant to be used as offsets (https://github.com/freebsd/freebsd-src/blob/6957cd86d9143f5ff179515e52fc42946e642f66/sys/netinet/ip.h#L162..L168)… We’ll see…

[markj]

So isn't there now a mismatch between hlen and cp when handling IPOPT_LSRR and other options? The loop header increments cp once each iteration but doesn't decrement hlen to match.

Perhaps this suggests that the better solution is to handle IPOPT_EOL by forcibly breaking out of the loop, rather than setting hlen to zero?

[jlduran]

In D38470#877133, @markj wrote:

So isn't there now a mismatch between hlen and cp when handling IPOPT_LSRR and other options? The loop header increments cp once each iteration but doesn't decrement hlen to match.

I'll test locally with a packet containing LSRR options with just a couple of routers + NOP and see the remaining hlen value.

Perhaps this suggests that the better solution is to handle IPOPT_EOL by forcibly breaking out of the loop, rather than setting hlen to zero?

I believe it is doing both.

If it helps, I have somewhere another branch where I declare an int optslen var, which is hlen - sizeof(struct ip), and use it inside the for loop insted of hlen, kind of like it was before.

Why the original authors decrease hlen, I haven't investigated, but there's probably a historical reason. With this patch hlen ends being either 0 or 20 depending on whether the original IP packet had options or not.

[jlduran]

@markj I've created D38593 as an exercise.

[cy]

Wouldn't it be simpler to simply revert hlen back to a signed int?

[jlduran]

Now that the fix is in place, just commit the tests.