Date: Mon, 20 Oct 2014 23:47:18 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Hiroki Sato <hrs@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, brde@optusnet.com.au Subject: Re: svn commit: r273295 - in head/sbin: ping ping6 Message-ID: <20141020204324.K913@besplex.bde.org> In-Reply-To: <20141020.162907.106891462107570268.hrs@allbsd.org> References: <201410200027.s9K0RegN062458@svn.freebsd.org> <20141020140848.D935@besplex.bde.org> <20141020.162907.106891462107570268.hrs@allbsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 20 Oct 2014, Hiroki Sato wrote:
> Thank you for your review. I fixed lines you pointed out and
> attached a patch candidate. I will show each change line by line in
> this email, too:
Thanks.
> Bruce Evans <brde@optusnet.com.au> wrote
> in <20141020140848.D935@besplex.bde.org>:
>
> br> > - if (cc - ICMP_MINLEN - phdr_len >= sizeof(tv1)) {
> br> > + if ((size_t)(cc - ICMP_MINLEN - phdr_len) >=
> br> > + sizeof(tv1)) {
> br> > /* Copy to avoid alignment problems: */
> br> > memcpy(&tv32, tp, sizeof(tv32));
> br> > tv1.tv_sec = ntohl(tv32.tv32_sec);
> br>
> br> This breaks the warning, and breaks the code on exotic (unsupported)
> br> where it used to work accidentally. The code was already broken on
> br> non-exotic arches.
>
> - if ((size_t)(cc - ICMP_MINLEN - phdr_len) >=
> - sizeof(tv1)) {
> + if (cc - ICMP_MINLEN - phdr_len >= (int)sizeof(tv1)) {
I tested this. I couldn't get it to fail, but noticed another type
error and re-noticed associated style bugs. The packet format is a
network 32+32-bit timeval, and that is what is copied, but the bounds
check is for a host timeval. On i386, network timeval == host timeval
so there is no problem. On other 32-bit systems with 32+32-bit
timevals, ntohl() fixes up the byte order if necessary. But on N-bit
systems with >64-bit timevals, ping -s 8 is broken. It should send a
32+32-bit timeval but either doesn't send it or can't receive it. Such
systems include:
- most 64-bit systems. All supported ones. Most have a 64-bit time_t,
and if they didn't then all supported ones have 64-bit longs so a
timeval would consist of a 32-bit time_t, 32 bits of padding, and a
64-bit long. ping -s 15 doesn't work on these systems, but ping -s 16
does.
- 32-bit systems with 64-bit time_t. All supported ones have only 32-bit
32-bit longs and probably have no padding, so they have 96-bit time_t
and ping -s 12 would work.
ping -s 8 worked on i386 in my simple tests because phdr_len is normally
0 (I don't know how to make it nonzero). The full packet size is then
36. This consists of hlen = 20, ICMP_MINLEN = 8, phdr_len = 0, and a
payload consisting of the network 8-byte timeval.
The style bugs here are that there is a macro TIMEVAL_LEN that is used in
some places to obfuscate the problem. I don't know much about networking
but have hacked on ping a bit and have old code with comments about this:
% diff -u2 ping.c~ ping.c
*** ping.c~ Sat Apr 10 12:10:59 2004
--- ping.c Sat Apr 10 12:11:00 2004
% @@ -93,4 +93,9 @@
%
% #define INADDR_LEN ((int)sizeof(in_addr_t))
% +/*
% + * XXX this macro used to hide the details of the packet layout, but it now
% + * just obfuscates timevals and is not even consistently used for the size
% + * of a timeval.
% + */
% #define TIMEVAL_LEN ((int)sizeof(struct timeval))
% #define MASK_LEN (ICMP_MASKLEN - ICMP_MINLEN)
The obfuscation has increased in -current. The above is broken since it
uses host timevals. This has been fixed in -current by replacing 'timeval'
by 'tv32', but that increases the obfuscation because the macro still
looks like it is for a host timeval.
Note that the macro already has a cast to int in it (presumably to fix
warnings), so changing the patch to use it will fix both problems. But
I don't like the obfuscation of that. It is good practice to use
sizeof(var) instead of sizeof(typedef_name) like the code does now.
This gives a better chance of applying sizeof() to the right variable
although that is not done now. It was done in the old version that was
broken by using host timevals. That version was consistently broken --
it didn't have tv2, and it both the sizeof and the copy were done on tv1.
Further review showed that TIMEVAL_LEN is used a lot and is clearer
than '(int)sizeof(struct tv32)' all over. Just rename it to something
like NTIMEVAL_LEN so that it is clear that it is for a network timeval.
Grepping for sizeof shows just one place where sizeof() is applied to
struct timeval. This allocates space for a message. I think this
should use struct tv32 too, but allocating more space than is needed
and sending a larger message than needed is fairly harmless. Only
6 out of 32 lines have the style bug of following sizeof by a space.
All of these also have the style bug of not parenthesizing the arg.
ping6.c doesn't have TIMEVAL, and also doesn't seem to have the bug
-- ping6 -s 8 works on freefall.
> br> > +static u_int8_t nonce[8]; /* nonce field for node information */
> br>
> br> This uses the deprecated nonstandard spelling of uint8_t.
>
> s/u_int_{8,16,32}_t/uint_{8,16,32}_t/ in various places.
I don't like changing old code like that. Makes it hard to see
important changes. You actually changed u_int8_t to u_char.
> br> > - sockbufsize = lsockbufsize;
> br> > + sockbufsize = (int)lsockbufsize;
> br> > if (errno || !*optarg || *e ||
> br> > - sockbufsize != lsockbufsize)
> br> > + lsockbufsize > INT_MAX)
> br> > errx(1, "invalid socket buffer size");
>
> br> I don't like warnings about hackish but correct code like the previous
> br> version of the above. Does the compiler actually complain about
> br> comparing
> br> ints with unsigned longs for equality?
>
> Yes, this part caused the following warning:
>
> /usr/src/head/sbin/ping6/ping6.c:395:20: error: comparison of integers of
> different signs: 'int' and 'u_long' (aka 'unsigned long')
> [-Werror,-Wsign-compare]
> sockbufsize != lsockbufsize)
ping has much better args handling than ping6. wollman changed it from
sloppy atoi()s to reasonable strtoul()s in 1997. strtoul() is easier
to use than usual because most of the args have a lower limit of 0 and
an upper limit of less than ULONG_MAX so simple range checks work.
The methods of setting errno to 0 before the call and checking it
afterwards are never needed or used. Most cases seem to be correct.
ping6 was apparently fixed separatlely, not as well, and never merged.
It uses errno or strtoul() only in the code quoted above. It mostly
uses strtol() with the base restricted to 10 instead of strtoul() with
the base unrestricted as in ping. Most cases are broken using the
technique of assigning the long result to an int variable before
checking it.
% Index: ping6.c
% ===================================================================
% --- ping6.c (revision 273295)
% +++ ping6.c (working copy)
% @@ -136,8 +137,8 @@
% #include <md5.h>
%
% struct tv32 {
% - u_int32_t tv32_sec;
% - u_int32_t tv32_usec;
% + uint32_t tv32_sec;
% + uint32_t tv32_usec;
% };
tv32 actually consists of a 32-bit time_t and a 32-bit long. The first
is signed on all supported arches. The second is always signed.
This is surely harmless. There are lots of type puns no matter how this
is declared. The host values started as signed, but are usually
nonnegative so there is no problem. Even htonl() to a host time_t or
long is a type error. It depends on 2's complement to work, but you
get that using int32_t.
% @@ -316,7 +316,7 @@
% char *policy_out = NULL;
% #endif
% double t;
% - u_long alarmtimeout;
% + u_long alarmtimeout, ultmp;
% size_t rthlen;
% #ifdef IPV6_USE_MIN_MTU
% int mflag = 0;
u_long is a bogus type for the alarm timeout. alarm() takes a u_int arg.
ping.c has this bug too. All the alarmtimeout code in ping.c was obviously
not cleaned up by Wollman in 1997, since has a different style and lower
quality. u_long is used here mainly to assign the result of alarmtimeout
directly to it, instead of using ultmp like almost everywhere else. Other
style bugs include using excessive parentheses not checking for null
operands in the usual way (this is only a style bug since null operands
give an out-of-range operand of 0) and a bogus ULONG_MAX check (this is
only a style bug since ULONG_MAX fails a correct range check later). Other
bugs include not checking for garbage after the operand. Implementing these
bugs takes about 50% more code (mainly to duplicate the err() message).
That was for the arg parsing of alarmtimeout. Then when it is used, it
is bogusly cast to int. The prototype would convert it to the correct
type, u_int. Compilers should warn about either or none of the the
explicit conversion to int or the prototype's conversion to u_int,
depending on the warning level and whether thay can see that the range
checking ensures that that the value always fits in an int.
% @@ -388,11 +388,10 @@
% #if defined(SO_SNDBUF) && defined(SO_RCVBUF)
% errno = 0;
This can probably be removed too.
% e = NULL;
This was nonsense and can certainly be removed. The endptr parameter in
the strtol() family is output-only.
% - lsockbufsize = strtoul(optarg, &e, 10);
% - sockbufsize = (int)lsockbufsize;
% - if (errno || !*optarg || *e ||
% - lsockbufsize > INT_MAX)
% + ultmp = strtoul(optarg, &e, 10);
% + if (errno || !*optarg || *e || ultmp > INT_MAX)
% errx(1, "invalid socket buffer size");
% + sockbufsize = ultmp;
Compare with 1997 ping code. It is similar after removing errno. Other
differences:
- ping spells the endptr variable ep
- ping doesn't limit the base to 10
- ping orders the test with *ep first
- both use the boolean test for the non-boolean *ep, but only ping6 uses
it for !*optarg. Anyway, the !*optarg test is broken. It only detects
empty strings, while the correct test ep == optarg that is used by ping
detects other garbage like strings consisting of only leading whitespace
(maybe followed by a sign).
% #else
% errx(1,
% "-b option ignored: SO_SNDBUF/SO_RCVBUF socket options not supported");
% @@ -522,14 +521,15 @@
% options |= F_SRCADDR;
% break;
% case 's': /* size of packet to send */
% - datalen = strtol(optarg, &e, 10);
% - if (datalen <= 0 || *optarg == '\0' || *e != '\0')
% - errx(1, "illegal datalen value -- %s", optarg);
% - if (datalen > MAXDATALEN) {
% - errx(1,
% - "datalen value too large, maximum is %d",
% + ultmp = strtoul(optarg, &e, 10);
% + if (*e || e == optarg)
% + errx(EX_USAGE, "invalid packet size: `%s'",
% + optarg);
% + if (ultmp > MAXDATALEN)
% + errx(EX_USAGE,
% + "packetsize too large, maximum is %d",
% MAXDATALEN);
% - }
% + datalen = ultmp;
% break;
Here you seem to have mostly copied ping. ping requires privileges for
sizes larger than DEFDATALEN. In the privileged case it is broken by not
having a bounds check. In the unprivileged case, its error is EPERM,
while the error is always EX_USAGE in the above.
I don't like the restriction for ping. Is inet6 so much different that
it doesn't need the restriction? I think inet4 never needed it after
about 1990 in the US and a bit later here when anyone could afford a
system for spamming the internet. Anyone could exercise the winping
bug, but such bugs should have been fixed by now.
The separate context-dependent messages in the above and in ping are
not very useful. The first one is often incorrect. It says that the
packet size is invalid, but the size has not been checked yet. Only
the format of the arg has been checked. The second message is missing
a space in "packet size" and has a grammar error (comman splice) in
the above only. ping.c's second message also prints the value that
is too large (this is more than echoing the arg, since it removes
leading spaces and converts -1 to a huge u_long value).
I wouldn't try hard to generate correct context-sensitive messages for
all types of errors from strto*().
% @@ -718,7 +718,7 @@
% errx(1, "-f and -i incompatible options");
%
% if ((options & F_NOUSERDATA) == 0) {
% - if (datalen >= sizeof(struct tv32)) {
% + if (datalen >= (int)sizeof(struct tv32)) {
Another use for TIMEVAL_LEN.
% @@ -1419,7 +1419,7 @@
% while (cp < ep) {
% i = *cp;
% if (i == 0 || cp != *sp) {
% - if (strlcat((char *)buf, ".", bufsiz) >= bufsiz)
% + if ((int)strlcat((char *)buf, ".", bufsiz) >= bufsiz)
% return NULL; /*result overrun*/
% }
% if (i == 0)
I don't like strlcat() and haven't thought about code like this much. It
returns size_t where snprintf() returns int. This causes them to have
the opposite sign extension and possibly representability problems. I
think the case is safe because all possible return values are small.
% @@ -1812,7 +1813,7 @@
% * Bounds checking on the ancillary data buffer:
% * subtract the size of a cmsg structure from the buffer size.
% */
% - if (bufsize < (extlen + CMSG_SPACE(0))) {
% + if (bufsize < (int)(extlen + CMSG_SPACE(0))) {
% extlen = bufsize - CMSG_SPACE(0);
% warnx("options truncated, showing only %u (total=%u)",
% (unsigned int)(extlen / 8 - 1),
Oops, it wasn't such a good idea to change bufsize from size_t. Now
it interacts badly with strlcat() and CMSG*().
% @@ -1835,7 +1836,7 @@
% offset = inet6_opt_get_val(databuf, offset,
% &value4, sizeof(value4));
% printf(" Jumbo Payload Opt: Length %u\n",
% - (u_int32_t)ntohl(value4));
% + (uint32_t)ntohl(value4));
This case is bogus. ntohl() already returns uint32_t, so the cast has
no effect. However it is only accidental that uint32_t matches the
format arg. The correct cast is to u_int, but it is painful to fix
all the print formats that have this bug (similarly for signed int).
% @@ -2027,7 +2028,7 @@
%
% for (off = 0; off < clen; off += sizeof(v)) {
% memcpy(&v, cp + off, sizeof(v));
% - v = (u_int32_t)ntohl(v);
% + v = (uint32_t)ntohl(v);
This bogus cast has no effect at all. ntohl() returning uint32_t has
been in POSIX since at least 2001, and BSD always assumed 32-bit longs
so it didn't need this cast before 2001.
% @@ -2072,16 +2073,16 @@
% * by the length of the data, but note that the detection algorithm
% * is incomplete. We assume the latest draft by default.
% */
% - if (nilen % (sizeof(u_int32_t) + sizeof(struct in6_addr)) == 0)
% + if (nilen % (sizeof(uint32_t) + sizeof(struct in6_addr)) == 0)
% withttl = 1;
% while (nilen > 0) {
% - u_int32_t ttl;
% + uint32_t ttl;
%
% if (withttl) {
% /* XXX: alignment? */
% - ttl = (u_int32_t)ntohl(*(u_int32_t *)cp);
% - cp += sizeof(u_int32_t);
% - nilen -= sizeof(u_int32_t);
% + ttl = (uint32_t)ntohl(*(uint32_t *)cp);
% + cp += sizeof(uint32_t);
% + nilen -= sizeof(uint32_t);
Lots more bogus casts. Why spell 4 as sizeof(uint32_t)? Spelling it as
sizeof(var), where var is the uint32_t variable copied to would be a more
useful spelling.
% @@ -2527,7 +2528,7 @@
%
% printf("Vr TC Flow Plen Nxt Hlim\n");
% printf(" %1x %02x %05x %04x %02x %02x\n",
% - (ip6->ip6_vfc & IPV6_VERSION_MASK) >> 4, tc, (u_int32_t)ntohl(flow),
% + (ip6->ip6_vfc & IPV6_VERSION_MASK) >> 4, tc, (uint32_t)ntohl(flow),
% ntohs(ip6->ip6_plen), ip6->ip6_nxt, ip6->ip6_hlim);
% if (!inet_ntop(AF_INET6, &ip6->ip6_src, ntop_buf, sizeof(ntop_buf)))
% strlcpy(ntop_buf, "?", sizeof(ntop_buf));
Looks like most of the bogus casts were from a time when ntohl() returned
long on some systems, but instead of fixing the format to match the type,
the type was broken to match the format.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141020204324.K913>