Add test cases for ping with IP options in the response

[asomers]

Summary

Add test cases for ping with IP options in the response

MFC after: 1 week

Test Plan

ATF tests included

[jlduran]

sbin/ping/tests/Makefile

16

Missing file?

[asomers]

• Add missing file

[jlduran]

Although fearing my suggestions could make the tests more brittle, I'm still interested to know what do you think?

sbin/ping/tests/ping_test.sh

135

I wonder if the output should also be checked:
-o match:"wrong total length" -o match:"NOP"?

148

Same here:
-o not-match:"01010101"?

163

Even here:
-o match:"1 packets transmitted, 1 packets received"?

[asomers]

sbin/ping/tests/ping_test.sh

135

Yeah, it should. I'll add that.

148

What's the significance of "01010101"?

163

yes

[asomers]

• Match ping's stdout in the injection tests.
• Python style

[jlduran]

sbin/ping/tests/ping_test.sh

148

Without this patch, the 40 01s from injection.py will show. It replies:

PING 192.0.2.15 (192.0.2.15): 56 data bytes
132 bytes from 192.0.2.15: Destination Host Unreachable
Vr HL TOS  Len   ID Flg  off TTL Pro  cks      Src      Dst
 4  f  00 007c 0001   0 0000  40  01 d84e 192.0.2.15  192.0.2.14 01010101010101010101010101010101010101010101010101010101010101010101010101010101


--- 192.0.2.15 ping statistics ---
1 packets transmitted, 0 packets received, 100.0% packet loss

After the patch,:

PING 192.0.2.15 (192.0.2.15): 56 data bytes
132 bytes from 192.0.2.15: Destination Host Unreachable
Vr HL TOS  Len   ID Flg  off TTL Pro  cks      Src      Dst
 4  f  00 007c 0001   0 0000  40  01 d84e 192.0.2.15  192.0.2.14 16110000ee8b2005171100000301fcfe000000004f00007c000100004001d84ec000020fc000020e


--- 192.0.2.15 ping statistics ---
1 packets transmitted, 0 packets received, 100.0% packet loss

Destination Host Unreachable would match either way... But I guess what matters most is the exit code.

[asomers]

• More specific output checking for inject_pip

[jlduran]

sbin/ping/tests/ping_test.sh

148

This is where I think the test might get brittle. If one runs the test suite a number of times enough to find a collision with this pattern. But I do not understand fully the idea behind it. I would have guessed testing for inner packet too short or something more trivial.

[jlduran]

I may also have a slight suspicion you applied your diff to a different ping.c base. After all, the idea was just to add test cases.

[asomers]

In D37210#845043, @jlduran_gmail.com wrote:

I may also have a slight suspicion you applied your diff to a different ping.c base. After all, the idea was just to add test cases.

Yes I did. But I can't figure out how to make the ping.c changes go away from phabricator. Can we just pretend, for the sake of review, that they aren't there?

[jlduran]

In D37210#845076, @asomers wrote:

In D37210#845043, @jlduran_gmail.com wrote:

I may also have a slight suspicion you applied your diff to a different ping.c base. After all, the idea was just to add test cases.

Yes I did. But I can't figure out how to make the ping.c changes go away from phabricator. Can we just pretend, for the sake of review, that they aren't there?

😂 that changes everything!
But in all seriousness, we may have hit a bug? in the opts case.
Here is what I used for the pip case:

atf_check -s exit:2 \
	-o match:"Destination Host Unreachable" \
	-o match:"(01){40}" \
	python3 $(atf_get_srcdir)/injection.py pip

[asomers]

In D37210#845107, @jlduran_gmail.com wrote:

In D37210#845076, @asomers wrote:

In D37210#845043, @jlduran_gmail.com wrote:

I may also have a slight suspicion you applied your diff to a different ping.c base. After all, the idea was just to add test cases.

Yes I did. But I can't figure out how to make the ping.c changes go away from phabricator. Can we just pretend, for the sake of review, that they aren't there?

😂 that changes everything!
But in all seriousness, we may have hit a bug? in the opts case.

Yes, but the bug fix is being reviewed separately. I couldn't figure out how to add my test changes to that review, so I opened a second one. Let's ignore the bug fix here.

[asomers]

BTW, these tests are for the code change in D37195

[jlduran]

Remembering the late Mike Muuss, I decided to take a stab at this problem while we wait for the cavalry to arrive.
The short version is revert d9cacf605e2ac0f704e1ce76357cbfbe6cb63d52, and start from that.
Let me know if you are interested, and I can open a Draft Pull Request on GitHub.
Without knowing the full picture, here is the full version:

1. A DROP commit that runs the tests, mandoc lint, etc.
2. Add a few tests to avoid further regressions + a test that fails (skipped).
3. Revert commit d9cacf605e2ac0f704e1ce76357cbfbe6cb63d52 + NO_WCAST_ALIGN= + remove skip on failed test.
4. A few courtesy commits to fix style, old stuff, etc.
5. A few minor bug fixes.
6. A few suggestions/improvements (can all be dropped, some of them are really proposals).
7. WIP Actual alignment fix (fix most of the alignment issues), I could use your help here! (eventually remove NO_WCAST_ALIGN=).

I plan on keep this going for ping6 (ping -6) as well, depending on your feedback.
It has been fun and while at it, I learned a few tricks I wasn't aware ping was able to do (in other OSs as well). Finally, it was a great excuse for me to learn Scapy, a fun tool to experiment with.
Thank you!

[asomers]

@jlduran_gmail.com don't get too invested; @thj already has a fix prepared.

[jlduran]

In D37210#851432, @asomers wrote:

@thj already has a fix prepared.

Great! Thank you!

[asomers]

@thj the fix is in main now. Could you please review again?

[markj]

One problem I noticed is that the tests can spuriously fail with some error messages from scapy:

markj@nuc> sudo kyua debug ping_test:inject_opts
Executing command [ python3 /usr/tests/sbin/ping/injection.py opts ]
Fail: stderr not empty
--- /dev/null   2022-11-30 14:43:29.782039000 +0000
+++ /tmp/kyua.FOws6H/2/work/check.VgUqKM/stderr 2022-11-30 14:43:27.574257000 +0000
@@ -0,0 +1,5 @@
+
+(injection.py:37468): dbind-WARNING **: 14:43:27.258: Couldn't connect to accessibility bus: Failed to connect to socket /tmp/kyua.y0IpE0/7/work/.cache/at-spi/bus_0: No such file or directory
+WARNING: No IPv4 address found on bridge0 !
+WARNING: No IPv4 address found on epair0a !
+WARNING: more No IPv4 address found on epair0b !
Files left in work directory after failure: .cache, .config, tun.txt
ping_test:inject_opts  ->  failed: atf-check failed; see the output of the test for details

[asomers]

In D37210#853886, @markj wrote:

One problem I noticed is that the tests can spuriously fail with some error messages from scapy:

markj@nuc> sudo kyua debug ping_test:inject_opts
Executing command [ python3 /usr/tests/sbin/ping/injection.py opts ]
Fail: stderr not empty
--- /dev/null   2022-11-30 14:43:29.782039000 +0000
+++ /tmp/kyua.FOws6H/2/work/check.VgUqKM/stderr 2022-11-30 14:43:27.574257000 +0000
@@ -0,0 +1,5 @@
+
+(injection.py:37468): dbind-WARNING **: 14:43:27.258: Couldn't connect to accessibility bus: Failed to connect to socket /tmp/kyua.y0IpE0/7/work/.cache/at-spi/bus_0: No such file or directory
+WARNING: No IPv4 address found on bridge0 !
+WARNING: No IPv4 address found on epair0a !
+WARNING: more No IPv4 address found on epair0b !
Files left in work directory after failure: .cache, .config, tun.txt
ping_test:inject_opts  ->  failed: atf-check failed; see the output of the test for details

I see that. It worked when I ran it in a VM, but now that I run it on bare metal with unused ethernet ports I get these warnings. The warnings come from Scapy and are harmless, but Kyua isn't expecting them. I'll try to figure out how to suppress them.

[markj]

In D37210#853891, @asomers wrote:

I'll try to figure out how to suppress them.

The sledgehammer approach would be to just add -e ignore to the atf_check parameters, but hopefully there's a better way than that.

[asomers]

• Add missing file
• Match ping's stdout in the injection tests.
• Python style
• More specific output checking for inject_pip
• fixup:

[asomers]

sbin/ping/tests/injection.py

12

I don't know how to suppress this PEP8 lint error. The imports _must_ be in this order, because Scapy prints those warning messages at import time.

[jlduran]

sbin/ping/tests/injection.py

12

I got rid of those, and pass pylint validation by just using:

import scapy.all as sc

Like the other Scapy tests from sys/netinet.

[asomers]

sbin/ping/tests/injection.py

12

That doesn't work. I still get the warning.

[asomers]

@markj @jlduran_gmail.com I fixed the network errors that markj found. A necessary side effect seems to be the pep-8 style warnings, and I don't see any way to suppress those. Are there any other problems here?

[jlduran]

In D37210#856150, @asomers wrote:

@markj @jlduran_gmail.com I fixed the network errors that markj found. A necessary side effect seems to be the pep-8 style warnings, and I don't see any way to suppress those. Are there any other problems here?

The test inject_pip_body is currently passing, however, at the moment it should be failing.

sbin/ping/tests/ping_test.sh

148

I believe the test should check for the 40 01s in the original IP header. At the moment is checking for the absence of 4 NOPs, I would guess this is wrong.

[markj]

In D37210#856150, @asomers wrote:

@markj @jlduran_gmail.com I fixed the network errors that markj found. A necessary side effect seems to be the pep-8 style warnings, and I don't see any way to suppress those. Are there any other problems here?

The network errors are gone, but I still see

markj@nuc> sudo kyua debug ping_test:inject_pip
Executing command [ python3 /usr/tests/sbin/ping/injection.py pip ]
Fail: stderr not empty
--- /dev/null   2022-12-19 15:56:11.362786000 +0000
+++ /tmp/kyua.Y6LlPJ/2/work/check.u8irfX/stderr 2022-12-19 15:56:07.798303000 +0000
@@ -0,0 +1,2 @@
+
+(injection.py:7882): dbind-WARNING **: 15:56:07.798: Couldn't connect to accessibility bus: Failed to connect to socket /tmp/kyua.y0IpE0/7/work/.cache/at-spi/bus_0: No such file or directory
Files left in work directory after failure: .cache, .config, tun.txt
ping_test:inject_pip  ->  failed: atf-check failed; see the output of the test for details

Unsetting DISPLAY in the environment seems to fix the problem but that's a weird thing to have to do.

OTOH, there are other network tests that fail for me in the same way. I didn't notice since I usually run tests in a VM. So the problem shouldn't block this patch.

sbin/ping/tests/ping_test.sh

129

Opening braces should be on their own line.

[asomers]

I can't reproduce markj's problem about dbind, whether I run on a system with or without X . I'm going to guess that it's something about his particular environment. I'll fix the braces and commit.

sbin/ping/tests/ping_test.sh

148

I think you were looking at an older version of the test? The latest one in the review does check for those 01 bytes.