svn commit: r366207 - head/lib/libc/gen

Konstantin Belousov kostikbel at gmail.com
Mon Sep 28 15:04:14 UTC 2020


On Sun, Sep 27, 2020 at 10:08:49PM -0600, Alan Somers wrote:
> On Sun, Sep 27, 2020 at 5:15 PM Konstantin Belousov <kostikbel at gmail.com>
> wrote:
> 
> > On Sun, Sep 27, 2020 at 10:26:41PM +0000, Alan Somers wrote:
> > > Author: asomers
> > > Date: Sun Sep 27 22:26:41 2020
> > > New Revision: 366207
> > > URL: https://svnweb.freebsd.org/changeset/base/366207
> > >
> > > Log:
> > >   Misc compiler warning fixes in lib/libc
> > >
> > >   Reviewed by:        kevans, imp
> > >   MFC after:  2 weeks
> > >   Differential Revision:      https://reviews.freebsd.org/D26534
> > >
> > > Modified:
> > >   head/lib/libc/gen/auxv.c
> > >   head/lib/libc/gen/basename_compat.c
> > >   head/lib/libc/gen/crypt.c
> > >   head/lib/libc/gen/dirname_compat.c
> > >   head/lib/libc/gen/fts-compat.c
> > >   head/lib/libc/gen/ftw-compat11.c
> > >   head/lib/libc/gen/getentropy.c
> > >
> > > Modified: head/lib/libc/gen/auxv.c
> > >
> > ==============================================================================
> > > --- head/lib/libc/gen/auxv.c  Sun Sep 27 21:43:19 2020        (r366206)
> > > +++ head/lib/libc/gen/auxv.c  Sun Sep 27 22:26:41 2020        (r366207)
> > > @@ -67,7 +67,8 @@ __init_elf_aux_vector(void)
> > >  }
> > >
> > >  static pthread_once_t aux_once = PTHREAD_ONCE_INIT;
> > > -static int pagesize, osreldate, canary_len, ncpus, pagesizes_len,
> > bsdflags;
> > > +static int pagesize, osreldate, ncpus, bsdflags;
> > > +static size_t canary_len, pagesizes_len;
> > >  static int hwcap_present, hwcap2_present;
> > >  static char *canary, *pagesizes, *execpath;
> > >  static void *ps_strings, *timekeep;
> > > @@ -245,16 +246,21 @@ int
> > >  _elf_aux_info(int aux, void *buf, int buflen)
> > >  {
> > >       int res;
> > > +     size_t buflen_;
> > >
> > >       __init_elf_aux_vector();
> > >       if (__elf_aux_vector == NULL)
> > >               return (ENOSYS);
> > >       _once(&aux_once, init_aux);
> > >
> > > +     if (buflen < 0)
> > > +             return (EINVAL);
> > > +     buflen_ = (size_t)buflen;
> > > +
> > >       switch (aux) {
> > >       case AT_CANARY:
> > > -             if (canary != NULL && canary_len >= buflen) {
> > > -                     memcpy(buf, canary, buflen);
> > > +             if (canary != NULL && canary_len >= buflen_) {
> > > +                     memcpy(buf, canary, buflen_);
> > >                       memset(canary, 0, canary_len);
> > >                       canary = NULL;
> > >                       res = 0;
> > > @@ -267,35 +273,35 @@ _elf_aux_info(int aux, void *buf, int buflen)
> > >               else if (buf == NULL)
> > >                       res = EINVAL;
> > >               else {
> > > -                     if (strlcpy(buf, execpath, buflen) >= buflen)
> > > +                     if (strlcpy(buf, execpath, buflen_) >= buflen_)
> > >                               res = EINVAL;
> > >                       else
> > >                               res = 0;
> > >               }
> > >               break;
> > >       case AT_HWCAP:
> > > -             if (hwcap_present && buflen == sizeof(u_long)) {
> > > +             if (hwcap_present && buflen_ == sizeof(u_long)) {
> > >                       *(u_long *)buf = hwcap;
> > >                       res = 0;
> > >               } else
> > >                       res = ENOENT;
> > >               break;
> > >       case AT_HWCAP2:
> > > -             if (hwcap2_present && buflen == sizeof(u_long)) {
> > > +             if (hwcap2_present && buflen_ == sizeof(u_long)) {
> > >                       *(u_long *)buf = hwcap2;
> > >                       res = 0;
> > >               } else
> > >                       res = ENOENT;
> > >               break;
> > >       case AT_PAGESIZES:
> > > -             if (pagesizes != NULL && pagesizes_len >= buflen) {
> > > -                     memcpy(buf, pagesizes, buflen);
> > > +             if (pagesizes != NULL && pagesizes_len >= buflen_) {
> > > +                     memcpy(buf, pagesizes, buflen_);
> > >                       res = 0;
> > >               } else
> > >                       res = ENOENT;
> > >               break;
> > >       case AT_PAGESZ:
> > > -             if (buflen == sizeof(int)) {
> > > +             if (buflen_ == sizeof(int)) {
> > >                       if (pagesize != 0) {
> > >                               *(int *)buf = pagesize;
> > >                               res = 0;
> > > @@ -305,7 +311,7 @@ _elf_aux_info(int aux, void *buf, int buflen)
> > >                       res = EINVAL;
> > >               break;
> > >       case AT_OSRELDATE:
> > > -             if (buflen == sizeof(int)) {
> > > +             if (buflen_ == sizeof(int)) {
> > >                       if (osreldate != 0) {
> > >                               *(int *)buf = osreldate;
> > >                               res = 0;
> > > @@ -315,7 +321,7 @@ _elf_aux_info(int aux, void *buf, int buflen)
> > >                       res = EINVAL;
> > >               break;
> > >       case AT_NCPUS:
> > > -             if (buflen == sizeof(int)) {
> > > +             if (buflen_ == sizeof(int)) {
> > >                       if (ncpus != 0) {
> > >                               *(int *)buf = ncpus;
> > >                               res = 0;
> > > @@ -325,7 +331,7 @@ _elf_aux_info(int aux, void *buf, int buflen)
> > >                       res = EINVAL;
> > >               break;
> > >       case AT_TIMEKEEP:
> > > -             if (buflen == sizeof(void *)) {
> > > +             if (buflen_ == sizeof(void *)) {
> > >                       if (timekeep != NULL) {
> > >                               *(void **)buf = timekeep;
> > >                               res = 0;
> > > @@ -335,14 +341,14 @@ _elf_aux_info(int aux, void *buf, int buflen)
> > >                       res = EINVAL;
> > >               break;
> > >       case AT_BSDFLAGS:
> > > -             if (buflen == sizeof(int)) {
> > > +             if (buflen_ == sizeof(int)) {
> > >                       *(int *)buf = bsdflags;
> > >                       res = 0;
> > >               } else
> > >                       res = EINVAL;
> > >               break;
> > >       case AT_PS_STRINGS:
> > > -             if (buflen == sizeof(void *)) {
> > > +             if (buflen_ == sizeof(void *)) {
> > >                       if (ps_strings != NULL) {
> > >                               *(void **)buf = ps_strings;
> > >                               res = 0;
> >
> > This is significant uglification of the code in the name of fixing
> > pointless
> > warnings.
> >
> 
> Warnings are NOT pointless.  99% of them are.  But the only way to find the
> 1% that aren't is to quell the 99% that are.  Last week I wrote a bug that
> fortunately got caught in code review.  But it shouldn't have made it that
> far.  It should've been caught by the compiler, but libc is only built with
> WARNS=2.  This commit is one step towards raising libc's WARNs level to 3.
> Only 334 files left to go.
I specifically replied to the part of the change that I quoted.  I did not
discussed generic arguments WRT handling some warnings.

> 
> 
> >
> > I suspect that you tried to shut down warning about comparision of integers
> > of different size, int vs. sizeof() which has size_t result.  Is there
> > anything else ?
> >
> 
> Mostly the warnings were about comparisons of integers with different
> signedness.  Also there were some warnings about missing prototypes.
In this fragment ?  I think no, so only signess mismatch.

> 
> 
> >
> > All these values should be small integers, which is quite vividly
> > illustrated by comparision with sizeof() of built-in types.  If compiler
> > cannot deduce that itself the warning should be forcibly disabled by
> > flag instead of doing 'buflen_'.
> >
> > And canary_len/pagesizes_len do not need to take 8 bytes, their values
> > never become greater than one hundred.
> >
> 
> Better to give it enough space that the compiler can check it statically,
> rather than require runtime checks.
Natural and proper type for small numerical values in C is int. Both
external interface for the API, and internal implementation are built
around this. Making internal details mismatch the external view on the
interface make it much more risky then leaving warning that is mostly
due to the compiler deficiency.

Using size_t is not appropriate there. I prefer this warning to be shut
down for the file with the compiler switch. If you are so inclined to
shut it in the source code, please either change the type of unneeded
variable to unsigned int (and also perhaps applying ScottL suggestion),
or just do casts to int in comparisions.


More information about the svn-src-head mailing list