Re: y.a. static code analysis

From: Alex Riesen <raa.lkml@gmail.com>
Date: May 24, 2009, 13:04 UTC
Message-ID: <81b0412b0905240604q3c8c798bi6ad64d8916b4cd9e@mail.gmail.com>
In-Reply-To: <927245250905231200ifbda2f6t1c54628e314d63e6@mail.gmail.com>

2009/5/23 Serhat Şevki Dinçer <jfcgauss@gmail.com>:

Show 10 quoted lines

> i think only the ones about date.c (below note) are real defects
> (first chars are not checked).
>
> and also how about http://scan.coverity.com? i see it was mentined
> before (http://article.gmane.org/gmane.comp.version-control.git/111562)
> with apparently no responses or arguments (there has been a suggestion
> of bad license terms in that message, but if the scan is suitable for
> so many FOSS (see all rungs) including the kernel, why would it be not
> good for git?). i think it could be a good free (as in beer) code
> check for git.

There is a reason why the static checking tools are not popular: too many false positives.

> [./builtin-apply.c:482]: (error) Using 'name' after it is deallocated / released

Just wrong.

> [./compat/mingw.c:273]: (style) Found 'mktemp'. You should use 'mkstemp' instead
> [./compat/mkdtemp.c:5]: (style) Found 'mktemp'. You should use 'mkstemp' instead

Assuming the platform (see "compat"?) has mkstemp(3).

> [./date.c:268]: (style) Redundant code: Found a statement that begins
> with numeric constant
> [./date.c:483]: (style) Redundant code: Found a statement that begins
> with numeric constant

There is no numeric constant in the line, and while you're right (almost) regarding skipping the first character, the message itself is confusing.

You're not completely right, because looking at the code, the character you think is skipped is already tested for existence in other places. Yes, the code could be clearer at this point. Could be just a sign of refactoring passes, though.

> [./http-push.c:1419]: (error) Using 'lock' after it is deallocated / released

This is the only real bug.

Show 6 quoted lines

> [./read-cache.c:938] -> [./read-cache.c:759] -> [./read-cache.c:729]:
> (all) Array index out of bounds
> [./read-cache.c:938] -> [./read-cache.c:759] -> [./read-cache.c:731]:
> (all) Array index out of bounds
> [./read-cache.c:938] -> [./read-cache.c:759] -> [./read-cache.c:736]:
> (all) Array index out of bounds

Definitely not. It is just a flexarray, worked around with array[1] for some compilers.

> [./test-sha1.c:16]: (error) Memory leak: buffer

The program ends and there is no point deallocating the buffer.