Page MenuHomeFreeBSD

ping6: Fix alignment errors
ClosedPublic

Authored by jansucan on Aug 11 2019, 2:52 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 16, 11:42 AM
Unknown Object (File)
Sat, Mar 16, 11:42 AM
Unknown Object (File)
Sat, Mar 16, 11:42 AM
Unknown Object (File)
Sat, Mar 16, 11:42 AM
Unknown Object (File)
Sat, Mar 16, 11:30 AM
Unknown Object (File)
Jan 17 2024, 2:03 PM
Unknown Object (File)
Jan 12 2024, 5:02 AM
Unknown Object (File)
Jan 12 2024, 5:02 AM
Subscribers

Details

Summary

Fix alignment errors

This fixes -Wcast-align errors when compiled with WARNS=6.

Submitted by: Ján Sučan <sucanjan@gmail.com>
Sponsored by: Google, inc. (Google Summer of Code 2019)

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 25866
Build 24437: arc lint + arc unit

Event Timeline

sbin/ping6/ping6.c
1326–1327

Yes, it should be htons. The existing code works, though, because htons and ntohs are identical. My bigger worry is about the type of seq. Shouldn't it be short instead of int?

1389–1390

This is a bug. ping6 making two calls to gettimeofday and comparing the (usually subsecond) output to compute a duration. That will produce incorrect results if the system clock changes in the meantime. The correct way to measure durations is with clock_gettime and CLOCK_MONOTONIC. However, this bug is wholly unrelated to your task at hand, so you should fix it separately.

1390–1398

This line silently truncates tv.tv_sec down to 32 bits. It's ok given how it's used, but it looks like a bug. Better to annotate with a comment, and/or explicitly cast tv.tv_sec to a uint32_t.

sbin/ping6/ping6.c
1326–1327

Yes. It looks like the author wanted to use short when he or she used ntohs and type casting to u_int16_t. Would it be better to use short or uint16_t?

sbin/ping6/ping6.c
1326–1327

The definition of htons says uint16_t, so that would be the best.

sbin/ping6/ping6.c
1326–1327

seq variable in other parts of the code uses u_int16_t. What's the difference between u_int16_t and uint16_t? Shouldn't only one "family" of fixed-width types be used in the code?

sbin/ping6/ping6.c
1326–1327

Types like uint16_t were standardized by C99. Code that predates C99 used a plethora of nonstandard alternatives. New code should use the standard types.

sbin/ping6/ping6.c
1326–1327

So I will keep u_int16_t for the other seq variables and use uint16_t only for the single one.

sbin/ping6/ping6.c
1326–1327

Fixed in D21218 and D21219.

1389–1390

Thanks. I looked at the gettimeofday calls in ping6. There are 5 of them. Three are used for getting timeout value for select and two are used for measuring a packet trip time. I think all of them should be replaced by clock_gettime with CLOCK_MONOTONIC. I'm going to do that.

1390–1398

Fixed in D21220.

sbin/ping6/ping6.c
1389–1390

It's done D21226. But it's based on master so I will have to rebase it after D21220 is applied.

Rebase to the master branch.

I combined this revision with D21260 and ran a "make tinderbox". The build failed with GCC.
Here's with GCC 4.2

cc1: warnings being treated as errors
/home/asomers/freebsd/base/head/sbin/ping6/ping6.c: In function 'pr_nodeaddr':
/home/asomers/freebsd/base/head/sbin/ping6/ping6.c:2105: warning: 'ttl' may be used uninitialized in this function

And here's with GCC 8:

/home/asomers/freebsd/base/head/sbin/ping6/ping6.c: In function 'main':
/home/asomers/freebsd/base/head/sbin/ping6/ping6.c:963:15: warning: variable 'types' set but not used [-Wunused-but-set-variable]
   const char *types[1];
               ^~~~~
/home/asomers/freebsd/base/head/sbin/ping6/ping6.c:1171:4: warning: 'memset' used with length equal to number of elements without multiplication by element size [-Wmemset-elt-size]
    memset(cm, 0, CONTROLLEN);
    ^~~~~~
/home/asomers/freebsd/base/head/sbin/ping6/ping6.c: In function 'capdns_setup':
/home/asomers/freebsd/base/head/sbin/ping6/ping6.c:2816:6: warning: variable 'families' set but not used [-Wunused-but-set-variable]
  int families[1];
      ^~~~~~~~
/home/asomers/freebsd/base/head/sbin/ping6/ping6.c:2815:14: warning: variable 'types' set but not used [-Wunused-but-set-variable]
  const char *types[2];
              ^~~~~
At top level:
/home/asomers/freebsd/base/head/sbin/ping6/ping6.c:69:19: warning: 'copyright' defined but not used [-Wunused-const-variable=]
 static const char copyright[] =
                   ^~~~~~~~~
In file included from /home/asomers/freebsd/base/head/sys/net/if.h:47,
                 from /home/asomers/freebsd/base/head/sbin/ping6/ping6.c:111:
/home/asomers/freebsd/base/head/sbin/ping6/ping6.c: In function 'main':
/home/asomers/freebsd/base/head/sys/sys/time.h:380:35: error: 'intvl.tv_nsec' may be used uninitialized in this function [-Werror=maybe-uninitialized]
   (vsp)->tv_nsec = (tsp)->tv_nsec + (usp)->tv_nsec; \
                                   ^
/home/asomers/freebsd/base/head/sbin/ping6/ping6.c:298:24: note: 'intvl.tv_nsec' was declared here
  struct timespec last, intvl;
                        ^~~~~
In file included from /home/asomers/freebsd/base/head/sys/net/if.h:47,
                 from /home/asomers/freebsd/base/head/sbin/ping6/ping6.c:111:
/home/asomers/freebsd/base/head/sys/sys/time.h:379:33: error: 'intvl.tv_sec' may be used uninitialized in this function [-Werror=maybe-uninitialized]
   (vsp)->tv_sec = (tsp)->tv_sec + (usp)->tv_sec;  \
                                 ^
/home/asomers/freebsd/base/head/sbin/ping6/ping6.c:298:24: note: 'intvl.tv_sec' was declared here
  struct timespec last, intvl;
                        ^~~~~
cc1: all warnings being treated as errors
--- ping6.o ---
*** [ping6.o] Error code 1

The easiest way to reproduce the full failure with GCC8 (easy, but not fast), is to install the riscv64-xtoolchain-gcc package and then do env CROSS_TOOLCHAIN=riscv64-gcc TARGET=riscv TARGET_ARCH=riscv64 make -jwhatever buildworld.

This revision is now accepted and ready to land.Aug 15 2019, 7:38 PM
This revision was automatically updated to reflect the committed changes.