Page MenuHomeFreeBSD

Add test cases for ping with IP options in the response
ClosedPublic

Authored by asomers on Oct 29 2022, 10:07 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Jan 27, 7:28 PM
Unknown Object (File)
Fri, Jan 13, 7:34 PM
Unknown Object (File)
Mon, Jan 9, 5:53 AM
Unknown Object (File)
Mon, Jan 9, 5:15 AM
Unknown Object (File)
Dec 25 2022, 10:50 PM
Unknown Object (File)
Dec 25 2022, 10:03 PM
Unknown Object (File)
Dec 15 2022, 5:48 PM
Unknown Object (File)
Nov 30 2022, 2:38 AM

Details

Summary

Add test cases for ping with IP options in the response

MFC after: 1 week

Test Plan

ATF tests included

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jlduran_gmail.com added inline comments.
sbin/ping/tests/Makefile
16

Missing file?

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
163

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

176

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

191

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

sbin/ping/tests/ping_test.sh
163

Yeah, it should. I'll add that.

176

What's the significance of "01010101"?

191

yes

  • Match ping's stdout in the injection tests.
  • Python style
sbin/ping/tests/ping_test.sh
176

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.

  • More specific output checking for inject_pip
sbin/ping/tests/ping_test.sh
176

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.

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.

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?

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

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 changed the visibility from "Public (No Login Required)" to "Administrators".Oct 31 2022, 4:32 PM
asomers changed the visibility from "Administrators" to "Subscribers".
asomers edited subscribers, added: secteam; removed: imp.

BTW, these tests are for the code change in D37195

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!

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

@thj already has a fix prepared.

Great! Thank you!

asomers changed the visibility from "Subscribers" to "Public (No Login Required)".Nov 30 2022, 2:00 AM

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

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

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.

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.

  • Add missing file
  • Match ping's stdout in the injection tests.
  • Python style
  • More specific output checking for inject_pip
  • fixup:
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.

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.

sbin/ping/tests/injection.py
12

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

@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?

@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
176

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 @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
157

Opening braces should be on their own line.

This revision is now accepted and ready to land.Dec 19 2022, 4:16 PM

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
176

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

This revision was landed with ongoing or failed builds.Dec 25 2022, 10:03 PM
This revision was automatically updated to reflect the committed changes.