Date: Mon, 10 Dec 2018 20:35:39 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Eugene Grosbein <eugen@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r341768 - head/sbin/ping Message-ID: <20181210184329.A1122@besplex.bde.org> In-Reply-To: <201812092111.wB9LBFec011533@repo.freebsd.org> References: <201812092111.wB9LBFec011533@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 9 Dec 2018, Eugene Grosbein wrote: > Log: > ping(8): remove needless comparision with LONG_MAX > after unsigned long ultmp changed to long ltmp in r340245. > > MFC after: 1 week A range check is needed, and is now not even attempted for large positive values. The logic for the range check was broken in r340245. The LONG_MAX was a vestige of the correct logic. Several other vestiges remain. > Modified: head/sbin/ping/ping.c > ============================================================================== > --- head/sbin/ping/ping.c Sun Dec 9 19:14:21 2018 (r341767) > +++ head/sbin/ping/ping.c Sun Dec 9 21:11:15 2018 (r341768) > @@ -313,7 +313,7 @@ main(int argc, char *const *argv) > break; > case 'c': > ltmp = strtol(optarg, &ep, 0); > - if (*ep || ep == optarg || ltmp > LONG_MAX || ltmp <=0) > + if (*ep || ep == optarg || ltmp <=0) > errx(EX_USAGE, > "invalid count of packets to transmit: `%s'", > optarg); The bugs in r340245 visible in this patch are: - lost range checking for large positive values. Before r340245, this was done hackishly by using strtoul() so that the in-bound error value ULONG_MAX was out of bounds for the range check against LONG_MAX. This range check also checked for large values which don't overflow u_long. Changing to use strtol() broke this. LONG_MAX is in-band for strtol(), any check of it alone is buggy; the inherited check of it was nonsense. - style bug: nonsense use of a temporary variable. ultmp exists and has type u_long to hold the result of of strtoul(), mainly so that the results that don't need to be larger than LONG_MAX can be checked before clobbering them by assignment to a non-temporary variable of type long. Now going through ltmp makes no difference since ltmp's type is the same as the type of the final variable and there is no reason to preserve the current value of the final variable; clobbering has already occurred in strtol() (clamp overflowing values to LONG_MAX and set errno to ERANGE to indicate the error). - style bug: no space after '<=' The main bug near here before r340245 and visible in the patch for it and fixed in r340245 is: - the hackish range checking doesn't work for negative values unless they are large enough to cause overflow as u_longs. strtoul() converts -n by evaluating n as a u_long and negating the result. So small negative values were detected as errors after becoming > LONG_MAX, but values between about -(double)ULONG_MAX and LONG_MIN wee not detected as errors after becoming <= LONG_MAX. The main bugs not near here that are not similar and are not fixed in any version are: - in all of the "packet size too large" error messages the converted value is printed (in decimal) instead of the arg (quoted). Simpler messages like the one visible in the above patch just print the arg, and they even quote the arg consistently. The arg may have leading whitespace or signs, or a non-decimal base, and then printing its converted value in decimal obfuscates it. This is the main bug used to justify r340245. After not detecting some negative values, they were misprinted as large positive values (near ULONG_MAX). Related style/quality bugs: - the simpler error message mostly just say "invalid" without distinguishing between invalid formats and invalid due to overflow or invalid due to being negative or invalid due to failing another range check. Sometimes, often just due to implementation details of the range check, some range check failures cause a differently encrypted undocumented exit code (EX_NOPERM instead of EX_USAGE) and a different error message with more details. - the "invalid" messages give no hint of the the valid range together no hint of range errors in either the message or the exit code - the man page gives no hint of the valid range for most or all numeric args, even for the "packet size too large" case where the error messages give the top end of the range (this is a fairly arbitrary limit for non-root only; most other cases have a less arbitrary limit of LONG_MAX or INT_MAX). - the special error handling for "packet size too large" is too fussy unless most error handling is done almost perfectly. The error handling is messy enough without it. The important thing about this case is that it is special for non-root, but neither the message nor the man page mentions special cases for non-root. The man page doesn't even mention valid ranges for this. Only the exit code distinguishes this case. Perfect error handling would first distingish sub-cases of EINVAL errors (signs and purer gargage), ERANGE errors, and other range check errors with more resricted ranges for non-root. Then print specific messages including specific ranges for range check errors and something about privilege for restricted cases. This would be too fussy. Unrelated style/quality bugs: - the man page also doesn't mention the better-known restriction of ping -f to root - many of these restrictions to root are bogus. E.g., for ping -f and ping -i, unprivileged users can spam the network much faster than with these options, by using plain ping in a shell loop, unless the fork() or exec() rate is severely restricted, but that would break normal use. These 2 restrictions worked better in 1980 when shell loops and fork() and exec() might have taken longer for each iteration than the ping latency of a slow network. - to fix this security hole, ping could wait for 10 msec before doing anything (in the non-root case) - ping -f doesn't actually flood. It is limited to 100 pps unless the ping is echoed faster than that. 100 pps might have been a lot in 1980. If clock interrupt latency is 10 msec or lower, then ping -f has the 100 pps limit for root too. Plain ping in a shell loop can go much faster than 100 pps, except possibly in 1980 (the case where the shell loop might not be faster enough is with a fairly low ping latency of 100 usec or smaller. That is about as long as old versions of FreeBSD take to fork()+exec() /bin/cat on 1 4GHz CPU, but current versions are several times slower). - the granularity for the wait in ping -i is undocumented (the man page says "fractional"). The code attempts to implement only milliseconds resolution but gets the kernel timeout granularity. Root can use an interval of 0. The ultmp method was the result of wollman rewriting much worse arg parsing that used atoi(). Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20181210184329.A1122>