Date: Mon, 20 Oct 2014 16:27:42 +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 Subject: Re: svn commit: r273295 - in head/sbin: ping ping6 Message-ID: <20141020140848.D935@besplex.bde.org> In-Reply-To: <201410200027.s9K0RegN062458@svn.freebsd.org> References: <201410200027.s9K0RegN062458@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 20 Oct 2014, Hiroki Sato wrote:
> Log:
> WARNS=3 and style fixes. No functionality change.
> ...
> Modified: head/sbin/ping/ping.c
> ==============================================================================
> --- head/sbin/ping/ping.c Mon Oct 20 00:22:08 2014 (r273294)
> +++ head/sbin/ping/ping.c Mon Oct 20 00:27:40 2014 (r273295)
> ...
> @@ -1150,7 +1150,8 @@ pr_pack(char *buf, int cc, struct sockad
> #endif
> tp = (const char *)tp + phdr_len;
>
> - if (cc - ICMP_MINLEN - phdr_len >= sizeof(tv1)) {
> + if ((size_t)(cc - ICMP_MINLEN - phdr_len) >=
> + sizeof(tv1)) {
> /* Copy to avoid alignment problems: */
> memcpy(&tv32, tp, sizeof(tv32));
> tv1.tv_sec = ntohl(tv32.tv32_sec);
This breaks the warning, and breaks the code on exotic (unsupported)
where it used to work accidentally. The code was already broken on
non-exotic arches.
We returned earlier for for runt packets, but this only guarantees
that cc >= ICMP_MINLEN. When cc equals ICMP_MINLEN or is only a little
larger, the subtractions give a negative value with type int (*). On
all supported arches, size_t has the same or higher rank than int, so
C's broken "value-preserving" promotion rules apply, and the small
negative int is promoted to a huge unsigned value. This compares >=
the small size_t value, so garbage beyond the end of the packet is
used as a timestamp.
On exotic arches where size_t has lower rank than int, the small size_t
value was promoted to int, so the code worked. Casting the int breaks
this. It is a downcast to an unsigned type, so the small negative int
becomes a not so huge unsigned value. Then the promotion rules convert
both sides to int for the comparision. This is a strictly upwards
conversion, so it doesn't change the values. The LHS remains not so
huge, and the RHS remains small. Since all the values are small before
the type errors, even the not so huge value on the LHS is >= the small
value on the RHS, so again garbage beyond the end of the packet is
used as a timestamp.
The easiest fix is to cast the size_t to int instead of the reverse.
(*) That the subtractions give an int depends on the unobvious point
that ICMP_MINLEN has type int. ICMP_MINLEN is not in POSIX and is not
documented or even referenced in any man page. Fortunately it is not
defined using sizeof(), so it doesn't suffer from the promotion bugs.
It is relatively obvious that we avoided foot-shooting for the other
terms in the expression -- both cc and phdrlen have type int.
Foot-shooting is implemented for CMSG_LEN() and cmsg_len, which are
used in a similar but simpler and correct comparision nearby.
CMESG_LEN() is also not in POSIX and is not documented in any man page,
but it is used in examples in man pages. It happens to be defined
using an expression involving sizeof(), so it has type size_t except
on exotic arches. This gives the usual promotion bugs.
The examples are of the form cmsg_len = CMSG_LEN(). cmsg_len is usually
a member of struct cmsghdr. cmsg_len is in POSIX and is well documented
there if not in some man page. It has type socklen_t. socklen_t is
also in POSIX and is very well documented there if not in some man
page. POSIX allows but doesn't require the foot shooting and notes
portability problems. It notes that old BSD didn't have socklen_t and
used plain int instead. Other systems were apparently broken and used
a foot-shooting type. So POSIX allows the foot-shooting. It specifies
that socklen_t is an integer type of width at least 32 bits, and notes
that the 32nd bit is unusable in portable code. Old BSD includes 4.4BSD-
Lite2. FreeBSD broke this and implemented both the foot shooting and
incompatibility with old BSD by making socklen_t an unsigned 32-bit type
(uint32_t) as not required by POSIX. This is ABI-compatible in practice
since the 32nd bit is not useful.
socklen_t is not the same as size_t on 64-bit arches, so the type of
cmsg_len is not even bug for bug compatible with the type of CMSG_MSG()
and doesn't even guarantee the foot shooting on these arches -- it
gets promoted to (signed) int64_t in some contexts, while size_t stays
unsigned. In future arches, socklen_t might remain as 32 for ABI
compatibility (32 bits is more than enough for buffer bloat), while
plain int expands to 64 bits (except ABI compatibilty will impede that
more strongly). Then the promotion bugs will move.
> Modified: head/sbin/ping6/ping6.c
> ==============================================================================
> --- head/sbin/ping6/ping6.c Mon Oct 20 00:22:08 2014 (r273294)
> +++ head/sbin/ping6/ping6.c Mon Oct 20 00:27:40 2014 (r273295)
> @@ -205,84 +205,83 @@ u_int options;
> * to 8192 for complete accuracy...
> */
> #define MAX_DUP_CHK (8 * 8192)
> -int mx_dup_ck = MAX_DUP_CHK;
> -char rcvd_tbl[MAX_DUP_CHK / 8];
> +static int mx_dup_ck = MAX_DUP_CHK;
> +static char rcvd_tbl[MAX_DUP_CHK / 8];
>
> -struct sockaddr_in6 dst; /* who to ping6 */
> -struct sockaddr_in6 src; /* src addr of this packet */
> -socklen_t srclen;
Old code like ping uses plain int for almost everything except where
the broken ABI requires an unsigned type like socklen_t.
> -int datalen = DEFDATALEN;
> -int s; /* socket file descriptor */
> ...
> +static u_int8_t nonce[8]; /* nonce field for node information */
This uses the deprecated nonstandard spelling of uint8_t.
I also don't like use of fixed-width types when not necessary. Using
fixed-width types for networking code is technically correct and necessary,
but most networking code just uses u_char and hacks like n_long. POSIX
now requires u_char to be the same as uint8_t, so changes like this are
not necessary.
> -void pr_suptypes(struct icmp6_nodeinfo *, size_t);
The size_t foot-shooting is used a lot in prototypes. It is probably
not needed for ABI compatibilty.
> @@ -390,9 +389,9 @@ main(int argc, char *argv[])
> errno = 0;
> e = NULL;
> lsockbufsize = strtoul(optarg, &e, 10);
> - sockbufsize = lsockbufsize;
> + sockbufsize = (int)lsockbufsize;
> if (errno || !*optarg || *e ||
> - sockbufsize != lsockbufsize)
> + lsockbufsize > INT_MAX)
> errx(1, "invalid socket buffer size");
I don't like warnings about hackish but correct code like the previous
version of the above. Does the compiler actually complain about comparing
ints with unsigned longs for equality?
If you are going to change the range check, then rearrange the code. It
was arranged to set sockbufsize before the value is known to fit so that
non-fitting values can be checked. With range checking on the original
value, it is more natural to not do the assignment until after the
check has passed. Then the cast should be unnecessary since a smart
compiler would see from the range check that the assignment can't overflow.
> @@ -751,7 +750,7 @@ main(int argc, char *argv[])
> *((int *)&nonce[i]) = rand();
> #else
> memset(nonce, 0, sizeof(nonce));
> - for (i = 0; i < sizeof(nonce); i += sizeof(u_int32_t))
> + for (i = 0; i < (int)sizeof(nonce); i += sizeof(u_int32_t))
> *((u_int32_t *)&nonce[i]) = arc4random();
> #endif
> optval = 1;
Casting sizeof() limits to int instead of the other way is correct, but
it is a compiler bug to warn about simple code like the above. `i' has
a very limited scope and obviously cannot be negative.
> @@ -1009,7 +1008,7 @@ main(int argc, char *argv[])
>
> #if defined(SO_SNDBUF) && defined(SO_RCVBUF)
> if (sockbufsize) {
> - if (datalen > sockbufsize)
> + if (datalen > (size_t)sockbufsize)
datalen was broken to have the foot-shooting (its type was changed from
int to size_t). This is collateral.
In ping4, datalen still has type int.
In ping4, datalen is is initialized more or less correctly using code
like the new code above for sockbufsize but better (use strtoul(), and
range-check the value before assigning it).
In ping6, the initialization of datalen is unchanged and more broken
than before. datalen is assigned to directly from strtol(); then the
overflowed value is checked. strtol() at least matched the sign of
datalen. I'm suprised that you don't get a warning for the 'datalen
<= 0' range check now. This is now an obfuscated way of writing
'datalen == 0'. Compilers apparently only warn about 'datalen < 0'.
ping4 also has better error messages ("invalid packet size: `%s'"
instead of "illegal datalen value -- %s").
BTW, I don't like compilers warning about checks of the form
(datalen >= 0 && datalen <= MAX) or "fixing" these warnings by
removing the datalen >= 0 check. datalen might be a typedefed
type that might be signed. Careful code must check for negative
values. This has no runtime cost if type happens to be unsigned,
and no runtime cost on many arches even if it is unsigned. Removing
the check breaks this careful code.
> @@ -1445,7 +1444,7 @@ dnsdecode(const u_char **sp, const u_cha
> while (i-- > 0 && cp < ep) {
> l = snprintf(cresult, sizeof(cresult),
> isprint(*cp) ? "%c" : "\\%03o", *cp & 0xff);
> - if (l >= sizeof(cresult) || l < 0)
> + if ((size_t)l >= sizeof(cresult) || l < 0)
> return NULL;
> if (strlcat(buf, cresult, bufsiz) >= bufsiz)
> return NULL; /*result overrun*/
The conversion is backwards.
snprintf() is hard to use. Almost no code uses it as carefully as the
above. l < 0 probably can't happen in the above. But if you are willing
to cast to unsigned, then you can fold the l < 0 check into the other
check. snprintf() returns -1 on error, and casting that to size_t gives
a huge unsigned value that looks like the non-error of the bufer being
too small. "Fixing" warnings by applying size_t casts usually gives this
check unintentionally.
> @@ -1468,7 +1467,7 @@ dnsdecode(const u_char **sp, const u_cha
> * which arrive ('tis only fair). This permits multiple copies of this
> * program to be run without having intermingled output (or statistics!).
> */
> -void
> +static void
> pr_pack(u_char *buf, int cc, struct msghdr *mhdr)
> {
> #define safeputc(c) printf((isprint((c)) ? "%c" : "\\%03o"), c)
> @@ -1500,7 +1499,7 @@ pr_pack(u_char *buf, int cc, struct msgh
> }
> from = (struct sockaddr *)mhdr->msg_name;
> fromlen = mhdr->msg_namelen;
> - if (cc < sizeof(struct icmp6_hdr)) {
> + if (cc < (int)sizeof(struct icmp6_hdr)) {
> if (options & F_VERBOSE)
> warnx("packet too short (%d bytes) from %s", cc,
> pr_addr(from, fromlen));
Correct.
> @@ -2562,13 +2561,13 @@ pr_addr(struct sockaddr *addr, int addrl
> * pr_retip --
> * Dump some info on a returned (via ICMPv6) IPv6 packet.
> */
> -void
> +static void
> pr_retip(struct ip6_hdr *ip6, u_char *end)
> {
> u_char *cp = (u_char *)ip6, nh;
> int hlen;
>
> - if (end - (u_char *)ip6 < sizeof(*ip6)) {
> + if ((size_t)(end - (u_char *)ip6) < sizeof(*ip6)) {
> printf("IP6");
> goto trunc;
> }
Backwards. The RHS should be cast to ptrdiff_t. Pointer difference
are only required to work up to 64K, but that is enough for the size
of small objects.
If ip6 is somehow larger than end, the unsigned comparsion gives
foot-shooting as usual. The pointer difference is then negative;
this gets promoted to a huge unsigned value which looks like enough
space.
> @@ -2642,7 +2641,7 @@ pr_retip(struct ip6_hdr *ip6, u_char *en
> return;
> }
>
> -void
> +static void
> fill(char *bp, char *patp)
> {
> int ii, jj, kk;
> @@ -2661,7 +2660,7 @@ fill(char *bp, char *patp)
> /* xxx */
> if (ii > 0)
> for (kk = 0;
> - kk <= MAXDATALEN - (8 + sizeof(struct tv32) + ii);
> + (size_t)kk <= MAXDATALEN - 8 + sizeof(struct tv32) + ii;
> kk += ii)
> for (jj = 0; jj < ii; ++jj)
> bp[jj + kk] = pat[jj];
Backwards.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141020140848.D935>