|
From: Madhu M K. <mm...@ya...> - 2005-05-24 09:16:40
|
Hi folks,
As part of some course work that I'm doing at Stanford, there was a need to
apply static code analysis to some particular free/open source software. I
asked for permission and then applied this analysis on Valgrind. The
symmetry of applying static analysis to *the* dynamic analysis software on
Linux is not lost on me :). The tool used was Prevent [1] from Coverity Inc
[2]. This is a tool in the same lineage that was used with great success on
other free/open source projects such as Linux, OpenBSD etc.
I looked primarily at the 2.4.0 stable tree. The bottom line is that
Valgrind is in pretty good shape, IMO. For a code base that is as complex
and large as Valgrind, 23-26 possible error indicators is very decent. I got
the two numbers as I went back and tried to tweak parameters to see if I
could get any more.
To the best of my ability, most of the issues found are correct - from the
analytic sense. Except that we may not care about some of them. One example
of this class is the leak that occurs in coregrind/ume.c. The elfinfo struct
allocated in readelf() is not always freed - in the error cases (346,390,397
ume.c). In reality, the error cases imply we would exit out and releasing
memory is probably not critical. However, we might want to fix this if we
ever do something different rather than just exit.
Additionally, another resource leak involving get_file_clo from vg_main.c is
probably intended as there's a little comment above the definition of
get_file_clo:
/* Nb: malloc'd memory never freed -- kept throughout like argv, envp */
On the other hand, some of the check after use, use before check, overrun
static are indeed possible issues that we may want to fix. This is the
distribution of indicators:
5 coregrind/vg_translate.c
3 coregrind/ume.c
2 massif/ms_main.
2 massif/hp2ps/HpFile.c
2 coregrind/vg_mylibc.c
2 coregrind/vg_main.c
2 coregrind/demangle/cp-demangle.c
1 memcheck/mc_main.c
1 massif/hp2ps/AuxFile.c
1 coregrind/vg_threadmodel.c
1 coregrind/vg_scheduler.c
1 coregrind/vg_malloc2.c
1 coregrind/vg_dwarf.c
1 coregrind/demangle/cplus-dem.c
1 addrcheck/ac_main.c
I'll be sending more details for each, it is very possible that I could have
missed the intention of the code, so would it be possible for folks to look
at them and see if the indicator is really a problem? I'm currently going to
send them into the list, I can additionally open bugs if folks feel that
bugzilla is going to be a better interface.
Thanks,
Madhu
[1] http://www.coverity.com/products/nf_products_prevent.html
[2] http://www.coverity.com
Cheerio,
M
Madhu M Kurup /* Nemo Me Impune Lacessit */ mmk at yahoo-inc dt com
|
|
From: Nicholas N. <nj...@cs...> - 2005-05-24 13:04:29
|
On Tue, 24 May 2005, Madhu M Kurup wrote: > As part of some course work that I'm doing at Stanford, there was a need to > apply static code analysis to some particular free/open source software. I > asked for permission and then applied this analysis on Valgrind. The > symmetry of applying static analysis to *the* dynamic analysis software on > Linux is not lost on me :). The tool used was Prevent [1] from Coverity Inc > [2]. This is a tool in the same lineage that was used with great success on > other free/open source projects such as Linux, OpenBSD etc. Awesome! Thanks for doing this. > Additionally, another resource leak involving get_file_clo from vg_main.c is > probably intended as there's a little comment above the definition of > get_file_clo: > > /* Nb: malloc'd memory never freed -- kept throughout like argv, envp */ Hmm, looking at the code that comment is wrong -- the allocated memory gets copied later in augment_command_line(), and then there is a definite leak. The ones you've sent patches for mostly look pretty minor, but they're worth fixing. > I'll be sending more details for each, it is very possible that I could have > missed the intention of the code, so would it be possible for folks to look > at them and see if the indicator is really a problem? I'm currently going to > send them into the list, I can additionally open bugs if folks feel that > bugzilla is going to be a better interface. I'll definitely look soon at the ones in code that I understand. You haven't sent patches for all the files you mentioned (eg. vg_translate.c ac_main.c) -- are they coming, or are they ones you judged unimportant? Thanks again for doing this. N |
|
From: Madhu M K. <mm...@ya...> - 2005-05-24 17:34:53
|
Hi, > > I'll definitely look soon at the ones in code that I understand. You > haven't sent patches for all the files you mentioned (eg. vg_translate.c > ac_main.c) -- are they coming, or are they ones you judged unimportant? > There are a bunch of indicators that I could not reconcile with any problem. I decided to first send what seemed like real issues before reporting the ones that possible could be a analysis error. I will report all of them for sure. Cheerio, M Madhu M Kurup /* Nemo Me Impune Lacessit */ mmk at yahoo-inc dt com |
|
From: Nicholas N. <nj...@cs...> - 2005-05-24 18:07:59
|
On Tue, 24 May 2005, Madhu M Kurup wrote: > There are a bunch of indicators that I could not reconcile with any problem. > I decided to first send what seemed like real issues before reporting the > ones that possible could be a analysis error. I will report all of them for > sure. Great. I'd be interested to see exactly what information the tool is spitting out. N |
|
From: Nicholas N. <nj...@cs...> - 2005-06-19 14:57:13
|
On Tue, 24 May 2005, Nicholas Nethercote wrote: >> Additionally, another resource leak involving get_file_clo from >> vg_main.c is probably intended as there's a little comment above the >> definition of get_file_clo: >> >> /* Nb: malloc'd memory never freed -- kept throughout like argv, envp */ > > Hmm, looking at the code that comment is wrong -- the allocated memory gets > copied later in augment_command_line(), and then there is a definite leak. Turns out I was wrong about that, and my freeing of that memory broke the reading of options from files! Ah. Nick |