e2fsprogs coverity patch <cid-1.diff>

e2fsprogs coverity patch <cid-1.diff>

[Brian D. Behlendorf]

Lawrence Livermore National Labs recently ran the source code
analysis tool Coverity over the e2fsprogs-1.39 source to see 
if it would identify any significant bugs.  The analysis
turned up 38 mostly minor issues which are enumerated here
with patches.  We went through and resolved these issues
but would love to see these mostly minor changes reviewed
and commited upstream.

Thanks,
Brian Behlendorf <behlendorf1@llnl.gov>, and
Herb Wartens <wartens2@llnl.gov>

-----------------------------------------------------------------------------
Coverity ID: 2: Checked Return

Found 2 of the three places where a return code for ext2fs_write_inode() was
not being checked.

The second fix in e2fsck/emptydir.c is basically just to shut coverity up even
though it really is unnecessary.

Index: e2fsprogs+chaos/resize/resize2fs.c
===================================================================
--- e2fsprogs+chaos.orig/resize/resize2fs.c
+++ e2fsprogs+chaos/resize/resize2fs.c
@@ -1303,7 +1303,9 @@ static int check_and_change_inodes(ext2_
 	retval = ext2fs_read_inode(is->rfs->old_fs, dir, &inode);
 	if (retval == 0) {
 		inode.i_mtime = inode.i_ctime = time(0);
-		ext2fs_write_inode(is->rfs->old_fs, dir, &inode);
+		retval = ext2fs_write_inode(is->rfs->old_fs, dir, &inode);
+		if (retval)
+			return DIRENT_ERROR;
 	}
 
 	return DIRENT_CHANGED;
Index: e2fsprogs+chaos/e2fsck/emptydir.c
===================================================================
--- e2fsprogs+chaos.orig/e2fsck/emptydir.c
+++ e2fsprogs+chaos/e2fsck/emptydir.c
@@ -170,7 +170,9 @@ static int fix_directory(ext2_filsys fs,
 		edi->inode.i_size -= edi->freed_blocks * fs->blocksize;
 		edi->inode.i_blocks -= edi->freed_blocks *
 			(fs->blocksize / 512);
-		(void) ext2fs_write_inode(fs, db->ino, &edi->inode);
+		retval = ext2fs_write_inode(fs, db->ino, &edi->inode);
+		if (retval)
+			return 0;
 	}
 	return 0;
 }

Re: e2fsprogs coverity patch <cid-1.diff>

[Theodore Tso]

On Fri, Feb 09, 2007 at 06:08:16PM -0800, Brian D. Behlendorf wrote:
> Lawrence Livermore National Labs recently ran the source code
> analysis tool Coverity over the e2fsprogs-1.39 source to see 
> if it would identify any significant bugs.  The analysis
> turned up 38 mostly minor issues which are enumerated here
> with patches.  We went through and resolved these issues
> but would love to see these mostly minor changes reviewed
> and commited upstream.

Hi Brian, please see the SUBMITTING-PATCHES file in the e2fsprogs
repository.  

May I assume that you are willing to certify to the Developer's
Certification of Origin 1.1 statement so I can add a:

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>

to each of these patches before I check them into the e2fsprogs source
control repository?  

In the future, please add the Signed-off-by: line when submitting
patches assuming that you are willing to certify to the DCO 1.1.

Thanks, regards,

						- Ted


(The SUBMITTING-PATCHES file has been included below for your
convenience.)

Like the Linux kernel, submitted e2fsprogs patches now require the
following "sign-off" procedure:

The sign-off is a simple line at the end of the explanation for the
patch, which certifies that you wrote it or otherwise have the right to
pass it on as a open-source patch.  The rules are pretty simple: if you
can certify the below:

        Developer's Certificate of Origin 1.1

        By making a contribution to this project, I certify that:

        (a) The contribution was created in whole or in part by me and I
            have the right to submit it under the open source license
            indicated in the file; or

        (b) The contribution is based upon previous work that, to the best
            of my knowledge, is covered under an appropriate open source
            license and I have the right under that license to submit that
            work with modifications, whether created in whole or in part
            by me, under the same open source license (unless I am
            permitted to submit under a different license), as indicated
            in the file; or

        (c) The contribution was provided directly to me by some other
            person who certified (a), (b) or (c) and I have not modified
            it.

	(d) I understand and agree that this project and the contribution
	    are public and that a record of the contribution (including all
	    personal information I submit with it, including my sign-off) is
	    maintained indefinitely and may be redistributed consistent with
	    this project or the open source license(s) involved.

then you just add a line saying

	Signed-off-by: Random J Developer <random@developer.example.org>

Re: e2fsprogs coverity patch <cid-1.diff>

[Brian Behlendorf]

Hi Ted,

  Sorry about failing to read the SUBMITTING-PATCHES file, I hadn't noticed
it.  Yes I'm willing to certify to the Developer's Certification of Origin 
1.1 statement, please add the following line to the patches:

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>

  I'll make sure to add the Signed-off-by line in the future.

Thanks,
Brian

> Hi Brian, please see the SUBMITTING-PATCHES file in the e2fsprogs
> repository.
>
> May I assume that you are willing to certify to the Developer's
> Certification of Origin 1.1 statement so I can add a:
>
> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
>
> to each of these patches before I check them into the e2fsprogs source
> control repository?
>
> In the future, please add the Signed-off-by: line when submitting
> patches assuming that you are willing to certify to the DCO 1.1.
>
> Thanks, regards,

Re: e2fsprogs coverity patch <cid-1.diff>

[Theodore Tso]

On Fri, Feb 09, 2007 at 06:08:16PM -0800, Brian D. Behlendorf wrote:
> Coverity ID: 2: Checked Return

Would I be right in assuming this should be Coverity ID 1, and not 2?
(cid-2.diff is also labelled as Coverity ID 1).

What is Coverity ID's, anyway?  I assume they are something you
locally assigned?

						- Ted

Re: e2fsprogs coverity patch <cid-1.diff>

[Andreas Dilger]

On Mar 18, 2007  09:40 -0400, Theodore Tso wrote:
> On Fri, Feb 09, 2007 at 06:08:16PM -0800, Brian D. Behlendorf wrote:
> > Coverity ID: 2: Checked Return
> 
> Would I be right in assuming this should be Coverity ID 1, and not 2?
> (cid-2.diff is also labelled as Coverity ID 1).
> 
> What is Coverity ID's, anyway?  I assume they are something you
> locally assigned?

Yes, this is just the error number within a given software project
at a particular site.

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.

Re: e2fsprogs coverity patch <cid-01.diff>

[Theodore Tso]

On Fri, Feb 09, 2007 at 06:08:16PM -0800, Brian D. Behlendorf wrote:
> Found 2 of the three places where a return code for ext2fs_write_inode() was
> not being checked.
> 
> 
> Index: e2fsprogs+chaos/resize/resize2fs.c
> ===================================================================
> --- e2fsprogs+chaos.orig/resize/resize2fs.c
> +++ e2fsprogs+chaos/resize/resize2fs.c
> @@ -1303,7 +1303,9 @@ static int check_and_change_inodes(ext2_
>  	retval = ext2fs_read_inode(is->rfs->old_fs, dir, &inode);
>  	if (retval == 0) {
>  		inode.i_mtime = inode.i_ctime = time(0);
> -		ext2fs_write_inode(is->rfs->old_fs, dir, &inode);
> +		retval = ext2fs_write_inode(is->rfs->old_fs, dir, &inode);
> +		if (retval)
> +			return DIRENT_ERROR;
>  	}

The new lines should read:

	is->err = ext2fs_write_inode(...)
	if (is->err)
		return DIRENT_ABORT

I'll fix up the patch before committing.

> Index: e2fsprogs+chaos/e2fsck/emptydir.c
> ===================================================================
> --- e2fsprogs+chaos.orig/e2fsck/emptydir.c


n.b. emptydir.c is the ultimate in dead code.  It was some code that I
never finished, and it's not linked into e2fsck at all (emptydir.c/o
is not mentioned in the Makefile at all :-).

						- Ted

Re: e2fsprogs coverity patch

[Theodore Tso]

FYI,

I have appled patches to fix all of the Coverity problems reported
except for cid-02.diff, for which the correct fix is to rewrite
ss_current_request() to be a function with proper testing of ss_idx
instead of patching out the error check.

						- Ted