Today I decided to try Coccinelle on latest Lustre code found on master at git://git.whamcloud.com/fs/lustre-release.git. I came up with a simple Coccinelle script that tries to detect the case where a new object is allocated and dereferenced without checking it against NULL. Eight such bugs were unconvered: $ spatch -sp_file /tmp/LIBCFS_ALLOC.cocci -dir lnet 2>/dev/null > /tmp/lustre.diff $ spatch -sp_file /tmp/OBD_ALLOC.cocci -dir lustre 2>/dev/null >> /tmp/lustre.diff $ diffstat /tmp/lustre.diff b/llite/dir.c | 2 ++ b/mdc/lproc_mdc.c | 1 + b/mgc/mgc_request.c | 1 + b/obdclass/obd_mount.c | 2 ++ b/selftest/conctl.c | 1 + obdclass/obd_mount.c | 1 + 6 files changed, 8 insertions(+) I''ve attached here the lustre.diff, which contained dummy fixes. I hope they do get fixed. OBD_ALLOC.cocci is also attached. LIBCFS_ALLOC.cocci is almost identical - I guess they could be combined into one with some regex matching but I haven''t figured out how yet. This is to demonstrate how useful such a tool can be. The Linux kernel seems to have already integrated Coccinelle checks in the build system. Lustre could adopt something similar, and Coccinelle could do much more complex things than this. - Isaac -------------- next part -------------- // find calls to malloc @call@ expression ptr; expression E; position p; @@ OBD_ALLOC(ptr at p, E); // find ok calls to malloc @ok@ expression ptr; expression E; position call.p; @@ OBD_ALLOC(ptr at p, E); ... when != ptr ( (ptr == NULL || ...) | (ptr != NULL || ...) ) // fix bad calls to malloc @depends on !ok@ expression ptr; expression E; position call.p; @@ OBD_ALLOC(ptr at p, E); + LASSERT(ptr != NULL); /* or handle the NULL case? */ -------------- next part -------------- A non-text attachment was scrubbed... Name: lustre.diff Type: text/x-diff Size: 3435 bytes Desc: not available Url : http://lists.lustre.org/pipermail/lustre-discuss/attachments/20120108/c4fd14bd/attachment.bin
Nikitas Angelinas
2012-Jan-08 09:43 UTC
[Lustre-discuss] Finding bugs in Lustre with Coccinelle
Hi Isaac, Funny, I was planning to have a look at this, this weekend if time permitted. I was interested in finding out how noticeable the issue of false positives may be in Coccinelle, but that shouldn''t be a big problem as one will go through the generated patches manually anyway. Its application in the Linux kernel shows that it can yield fixes for common types of bugs, and depict cases of API misuse or where more efficient use of APIs can be made, and as you say, I guess much more complex things if we wanted to, so ftr I also think this could be a useful addition to Lustre. Cheers, Nikitas On Sun, 2012-01-08 at 00:58 -0700, Isaac Huang wrote:> Today I decided to try Coccinelle on latest Lustre code found on > master at git://git.whamcloud.com/fs/lustre-release.git. > > I came up with a simple Coccinelle script that tries to detect the > case where a new object is allocated and dereferenced without checking > it against NULL. > > Eight such bugs were unconvered: > $ spatch -sp_file /tmp/LIBCFS_ALLOC.cocci -dir lnet 2>/dev/null > /tmp/lustre.diff > $ spatch -sp_file /tmp/OBD_ALLOC.cocci -dir lustre 2>/dev/null >> /tmp/lustre.diff > $ diffstat /tmp/lustre.diff > b/llite/dir.c | 2 ++ > b/mdc/lproc_mdc.c | 1 + > b/mgc/mgc_request.c | 1 + > b/obdclass/obd_mount.c | 2 ++ > b/selftest/conctl.c | 1 + > obdclass/obd_mount.c | 1 + > 6 files changed, 8 insertions(+) > > I''ve attached here the lustre.diff, which contained dummy fixes. I > hope they do get fixed. OBD_ALLOC.cocci is also attached. > LIBCFS_ALLOC.cocci is almost identical - I guess they could be > combined into one with some regex matching but I haven''t figured out > how yet. > > This is to demonstrate how useful such a tool can be. The Linux kernel > seems to have already integrated Coccinelle checks in the build > system. Lustre could adopt something similar, and Coccinelle could do > much more complex things than this. > > - Isaac > _______________________________________________ > Lustre-discuss mailing list > Lustre-discuss at lists.lustre.org > http://lists.lustre.org/mailman/listinfo/lustre-discuss
Andreas Dilger
2012-Jan-08 17:20 UTC
[Lustre-discuss] Finding bugs in Lustre with Coccinelle
Isaac, I''m all in favor of using static code analysis tools to find bugs like this. The first step, as you have done is to find and fix the bugs (though with proper patches since LASSERT() as a means of error handling is unacceptable). After this it would be possible to add such a script to the build and/or prepare-commit-msg hook so that it detects such errors early in the development process. We are now entering a 3 month feature freeze for the 2.2 branch, so it is a perfect time to fix latent bugs like this before the 2.2 release. Please don''t delay too long since the last month will be a code freeze and only bugs being hit during testing will be landed. Cheers, Andreas On 2012-01-08, at 0:58, Isaac Huang <isaac_huang at xyratex.com> wrote:> Today I decided to try Coccinelle on latest Lustre code found on > master at git://git.whamcloud.com/fs/lustre-release.git. > > I came up with a simple Coccinelle script that tries to detect the > case where a new object is allocated and dereferenced without checking > it against NULL. > > Eight such bugs were unconvered: > $ spatch -sp_file /tmp/LIBCFS_ALLOC.cocci -dir lnet 2>/dev/null > /tmp/lustre.diff > $ spatch -sp_file /tmp/OBD_ALLOC.cocci -dir lustre 2>/dev/null >> /tmp/lustre.diff > $ diffstat /tmp/lustre.diff > b/llite/dir.c | 2 ++ > b/mdc/lproc_mdc.c | 1 + > b/mgc/mgc_request.c | 1 + > b/obdclass/obd_mount.c | 2 ++ > b/selftest/conctl.c | 1 + > obdclass/obd_mount.c | 1 + > 6 files changed, 8 insertions(+) > > I''ve attached here the lustre.diff, which contained dummy fixes. I > hope they do get fixed. OBD_ALLOC.cocci is also attached. > LIBCFS_ALLOC.cocci is almost identical - I guess they could be > combined into one with some regex matching but I haven''t figured out > how yet. > > This is to demonstrate how useful such a tool can be. The Linux kernel > seems to have already integrated Coccinelle checks in the build > system. Lustre could adopt something similar, and Coccinelle could do > much more complex things than this. > > - Isaac > <OBD_ALLOC.cocci> > <lustre.diff> > _______________________________________________ > Lustre-discuss mailing list > Lustre-discuss at lists.lustre.org > http://lists.lustre.org/mailman/listinfo/lustre-discuss
Karsten Weiss
2012-Jan-09 11:33 UTC
[Lustre-discuss] Finding bugs in Lustre with Coccinelle
On Sun, 8 Jan 2012, Andreas Dilger wrote:> I''m all in favor of using static code analysis tools to find bugs like this.I''ve compiled(^1) Lustre 2.1.0 on CentOS 6.2 with Clang''s static analyzer (LLVM 3.0). Here''s the bug summary to give you an idea of the result: Bug Type Quantity =======================================================All Bugs 594 Dead code Idempotent operation 11 Dead store Dead assignment 76 Dead increment 3 Dead initialization 3 Logic error Assigned value is garbage or undefined 9 Called function pointer is null (null dereference) 27 Dereference of null pointer 456 Dereference of undefined pointer value 1 Division by zero 3 Function call argument is an uninitialized value 4 Garbage return value 1 You can download the full result (annotated source code) here: http://dl.dropbox.com/u/1868416/lustre-2.1.0-scan-build.tar.bz2 (I will delete this file in a couple of days) To view the result extract the archive and point your web browser at: lustre-2.1.0-scan-build/2012-01-09-1/index.html Cheers, Karsten ^1: Here''s what I used: touch META sh autogen.sh scan-build ./configure --disable-server \ --with-linux=/usr/src/kernels/2.6.32-220.2.1.el6.x86_64 \ --with-linux-obj=/lib/modules/2.6.32-220.2.1.el6.x86_64/build/ \ --with-downstream-release=wc1 mkdir ./lustre-2.1.0-scan-build scan-build -o ./lustre-2.1.0-scan-build/ make -j 24 -- ___________________________________________creating IT solutions Dipl.-Inf. Karsten Weiss science + computing ag phone: +49 7071 9457 452 Hagellocher Weg 73 teamline: +49 7071 9457 681 72070 Tuebingen email: k.weiss at science-computing.de www.science-computing.de -- Vorstand/Board of Management: Dr. Bernd Finkbeiner, Dr. Roland Niemeier, Dr. Arno Steitz, Dr. Ingrid Zech Vorsitzender des Aufsichtsrats/ Chairman of the Supervisory Board: Philippe Miltin Sitz/Registered Office: Tuebingen Registergericht/Registration Court: Stuttgart Registernummer/Commercial Register No.: HRB 382196
On Sun, Jan 08, 2012 at 09:43:00AM +0000, Nikitas Angelinas wrote:> Hi Isaac, > > Funny, I was planning to have a look at this, this weekend if time > permitted. I was interested in finding out how noticeable the issue of > false positives may be in Coccinelle, but that shouldn''t be a big > problem as one will go through the generated patches manually anyway.It did ring a couple of false alarms, which I removed from the attached patch. But I believe they could all be avoided by improving the simplistic Coccinelle script. - Isaac
On Sun, Jan 08, 2012 at 10:20:36AM -0700, Andreas Dilger wrote:> Isaac, > I''m all in favor of using static code analysis tools to find bugs like this. The first step, as you have done is to find and fix the bugs (though with proper patches since LASSERT() as a means of error handling is unacceptable).That patch was only meant to point out where the bugs were.> ...... > We are now entering a 3 month feature freeze for the 2.2 branch, so it is a perfect time to fix latent bugs like this before the 2.2 release. Please don''t delay too long since the last month will be a code freeze and only bugs being hit during testing will be landed.I probably don''t have the time to fix them by myself but I''ll open JIRA tickets for them. - Isaac
Andreas Dilger
2012-Jan-12 05:22 UTC
[Lustre-discuss] Finding bugs in Lustre with Coccinelle
On 2012-01-09, at 4:33 AM, Karsten Weiss wrote:> I''ve compiled(^1) Lustre 2.1.0 on CentOS 6.2 with Clang''s static analyzer > (LLVM 3.0). Here''s the bug summary to give you an idea of the result: > > Bug Type Quantity > =======================================================> All Bugs 594 > > Dead code > Idempotent operation 11 > > Dead store > Dead assignment 76 > Dead increment 3 > Dead initialization 3 > > Logic error > Assigned value is garbage or undefined 9 > Called function pointer is null (null dereference) 27 > Dereference of null pointer 456 > Dereference of undefined pointer value 1 > Division by zero 3 > Function call argument is an uninitialized value 4 > Garbage return value 1 > > You can download the full result (annotated source code) here: > > http://dl.dropbox.com/u/1868416/lustre-2.1.0-scan-build.tar.bz2 > (I will delete this file in a couple of days)Karsten, can you please split this tarball into a couple of smaller parts and attach it to http://bugs.whamcloud.com/browse/LU-871, the bug that I previously opened to track defects found by Clang/LLVM. The attachment size limit is 10MB, but splitting into logical parts by the defect type would be ideal.> To view the result extract the archive and point your web browser at: > > lustre-2.1.0-scan-build/2012-01-09-1/index.html > > Cheers, > Karsten > > ^1: Here''s what I used: > > touch META > sh autogen.sh > scan-build ./configure --disable-server \ > --with-linux=/usr/src/kernels/2.6.32-220.2.1.el6.x86_64 \ > --with-linux-obj=/lib/modules/2.6.32-220.2.1.el6.x86_64/build/ \ > --with-downstream-release=wc1 > mkdir ./lustre-2.1.0-scan-build > scan-build -o ./lustre-2.1.0-scan-build/ make -j 24 > > -- > ___________________________________________creating IT solutions > Dipl.-Inf. Karsten Weiss science + computing ag > phone: +49 7071 9457 452 Hagellocher Weg 73 > teamline: +49 7071 9457 681 72070 Tuebingen > email: k.weiss at science-computing.de www.science-computing.de > -- > Vorstand/Board of Management: > Dr. Bernd Finkbeiner, Dr. Roland Niemeier, > Dr. Arno Steitz, Dr. Ingrid Zech > Vorsitzender des Aufsichtsrats/ > Chairman of the Supervisory Board: > Philippe Miltin > Sitz/Registered Office: Tuebingen > Registergericht/Registration Court: Stuttgart > Registernummer/Commercial Register No.: HRB 382196 > >Cheers, Andreas -- Andreas Dilger Whamcloud, Inc. Principal Engineer http://www.whamcloud.com/
Helo! On Jan 12, 2012, at 12:22 AM, Andreas Dilger wrote:>> You can download the full result (annotated source code) here: >> >> http://dl.dropbox.com/u/1868416/lustre-2.1.0-scan-build.tar.bz2 >> (I will delete this file in a couple of days) > Karsten, > can you please split this tarball into a couple of smaller parts and > attach it to http://bugs.whamcloud.com/browse/LU-871, the bug that I > previously opened to track defects found by Clang/LLVM.Also would be great to run something like this on 2.2 codebase, I guess. Bye, Oleg -- Oleg Drokin Senior Software Engineer Whamcloud, Inc.
Karsten Weiss
2012-Jan-12 16:19 UTC
[Lustre-discuss] Finding bugs in Lustre with Coccinelle
On Thu, 12 Jan 2012, Oleg Drokin wrote:> Also would be great to run something like this on 2.2 codebase, I guess.I''ve updated LU-871 with the results of a new Clang SA scan-build run on the Lustre 2.1.54 source code (i.e. today''s git HEAD): http://bugs.whamcloud.com/browse/LU-871 (To trim the tarball size I decided to disable the core.NullDereference check.) Cheers, Karsten