svn commit: r273295 - in head/sbin: ping ping6

[Hiroki Sato]

Hi Bruce,

 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:

Bruce Evans <brde at optusnet.com.au> wrote
  in <20141020140848.D935 at 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)) {

br> > -struct sockaddr_in6 dst;	/* who to ping6 */
br> > -struct sockaddr_in6 src;	/* src addr of this packet */
br> > -socklen_t srclen;
br>
br> Old code like ping uses plain int for almost everything except where
br> the broken ABI requires an unsigned type like socklen_t.

-static socklen_t srclen;
-static size_t datalen = DEFDATALEN;
+static int srclen;
+static int datalen = DEFDATALEN;

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.

br> > -void	 pr_suptypes(struct icmp6_nodeinfo *, size_t);

 s/size_t/int/

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)

br> If you are going to change the range check, then rearrange the code.
br> It
br> was arranged to set sockbufsize before the value is known to fit so
br> that
br> non-fitting values can be checked.  With range checking on the
br>
br> original
br> value, it is more natural to not do the assignment until after the
br> check has passed.  Then the cast should be unnecessary since a smart
br> compiler would see from the range check that the assignment can't
br> overflow.

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

br> > -				if (l >= sizeof(cresult) || l < 0)
br> > + if ((size_t)l >= sizeof(cresult) || l < 0)
br>
br> The conversion is backwards.
br>

-                               if ((size_t)l >= sizeof(cresult) || l < 0)
+                               if (l >= (int)sizeof(cresult) || l < 0)

br> > -	if (end - (u_char *)ip6 < sizeof(*ip6)) {
br> > +	if ((size_t)(end - (u_char *)ip6) < sizeof(*ip6)) {
br> > 		printf("IP6");
br> > 		goto trunc;
br> > 	}
br>
br> Backwards.  The RHS should be cast to ptrdiff_t.  Pointer difference
br> are only required to work up to 64K, but that is enough for the size
br> of small objects.

-       if ((size_t)(end - (u_char *)ip6) < sizeof(*ip6)) {
+       if (end - (u_char *)ip6 < (ptrdiff_t)sizeof(*ip6)) {


br> > -		    kk <= MAXDATALEN - (8 + sizeof(struct tv32) + ii);
br> > + (size_t)kk <= MAXDATALEN - 8 + sizeof(struct tv32) + ii;

-                   (size_t)kk <= MAXDATALEN - 8 + sizeof(struct tv32) + ii;
+                   kk <= MAXDATALEN - 8 + (int)sizeof(struct tv32) + ii;

-- Hiroki
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ping6.c_20141020-1.diff
Type: text/x-patch
Size: 12854 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20141020/e9f91ba3/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ping.c_20141020-1.diff
Type: text/x-patch
Size: 460 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20141020/e9f91ba3/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20141020/e9f91ba3/attachment.sig>