Page MenuHomeFreeBSD

ping: Reference implementation
AbandonedPublic

Authored by jlduran on Feb 7 2023, 11:34 PM.
Tags
None
Referenced Files
F102430447: D38431.diff
Tue, Nov 12, 4:19 AM
F102386987: D38431.id117011.diff
Mon, Nov 11, 2:00 PM
F102385332: D38431.id116747.diff
Mon, Nov 11, 1:28 PM
F102363624: D38431.diff
Mon, Nov 11, 6:15 AM
Unknown Object (File)
Fri, Nov 8, 10:25 PM
Unknown Object (File)
Oct 5 2024, 4:59 PM
Unknown Object (File)
Oct 5 2024, 2:05 PM
Unknown Object (File)
Oct 2 2024, 7:12 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):

  1. Revert 46d7b45a267b3d78c5054b210ff7b6c55bfca42b
  2. Revert d9cacf605e2ac0f704e1ce76357cbfbe6cb63d52
  3. Ignore alignment errors (d9cacf605e2ac0f704e1ce76357cbfbe6cb63d52 was created to fix this precisely)
  4. Unskip some failing tests/fix/add some tests
  5. 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 edited the summary of this revision. (Show Details)

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.

jlduran retitled this revision from WIP ping: Reference implementation to ping: Reference implementation.
jlduran edited the summary of this revision. (Show Details)
  • 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.

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

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.

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
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.

Sorry. I realized that after I had hit submit.

This can now be abandoned. Thank you!