Page MenuHomeFreeBSD
Differential D25622

ping(8): Improve validation of -g, -G and -h parameters.
ClosedPublic
Actions

Authored by markj on Jul 11 2020, 5:11 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 24, 3:33 AM
Unknown Object (File)
Sun, Nov 16, 2:33 AM
Unknown Object (File)
Sun, Nov 16, 12:21 AM
Unknown Object (File)
Thu, Nov 13, 5:26 PM
Unknown Object (File)
Wed, Nov 12, 11:03 PM
Unknown Object (File)
Tue, Nov 11, 1:27 PM
Unknown Object (File)
Sun, Nov 9, 11:33 PM
Unknown Object (File)
Mon, Nov 3, 10:19 PM
View All Files
Subscribers
imp
maxim
yuripv

Details

Reviewers
eugen_grosbein.net
Commits
rS367988: ping(8): Improve parameter validation
Summary
  • Check for potential overflow from strtol().
  • Make sure sweepmax isn't larger than the maximum packet size.
  • Check against sweepmax before incrementing the packet size, not after.

Inspired by PR 239978.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 32244
Build 29734: arc lint + arc unit

Event Timeline

markj requested review of this revision.Jul 11 2020, 5:11 PM
markj created this revision.
Harbormaster completed remote builds in B32244: Diff 74313.Jul 11 2020, 5:11 PM
markj added a reviewer: eugen_grosbein.net.Jul 11 2020, 5:11 PM
yuripv added a subscriber: yuripv.Jul 11 2020, 6:23 PM
yuripv added inline comments.
sbin/ping/ping.c
342

All of these would benefit of using strtonum(3) instead.

markj planned changes to this revision.Jul 11 2020, 6:41 PM
markj added inline comments.
sbin/ping/ping.c
342

Oh nice, I didn't know about that function.

maxim added a subscriber: maxim.EditedNov 4 2020, 1:49 PM

For the record, the patch also fixes 239974 and 239975.

markj updated this revision to Diff 79514.Nov 13 2020, 6:51 PM

Convert to strtonum().

Harbormaster completed remote builds in B34782: Diff 79514.Nov 13 2020, 6:51 PM
maxim added inline comments.Nov 13 2020, 8:07 PM
sbin/ping/ping.c
248

Do you really need to change the type here? All strtonum() use cases are limited by LONG_MAX or smaller values.

971–973

You don't need a whitespace before ";" here. Overall, this change looks unrelated.

markj added inline comments.Nov 13 2020, 8:18 PM
sbin/ping/ping.c
248

I don't see any reason why not though - leaving it defined as long will trigger compiler warnings, static analyzer warnings, etc..

971–973

I didn't modify that line, but I can fix it.

This is a bug related to the validation of -g/-G/-h. It could be split out in a separate diff, I don't have strong feelings either way.

markj updated this revision to Diff 79522.Nov 13 2020, 8:21 PM

Fix a nearby style bug.

Harbormaster completed remote builds in B34787: Diff 79522.Nov 13 2020, 8:21 PM
This revision was not accepted when it landed; it landed in state Needs Review.Nov 24 2020, 5:12 PM
Closed by commit rS367988: ping(8): Improve parameter validation (authored by markj). · Explain Why
This revision was automatically updated to reflect the committed changes.
markj added a commit: rS367988: ping(8): Improve parameter validation.
Herald added a subscriber: imp. · View Herald TranscriptNov 24 2020, 5:12 PM

Revision Contents
Changeset List

PathSize
sbin/
ping/
25 lines

Diff 74313

View Options

sbin/ping/ping.c

Loading...