y.a. static code analysis

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

y.a. static code analysis

Serhat Sevki Dincer
hi,
although static code analysis have apparently been used/mentioned
here, i did not see any mention of cppcheck
(http://cppcheck.wiki.sourceforge.net) in the mailist archive. i was
playing with cppcheck (1.32) on some OSS, so i decided to try it on
git (1.6.3.1) as well.
$ cppcheck -a -q -s . &> ccgit.txt
possibly the most useful parts of the output are:
$ grep -v 'is never used\|The scope of the variable\| Error: In' ccgit.txt
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.
regards

note:
[./builtin-apply.c:482]: (error) Using 'name' after it is deallocated / released
[./compat/mingw.c:273]: (style) Found 'mktemp'. You should use 'mkstemp' instead
[./compat/mkdtemp.c:5]: (style) Found 'mktemp'. You should use 'mkstemp' instead
[./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
[./http-push.c:1419]: (error) Using 'lock' after it is deallocated / released
[./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
[./test-sha1.c:16]: (error) Memory leak: buffer

ccgit.txt (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: y.a. static code analysis

Alex Riesen
2009/5/23 Serhat Şevki Dinçer <[hidden email]>:

> 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.

> [./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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: y.a. static code analysis

Alex Riesen
Noticed and reported by Serhat Şevki Dinçer.

Signed-off-by: Alex Riesen <[hidden email]>
---

Alex Riesen, Sun, May 24, 2009 15:04:06 +0200:
> 2009/5/23 Serhat Şevki Dinçer <[hidden email]>:
>
> > [./http-push.c:1419]: (error) Using 'lock' after it is deallocated / released
>
> This is the only real bug.
>

Here's a fix.

 http-push.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/http-push.c b/http-push.c
index dac2c6e..45e8a69 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1415,8 +1415,9 @@ static void remove_locks(void)
 
  fprintf(stderr, "Removing remote locks...\n");
  while (lock) {
+ struct remote_lock *next = lock->next;
  unlock_remote(lock);
- lock = lock->next;
+ lock = next;
  }
 }
 
--
1.6.3.1.93.g316b2



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: y.a. static code analysis

Clemens Buchacher
On Sun, May 24, 2009 at 03:16:49PM +0200, Alex Riesen wrote:
> Noticed and reported by Serhat Şevki Dinçer.
>
> Signed-off-by: Alex Riesen <[hidden email]>

Thanks. Ack.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html