Created attachment 227435 [details] Potential Fix Overview: When ping6 was merged into ping, the option parsing was broken. During the first pass of the options, to determine ipv4 or ipv6, only options "46" are provided to getopt(). This causes getopt() to match any '4' or '6', because it believes these are the only valid options. Below are some examples of how the first pass into getopt() is interpreted: "-c6" will match 'c' as invalid (which is ignored) and '6' as ipv6 (instead of a count of 6) "-s46" will match 's' as invalid (which is ignored), '4' as ipv4, and '6' as ipv6 (instead of a size of 46). I've provided a diff that moves the option string creation to main.h and uses that for all getopt() calls. The diff also includes the following additional fixes: - Reorder the PING6ADDOPTS options alphabetically like the PING4ADDOPTS. - Add missing space after the "[-m ttl]" option the in usage() text. - Swap the "aA" with "Aa" in the ping 6 usage() text to match PING6ADDOPTS. - Add some #ifdef IPSEC around some unused declarations. Steps to Reproduce: ping -s46 127.0.0.1 ping -s46 ::1 ping -4 -c6 127.0.0.1 ping -6 -c4 ::1 ping -4 -t6 127.0.0.1 ping -6 -t4 ::1 ping -6 -t4 ::1 ping -6 -z4 -c1 ::1 ping -s6 -c1 127.0.0.1 ping -s4 -c1 ::1 Actual Results: All the "valid" command above return the following errors. "ping: -4 and -6 cannot be used simultaneously" OR "ping: IPv6 requested but IPv4 target address provided" Expected Results: ping should not interpret '4' and '6' in the values of other options as "options". Build Date & Hardware: git HEAD @ 9f7a81b133c715f649136dcd0ad004e4180c56c9 Additional Information: none
Thanks for the bug report and the patch. One more thing, would you be able to add any regression tests to sbin/ping/tests/ping_test.sh ?
I've never looked at those regression tests. It may be faster to have someone familiar with them do it, but I'll have a look and see.
(In reply to ghuckriede from comment #2) The test suite can be a bit awkward to use but it should be pretty straightforward to start from an existing ping test case as an example. Don't need to add test cases for every combination of options (ping -4 -s6, ping -6 -s4, etc.). It's probably sufficient to run ping -4 -s6 -c1 127.0.0.1 checking that it returns 0 and run ping -46 127.0.0.1 and check that it returns 1 with the error message
Oh, I already wrote some tests. I'm just waiting on code review at the linked URL.
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=9ce201f2ee3ca340032d9cc71d91a36b3b45a4c3 commit 9ce201f2ee3ca340032d9cc71d91a36b3b45a4c3 Author: Alan Somers <asomers@FreeBSD.org> AuthorDate: 2021-10-06 22:54:59 +0000 Commit: Alan Somers <asomers@FreeBSD.org> CommitDate: 2021-10-21 00:05:43 +0000 ping: fix parsing of options including '4' and '6' ping uses a two-pass option parser. The first pass determines whether ipv4 or ipv6 is desired, and the second parses the rest of the options. But the first pass wrongly detects a '4' or '6' in an option's value as a request to use ipv6 or ipv6 respectively, for example in an invocation like "ping -c6 1.2.3.4". Fix this confusion by including all options in the first round of parsing, but ignoring those unrelated to ipv4/ipv6 selection. PR: 258048 Reported by: ghuckriede@blackberry.com Submitted by: ghuckriede@blackberry.com MFC after: 2 weeks Reviewed by: emaste Differential Revision: https://reviews.freebsd.org/D32344 sbin/ping/main.c | 14 +++++------ sbin/ping/main.h | 20 +++++++++++++++ sbin/ping/ping.c | 10 +------- sbin/ping/ping6.c | 20 ++++++--------- sbin/ping/tests/ping_test.sh | 58 +++++++++++++++++++++++++++++++++++++------- 5 files changed, 84 insertions(+), 38 deletions(-)
If you're wondering, emaste, this bug does not apply to stable/12.
*** Bug 260314 has been marked as a duplicate of this bug. ***
Thanks for this fix. Looks like the MFC for stable/13 hasn't been carried out yet and I think this also reflects the state on 13.0-RELEASE. An errata might seem a little over the top, but OTOH, the current parsing has a whole cluster of unintended side effects (see duplicated ticket). Cheers, Franco
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=c775b6ebabb3431cce9233bc6cbb66127cbb5ea0 commit c775b6ebabb3431cce9233bc6cbb66127cbb5ea0 Author: Alan Somers <asomers@FreeBSD.org> AuthorDate: 2021-10-06 22:54:59 +0000 Commit: Ed Maste <emaste@FreeBSD.org> CommitDate: 2021-12-13 01:27:12 +0000 ping: fix parsing of options including '4' and '6' ping uses a two-pass option parser. The first pass determines whether ipv4 or ipv6 is desired, and the second parses the rest of the options. But the first pass wrongly detects a '4' or '6' in an option's value as a request to use ipv6 or ipv6 respectively, for example in an invocation like "ping -c6 1.2.3.4". Fix this confusion by including all options in the first round of parsing, but ignoring those unrelated to ipv4/ipv6 selection. PR: 258048 Reported by: ghuckriede@blackberry.com Submitted by: ghuckriede@blackberry.com MFC after: 2 weeks Reviewed by: emaste Differential Revision: https://reviews.freebsd.org/D32344 (cherry picked from commit 9ce201f2ee3ca340032d9cc71d91a36b3b45a4c3) sbin/ping/main.c | 14 +++++------ sbin/ping/main.h | 20 +++++++++++++++ sbin/ping/ping.c | 10 +------- sbin/ping/ping6.c | 20 ++++++--------- sbin/ping/tests/ping_test.sh | 58 +++++++++++++++++++++++++++++++++++++------- 5 files changed, 84 insertions(+), 38 deletions(-)
Thanks a lot. :)
Fixed in main and stable/13. We can consider it for a 13.0 errata - if there's a desire for an errata someone could help by contributing content for sections 1 through 4 in the errata template: https://www.freebsd.org/security/errata-template.txt