[PATCH v2 0/4] xfsprogs misc fixes

[PATCH v2 0/4] xfsprogs misc fixes

[Vivek Trivedi]

minor fixes in xfsprogs, mostly coverity reported fixes.

Changes since v1:
 * to fix integer overflow warnings, change variable type instead
   of doing typecast

Vivek Trivedi (4):
  xfsprogs: xfs_io: fix a memory leak in imap_f
  xfsprogs: fix integer overflow in xlog_find_verify_cycle
  xfsprogs: mkfs: fix unintentional integer overflow
  xfsprogs: xfsdb: remove unnessary checks in
    process_leaf_node_dir_v2_free

 db/check.c                |    2 --
 io/imap.c                 |    7 ++++++-
 libxlog/xfs_log_recover.c |    2 +-
 mkfs/xfs_mkfs.c           |    2 +-
 4 files changed, 8 insertions(+), 5 deletions(-)

-- 
1.7.9.5

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v2 1/4] xfsprogs: xfs_io: fix a memory leak in imap_f

[Vivek Trivedi]

add NULL check for malloc return and free allocated memory in
return path in imap_f

Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
---
 io/imap.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/io/imap.c b/io/imap.c
index 34901cb..6ed98eb 100644
--- a/io/imap.c
+++ b/io/imap.c
@@ -39,6 +39,8 @@ imap_f(int argc, char **argv)
 		nent = atoi(argv[1]);
 
 	t = malloc(nent * sizeof(*t));
+	if (!t)
+		return 0;
 
 	bulkreq.lastip  = &last;
 	bulkreq.icount  = nent;
@@ -46,8 +48,10 @@ imap_f(int argc, char **argv)
 	bulkreq.ocount  = &count;
 
 	while (xfsctl(file->name, file->fd, XFS_IOC_FSINUMBERS, &bulkreq) == 0) {
-		if (count == 0)
+		if (count == 0) {
+			free(t);
 			return 0;
+		}
 		for (i = 0; i < count; i++) {
 			printf(_("ino %10llu count %2d mask %016llx\n"),
 				(unsigned long long)t[i].xi_startino,
@@ -55,6 +59,7 @@ imap_f(int argc, char **argv)
 				(unsigned long long)t[i].xi_allocmask);
 		}
 	}
+	free(t);
 	perror("xfsctl(XFS_IOC_FSINUMBERS)");
 	exitcode = 1;
 	return 0;
-- 
1.7.9.5

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2 1/4] xfsprogs: xfs_io: fix a memory leak in imap_f

[Christoph Hellwig]

> @@ -39,6 +39,8 @@ imap_f(int argc, char **argv)
> -		if (count == 0)
> +		if (count == 0) {
> +			free(t);
>  			return 0;
> +		}

please use a goto out_free; here

>
>  		for (i = 0; i < count; i++) {
>  			printf(_("ino %10llu count %2d mask %016llx\n"),
>  				(unsigned long long)t[i].xi_startino,
> @@ -55,6 +59,7 @@ imap_f(int argc, char **argv)
>  				(unsigned long long)t[i].xi_allocmask);
>  		}
>  	}
> +	free(t);
>  	perror("xfsctl(XFS_IOC_FSINUMBERS)");
>  	exitcode = 1;
>  	return 0;

and place the free just before the return here so that we have a single
uwinding exit.

Otherwise this looks good to me.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2 1/4] xfsprogs: xfs_io: fix a memory leak in imap_f

[Dave Chinner]

On Sun, Dec 20, 2015 at 07:33:57AM -0800, Christoph Hellwig wrote:
> > @@ -39,6 +39,8 @@ imap_f(int argc, char **argv)
> > -		if (count == 0)
> > +		if (count == 0) {
> > +			free(t);
> >  			return 0;
> > +		}
> 
> please use a goto out_free; here
> 
> >
> >  		for (i = 0; i < count; i++) {
> >  			printf(_("ino %10llu count %2d mask %016llx\n"),
> >  				(unsigned long long)t[i].xi_startino,
> > @@ -55,6 +59,7 @@ imap_f(int argc, char **argv)
> >  				(unsigned long long)t[i].xi_allocmask);
> >  		}
> >  	}
> > +	free(t);
> >  	perror("xfsctl(XFS_IOC_FSINUMBERS)");
> >  	exitcode = 1;
> >  	return 0;
> 
> and place the free just before the return here so that we have a single
> uwinding exit.

<sigh>

I'll just make the change locally, given I *was* only 15 minutes
away from pushing this into the public repository.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v2 2/4] xfsprogs: fix integer overflow in xlog_find_verify_cycle

[Vivek Trivedi]

Fix unintentional integer overflow in xlog_find_verify_cycle.
Reported by coverity.

Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
---
 libxlog/xfs_log_recover.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libxlog/xfs_log_recover.c b/libxlog/xfs_log_recover.c
index ef7cf68..6116ecd 100644
--- a/libxlog/xfs_log_recover.c
+++ b/libxlog/xfs_log_recover.c
@@ -244,7 +244,7 @@ xlog_find_verify_cycle(
 	xfs_daddr_t	i, j;
 	uint		cycle;
 	xfs_buf_t	*bp;
-	xfs_daddr_t	bufblks;
+	int		bufblks;
 	char		*buf = NULL;
 	int		error = 0;
 
-- 
1.7.9.5

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2 2/4] xfsprogs: fix integer overflow in xlog_find_verify_cycle

[Christoph Hellwig]

On Fri, Dec 11, 2015 at 04:39:01PM +0530, Vivek Trivedi wrote:
> Fix unintentional integer overflow in xlog_find_verify_cycle.
> Reported by coverity.

xfs_daddr_t is a 64-bit integer.  How does replacing it with an int fix
overlows?

xlog_find_verify_cycle originates in the kernel code, so any fix
should probably be done there first.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2 2/4] xfsprogs: fix integer overflow in xlog_find_verify_cycle

[Dave Chinner]

On Sun, Dec 20, 2015 at 07:35:13AM -0800, Christoph Hellwig wrote:
> On Fri, Dec 11, 2015 at 04:39:01PM +0530, Vivek Trivedi wrote:
> > Fix unintentional integer overflow in xlog_find_verify_cycle.
> > Reported by coverity.
> 
> xfs_daddr_t is a 64-bit integer.  How does replacing it with an int fix
> overlows?

xfs_daddr_t is being abused here as a count rather than a disk
address, whereas all the other counts in the function are ints and
so the value can't be more than 32 bits anyway....

> xlog_find_verify_cycle originates in the kernel code, so any fix
> should probably be done there first.

I'll pull all the libxfs changes made first in userspace back into
the kernel - no point making people jump throw hoops just to get a
fix into the libxfs code...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v2 3/4] xfsprogs: mkfs: fix unintentional integer overflow

[Vivek Trivedi]

Fix unintentional integer overflow  in mkfs.
reported by coverity.

Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
---
 mkfs/xfs_mkfs.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 7cba41a..35dafb4 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -2022,7 +2022,7 @@ _("warning: sparse inodes not supported without CRC support, disabled.\n"));
 		 * and the underlying volume is striped, then set rtextblocks
 		 * to the stripe width.
 		 */
-		int		rswidth;
+		__uint64_t	rswidth;
 		__uint64_t	rtextbytes;
 
 		if (!norsflag && !xi.risfile && !(!rtsize && xi.disfile))
-- 
1.7.9.5

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2 3/4] xfsprogs: mkfs: fix unintentional integer overflow

[Christoph Hellwig]

> +++ b/mkfs/xfs_mkfs.c
> @@ -2022,7 +2022,7 @@ _("warning: sparse inodes not supported without CRC support, disabled.\n"));
>  		 * and the underlying volume is striped, then set rtextblocks
>  		 * to the stripe width.
>  		 */
> -		int		rswidth;
> +		__uint64_t	rswidth;
>  		__uint64_t	rtextbytes;

This looks odd.  We initiallize assigned ft.rtswidth (which is an int)
or 0 to it.  I think you want a separate variable for the result of
the  DTOBT(rswidth) statement to make this clear.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2 3/4] xfsprogs: mkfs: fix unintentional integer overflow

[Dave Chinner]

On Sun, Dec 20, 2015 at 07:36:49AM -0800, Christoph Hellwig wrote:
> > +++ b/mkfs/xfs_mkfs.c
> > @@ -2022,7 +2022,7 @@ _("warning: sparse inodes not supported without CRC support, disabled.\n"));
> >  		 * and the underlying volume is striped, then set rtextblocks
> >  		 * to the stripe width.
> >  		 */
> > -		int		rswidth;
> > +		__uint64_t	rswidth;
> >  		__uint64_t	rtextbytes;
> 
> This looks odd.  We initiallize assigned ft.rtswidth (which is an int)
> or 0 to it.  I think you want a separate variable for the result of
> the  DTOBT(rswidth) statement to make this clear.

I don't see any point in doing that. It really doesn't matter that
the variable it is initialised from is only a 32 bit int, and having
yet another variable in mkfs is not going to make the code easier to
understand...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v2 4/4] xfsprogs: xfsdb: remove unnessary checks in process_leaf_node_dir_v2_free

[Vivek Trivedi]

xfs_dir2_free_hdr_t entries are unsigned, so they can't be negative.
remove these unnessary checks in process_leaf_node_dir_v2_free.
Reported by coverity.

Signed-off-by: Vivek Trivedi <t.vivek@samsung.com>
---
 db/check.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/db/check.c b/db/check.c
index 9c1541d..9d986b9 100644
--- a/db/check.c
+++ b/db/check.c
@@ -3073,9 +3073,7 @@ process_leaf_node_dir_v3_free(
 		return;
 	}
 	if (be32_to_cpu(free->hdr.nvalid) > maxent || 
-				be32_to_cpu(free->hdr.nvalid) < 0 ||
 				be32_to_cpu(free->hdr.nused) > maxent || 
-				be32_to_cpu(free->hdr.nused) < 0 ||
 				be32_to_cpu(free->hdr.nused) > 
 					be32_to_cpu(free->hdr.nvalid)) {
 		if (!sflag || v)
-- 
1.7.9.5

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2 4/4] xfsprogs: xfsdb: remove unnessary checks in process_leaf_node_dir_v2_free

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs