svn commit: r366207 - head/lib/libc/gen
Kyle Evans
kevans at freebsd.org
Mon Sep 28 15:07:10 UTC 2020
On Mon, Sep 28, 2020 at 10:04 AM Konstantin Belousov
<kostikbel at gmail.com> wrote:
>
> 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.
I would be tempted to just revert the rest of the local modifications
(sans negative check, maybe) and widen it in the one spot that the
compiler complains about:
- if (strlcpy(buf, execpath, buflen_) >= buflen_)
+ if (strlcpy(buf, execpath, buflen) >= (size_t)buflen)
I had expressed this in the review, but with no particular conviction.
Thanks,
Kyle Evans
More information about the svn-src-all
mailing list