ping: Reference implementation
AbandonedPublic
Actions

Authored by jlduran_gmail.com on Feb 7 2023, 11:34 PM.

Details

Reviewers

asomers
markj

Group Reviewers

network

Summary

This is unlikely to be committed, as it reverts an SA and code already on -RELEASE branches.
It serves, however, the purpose of a reference implementation.
The current code corresponds to the following commits (https://github.com/jlduran/freebsd-src/compare/base...jlduran:freebsd-src:ping-reference-implementation):

Revert 46d7b45a267b3d78c5054b210ff7b6c55bfca42b
Revert d9cacf605e2ac0f704e1ce76357cbfbe6cb63d52
Ignore alignment errors (d9cacf605e2ac0f704e1ce76357cbfbe6cb63d52 was created to fix this precisely)
Unskip some failing tests/fix/add some tests
Remove #ifndef icmp_data guards (these vestigial guards were only needed on <= 4.2BSD)

Do not get too invested, as this revision will never land.

Test Plan

Start submitting other revisions that fix (but not optimize) the current code, while keeping this one for comparison.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

jlduran_gmail.com created this revision.Feb 7 2023, 11:34 PM

Herald added a subscriber: imp. · View Herald TranscriptFeb 7 2023, 11:34 PM

jlduran_gmail.com requested review of this revision.Feb 7 2023, 11:34 PM

jlduran_gmail.com added a parent revision: D38053: ping: Add ATF-Python tests.Feb 8 2023, 12:12 AM

Add tests for TCP/UDP unreachable packets:
Before tcpdump existed, ping provided some sort of packet sniffing capabilities. After ef9e6dc7eebe9830511602904d3ef5218d964080 this functionality was almost removed, to only display packets sent by us.
The TCP/UDP code remained, I do not know how to trigger it naturally, by doctoring the packet, however, we are able to test it.

Remove WIP commit (it serves no purpose)
Add tests to pr_pack() with options

The function pr_pack() prints out a packet. If the IP packet contains options, these are printed as well.
Commit 46d7b45a267b3d78c5054b210ff7b6c55bfca42b introduced an integer overflow bug, by changing hlen from int to uint8_t; while the fix is simple, let's test everything to avoid introducing regressions.

jlduran_gmail.com mentioned this in D38469: ping: Print the IP options of the original packet.Feb 9 2023, 7:47 PM

I don't understand why you need this revision. For what will it serve as a reference implementation?

In D38431#875438, @asomers wrote:

I don't understand why you need this revision. For what will it serve as a reference implementation?

As a reference for myself and the other reviewers that may not be acquainted with the output. Also, in order to compare it with the future revisions that attempt to fix this same issue, the first one being D38469. I'll relate the remaining ones to this revision for tracking.
Once the others have been reviewed/accepted, this revision will be abandoned.

jlduran_gmail.com mentioned this in D38470: ping tests: Add tests for IP header options.Feb 9 2023, 9:18 PM

jlduran_gmail.com mentioned this in D38475: ping: Remove vestigial code.Feb 9 2023, 11:16 PM

Update the "reference" implementation with the same tests added to the proposed fixes:

Add max inner packet IHL without payload test
Add IHL too short test
Add quoted data too short test
Add inner IHL too short test
Add inner packet too short test

jlduran_gmail.com mentioned this in D38593: ping: Fix an unsigned integer overflow (D38470 alternate take).Feb 14 2023, 6:41 PM

cy added a subscriber: cy.Feb 23 2023, 2:32 PM

See https://reviews.freebsd.org/D38744 for a simpler alternative.

In D38431#881732, @cy wrote:

See https://reviews.freebsd.org/D38744 for a simpler alternative.

This is my reference implementation (prior to both commits noted in the description), no only it fixes, as you well suggest, D38744, but also others (e.g., D38469), It is not intended to be committed.

In D38431#881769, @jlduran_gmail.com wrote:

In D38431#881732, @cy wrote:

See https://reviews.freebsd.org/D38744 for a simpler alternative.

This is my reference implementation (prior to both commits noted in the description), no only it fixes, as you well suggest, D38744, but also others (e.g., D38469), It is not intended to be committed.