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
// find calls to malloc
@call@
expression ptr;
expression E;
position p;
@@
OBD_ALLOC(ptr@p, E);
// find ok calls to malloc
@ok@
expression ptr;
expression E;
position call.p;
@@
OBD_ALLOC(ptr@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@p, E);
+ LASSERT(ptr != NULL); /* or handle the NULL case? */
diff -u -p a/selftest/conctl.c b/selftest/conctl.c
--- a/selftest/conctl.c
+++ b/selftest/conctl.c
@@ -751,6 +751,7 @@ int lst_test_add_ioctl(lstio_test_args_t
goto out;
LIBCFS_ALLOC(dstgrp, args-lstio_tes_dgrp_nmlen + 1);
+C2_ASSERT(dstgrp != NULL);/* or handle the NULL case? */
if (srcgrp == NULL)
goto out;
diff -u -p a/mdc/lproc_mdc.c b/mdc/lproc_mdc.c
--- a/mdc/lproc_mdc.c
+++ b/mdc/lproc_mdc.c
@@ -103,6 +103,7 @@ static int mdc_wr_kuc(struct file *file,
/* for mockup below */ 2 * cfs_size_round(sizeof(*hai));
OBD_ALLOC(lh, len);
+LASSERT(lh != NULL);/* or handle the NULL case? */
lh-kuc_magic = KUC_MAGIC;
lh-kuc_transport = KUC_TRANSPORT_HSM;
diff -u -p a/obdclass/obd_mount.c b/obdclass/obd_mount.c
--- a/obdclass/obd_mount.c
+++ b/obdclass/obd_mount.c
@@ -1562,6 +1562,7 @@ static void server_put_super(struct supe
tmpname_sz = strlen(lsi-lsi_ldd-ldd_svname) + 1;
OBD_ALLOC(tmpname, tmpname_sz);
+LASSERT(tmpname != NULL);/* or handle the NULL case? */
memcpy(tmpname, lsi-lsi_ldd-ldd_svname, tmpname_sz);
CDEBUG(D_MOUNT, server put_super %s\n, tmpname);
if (IS_MDT(lsi-lsi_ldd) (lsi-lsi_lmd-lmd_flags LMD_FLG_NOSVC))
@@ -1581,6 +1582,7 @@ static void server_put_super(struct supe
lprof = class_get_profile(lsi-lsi_ldd-ldd_svname);
if (lprof lprof-lp_dt) {
OBD_ALLOC(extraname, strlen(lprof-lp_dt) + 1);
+LASSERT(extraname != NULL);/* or handle the NULL case? */
strcpy(extraname, lprof-lp_dt);
}
diff -u -p a/llite/dir.c b/llite/dir.c
--- a/llite/dir.c
+++ b/llite/dir.c
@@ -672,6 +672,7 @@ char *ll_get_fsname(struct inode *inode)
int len;
OBD_ALLOC(fsname, MGS_PARAM_MAXLEN);
+LASSERT(fsname != NULL);/* or handle the NULL case? */
len = strlen(lsi-lsi_lmd-lmd_profile);
ptr = strrchr(lsi-lsi_lmd-lmd_profile, '-');
if (ptr (strcmp(ptr, -client) == 0))
@@ -746,6 +747,7 @@ int ll_dir_setstripe(struct inode *inode
need the make the distiction between the 2 versions */
if (set_default mgc-u.cli.cl_mgc_mgsexp) {
OBD_ALLOC(param, MGS_PARAM_MAXLEN);
+LASSERT(param != NULL);/* or handle the NULL case? */
/* Get fsname and assume devname to be -MDT. */
fsname = ll_get_fsname(inode);
diff -u -p a/obdclass/obd_mount.c b/obdclass/obd_mount.c
--- a/obdclass/obd_mount.c
+++ b/obdclass/obd_mount.c
@@ -753,6 +753,7 @@ static int lustre_start_mgc(struct super
/* Random uuid for MGC allows easier reconnects */
OBD_ALLOC_PTR(uuid);
+LASSERT(uuid != NULL);/* or handle the NULL case? */
ll_generate_random_uuid(uuidc);
class_uuid_unparse(uuidc, uuid);
diff -u -p a/mgc/mgc_request.c b/mgc/mgc_request.c
--- a/mgc/mgc_request.c
+++ b/mgc/mgc_request.c
@@ -1621,6 +1621,7 @@ static int mgc_copy_llog(struct obd_devi
/* set the log header uuid for fun */
OBD_ALLOC_PTR(uuid);
+LASSERT(uuid != NULL);/* or handle the NULL case? */
obd_str2uuid(uuid,
