Re: Errors from cppcheck

[OndÅej BÃlka <neleai at seznam dot cz>]

On Mon, Aug 26, 2013 at 11:25:27AM -0700, Brooks Moses wrote:
> On 08/24/2013 02:41 AM, OndÅej BÃlka wrote:
> >Hi, I ran a cppcheck to find latent bugs. Its output is quite large,
> >some are false positives (floats are reported because if's used were not
> >exhaustive...)
> 
> As noted yesterday, I've gone through these to pull out the real
> bugs from the rather large quantities of noise.  I'll reorder your
> list to group things.
> 
Thanks for doing this.

> Meanwhile, what version of cppcheck did you use?  Many of the false
> positives look like cppcheck bugs that should be reported to that
> project.
>
A debian one. A cppcheck --version gives Cppcheck 1.61

I posted link on cppcheck forum. To save them some time current sources are:

git clone git://sourceware.org/git/glibc.git 

I deleted duplicate entries for most false positives and reorder it a
bit.

> 
> ======================================================================
> 
> Then, some things that may or may not be real bugs, depending on
> assumptions about arguments and values that may or may not actually
> be completely valid.  My guess is that few if any of these are real
> bugs, but they are probably cases where some documentation of
> assumptions would be useful -- and where we have extra conditional
> checks that should be always true, so we should either remove them
> to help the compiler, or add "reached 'unreachable' code" errors.
> 
> I have not filed bugs about these yet.
> 
> ----------------------------------------------------------------------
> 
> Things that depend on run-time assumptions:
> 
> >vim ports/sysdeps/m68k/backtrace.c +129 # (error) Uninitialized struct member: arg.lastfp
> Needs evaluation: This applies if __backtrace is called with size=0.
> Do we guarantee that can't happen?
> 
> >vim stdio-common/printf_fphex.c +416 # (error) Uninitialized variable: exponent
> Needs evaluation: This fails if the value of âinfo->is_long_double
> == 0 || sizeof (double) == sizeof (long double)â is false and
> PRINT_FPHEX_LONG_DOUBLE is not defined.  Assume this never happens?
> 
> >vim sysdeps/ieee754/dbl-64/mpa.c +183 # (error) Uninitialized variable: c
> Needs evaluation: Depends on being called with p>=1, which depends
> on __mp_dbl being called with p>=1.

For this we could add assert or optimize code bit to
if (p < 5)
    {
      if (p == 1)
        c = X[1];
      else if (p == 2)
        c = X[1] + R * X[2];
      else if (p == 3)
        c = X[1] + R * (X[2] + R * X[3]);
      else /* if (p == 4) */
        c = (X[1] + R * X[2]) + R * R * (X[3] + R * X[4]);
    }
> 
> >vim sysdeps/ieee754/ldbl-128ibm/s_nearbyintl.c +113 # (error) Uninitialized variable: high
> Needs evaluation: We only enter this clause if u.dd[1] !=0, and low
> and high are defined if u.dd[1]>0 or u.dd[1]<0.  This only fails if
> u.dd[1] is a NaN.  Presumably that is not allowed by long-double
> spec?
> 
> >vim sysdeps/ieee754/dbl-64/e_j0.c +306 # (error) Uninitialized variable: p
> Needs evaluation: Depends on GET_HIGH_WORD always returning a value
> greater than or equal to 0x4000000.
>
Same case as mpa in following floats
We could add 'else impossible()' or write 'else /* if (...) */'

By the way why is following fragment there?

#ifdef DO_NOT_USE_THIS
        r = p[0]+z*(p[1]+z*(p[2]+z*(p[3]+z*(p[4]+z*p[5]))));
        s = one+z*(q[0]+z*(q[1]+z*(q[2]+z*(q[3]+z*q[4]))));
#else
 
> >vim sysdeps/ieee754/dbl-64/e_j1.c +304 # (error) Uninitialized variable: p
> Needs evaluation: Depends on GET_HIGH_WORD always returning a value
> greater than or equal to 0x4000000.
> 
> >vim sysdeps/ieee754/flt-32/e_j0f.c +236 # (error) Uninitialized variable: p
> Needs evaluation: Depends on GET_HIGH_WORD always returning a value
> greater than or equal to 0x4000000.
> 
> >vim sysdeps/ieee754/flt-32/e_j1f.c +233 # (error) Uninitialized variable: p
> Needs evaluation: Depends on GET_HIGH_WORD always returning a value
> greater than or equal to 0x4000000.
> 
> >vim sysdeps/ieee754/ldbl-96/e_j0l.c +385 # (error) Uninitialized variable: p
> Needs evaluation: Depends on GET_LDOUBLE_WORDS always returning a
> value greater than or equal to 0x4000.
> 
> >vim sysdeps/ieee754/ldbl-96/e_j1l.c +383 # (error) Uninitialized variable: p
> Needs evaluation: Depends on GET_LDOUBLE_WORDS always returning a
> value greater than or equal to 0x4000.
> 
> ----------------------------------------------------------------------
> 
> Likewise, some things that depend on interactions between macros.  I
> presume these are false positives, but it would be good for someone
> who knows the code to confirm:
> 
> >vim nscd/nscd_helper.c +186 # (error) Uninitialized variable: sock
> Needs evaluation: This is uninitialized if __ASSUME_SOCK_CLOEXEC is
> true and SOCK_CLOEXEC is false, but the macro names imply that wonât
> happen.
> 
Needs refactoring as macro conditions are quite confusing.

> >vim sysdeps/unix/sysv/linux/futimes.c +107 # (error) Uninitialized variable: result
> >vim sysdeps/unix/sysv/linux/opensock.c +84 # (error) Uninitialized variable: result
> Needs evaluation: Relies on __ASSUME_UTIMES not being defined if
> __NR_utimes is not defined.
> 
Here we could check if these conditions are neede. Minimal supported
kernel version is  2.6.16 and there is following comment:

   Starting with 2.6.22 the Linux kernel has the utimensat syscall which
   can be used to implement futimes.  Earlier kernels have no futimes()
   syscall so we use the /proc filesystem.  */


> 
> False positives and probable false positives because of
> overly-aggressive assumptions about the arguments to realloc:
>
This is real, you need to know about following obscure case (from manpage):

to free() is returned.  If realloc() fails the original block is left
untouched; it is not freed or moved.

 
> >vim malloc/mcheck.c +320 # (error) Common realloc mistake: 'hdr' nulled but not freed upon failure
> False positive(?): âhdrâ does not appear to point to allocated
> memory that needs to be freed.
> 
> >vim misc/err.c +64 # (error) Common realloc mistake: 'wformat' nulled but not freed upon failure
> False positive: Input wformat is allocaâed and does not need to be freed.
>
Passing alloca'ed pointer to realloc is bug so it is nulled before.


> ----------------------------------------------------------------------
> 
> Last of the maybe-bugs: A block of code that left me totally confused:
> 
> >vim sysdeps/unix/sysv/linux/powerpc/aix/tcgetattr.c +58 # (error) Uninitialized struct member: aixtermios.c_iflag
> Needs evaluation: I donât understand what is going on with this code
> at all.  It starts with "result = /* make syscall */;" which doesn't
> even look like valid C.
>
git blame shows
fda4deac (Ulrich Drepper 2000-05-05 20:48:16 +0000
on everything except license changes.

Most likely explanation is that it was not completed and could be removed.

>From here there are IMO only false positives.
> 
> ======================================================================
> 
> And the things that are clear false positives in cppcheck:
> 
> ----------------------------------------------------------------------
> 
> False positives because cppcheck doesn't correctly handle constructs
> of the form "case MACRO:" or other cases where macros have specific
> syntax:
> 
> ----------------------------------------------------------------------
>
> > vim hurd/hurdexec.c +361 # (error) Analysis failed. If the code is valid then please report this failure.
>
> False positive: This looks valid, but depends on value of a macro.
>
> ----------------------------------------------------------------------
> 
> And a few things in the soft-float code that depend on chasing down
> macro values that I didn't have time to do:
> 
> >vim soft-fp/fmadf4.c +43 # (error) Uninitialized variable: TD_f0
> Needs evaluation: This depends on macro chains that I didn't track
> down, but look unlikely to contain real bugs.
> 
> >vim soft-fp/adddf3.c +43 # (error) Uninitialized variable: R_f
> Needs evaluation: This depends on macro chains that I didn't track
> down, but look unlikely to contain real bugs.
> 
> ----------------------------------------------------------------------
> 
> False positives because cppcheck is confused between accessing the
> value of an uninitialized variable and accessing its address:
> 
> >vim login/openpty.c +94 # (error) Uninitialized variable: _buf
> False positive: cppcheck doesnât realize that âchar _buf[512]â
> defines _buf (as an address).
> 
> ----------------------------------------------------------------------
> 
> False positives because cppcheck isn't correctly handling values
> that are set in clauses of conditionals after "&&" and "||"
> operators:
> 
> >vim grp/fgetgrent_r.c +97 # (error) Uninitialized variable: parse_result
> False positive: parse_result is set in a clause of a complex
> conditional that must be evaluated to leave the preceding while
> loop.

> >vim sysdeps/posix/getaddrinfo.c +1436 # (error) Uninitialized variable: scope
> False positive: This is only uninitialized if we exit from an
> infinite loop, which of course never happens.  (There's a
> conditional return in the middle of the loop, which happens
> instead.)
> 
> >vim sunrpc/rpc_cout.c +638 # (error) Uninitialized variable: ptr
> False positive: ptr is initialized in part of the containing "if" clause.
> 
> ----------------------------------------------------------------------
> 
> And some general false positives due to cppcheck not understanding
> the control flow:
> 
> >vim csu/libc-tls.c +178 # (error) Possible null pointer dereference: initimage
> False positive(?): We may indeed call memcpy with a NULL input
> pointer, but only with a length of zero so the pointer isn't
> dereferenced.
> 
> >vim elf/dl-error.c +187 # (error) Uninitialized struct member: c.objname
> >vim elf/dl-error.c +189 # (error) Uninitialized struct member: c.malloced
> False positive: cppcheck doesnât understand setjmp control paths.
> 
> >vim hurd/hurdstartup.c +101 # (error) Uninitialized struct member: data.flags
> False positive: data.flags is only used if the previous conditional
> was entered, which would define it.
> 
> >vim stdio-common/printf-prs.c +73 # (error) Uninitialized struct member: spec.next_fmt
> False positive: cppcheck doesnât realize third clause of for(;;)
> construct is only evaluated after the loop body.
> 
> >vim sysdeps/ieee754/ldbl-128/e_asinl.c +204 # (error) Uninitialized variable: t
> False positive: If we donât return from the middle of the previous
> âifâ nest, t is defined.
> 
> >vim sysdeps/ieee754/ldbl-128ibm/e_asinl.c +197 # (error) Uninitialized variable: t
> False positive: If we donât return from the middle of the previous
> âifâ nest, t is defined.
> 
> >vim nptl/sysdeps/s390/pthread_spin_trylock.c +32 # (error)
> Uninitialized variable: old
> False positive: Defined in __asm block.
> 
> ----------------------------------------------------------------------
> 
> Cppcheck doesn't understand nested functions either:
> 
> >vim sysdeps/mach/hurd/getpriority.c +35 # (error) Uninitialized variable: pip
> False positive: cppcheck doesnât understand nested function definitions.
> 
> >vim sysdeps/mach/hurd/sigwait.c +39 # (error) Uninitialized variable: ss
> False positive: cppcheck doesnât understand nested function definitions.
> 
> ----------------------------------------------------------------------
> 
> Some cases where the output from cppcheck was entirely incomprehensible:
> 
> >vim sysdeps/ieee754/flt-32/s_sinf.c +54 # (error) Array 'y[1]' accessed at index 1, which is out of bounds.
> >vim sysdeps/ieee754/flt-32/s_sinf.c +56 # (error) Array 'y[1]' accessed at index 1, which is out of bounds.
> False positive: This looks totally bogus; y is declared as y[2].
> 
> >vim sysdeps/mach/hurd/i386/sigreturn.c +131 # (error) Invalid number of character (() when these macros are defined: ''.
> False positive: It is not even clear what cppcheck is thinking is
> happening here.
> 
> ----------------------------------------------------------------------
> 
> And, finally, the only false positive in the whole lot that is
> actually completely defensible:
> 
> >vim sysdeps/mach/hurd/_exit.c +43 # (error) Division by zero.
> False positive: Yes, it's invalid, but we meant to do that.  It's a