[PATCH 0/8] xfsprogs: misc small fixes

[PATCH 0/8] xfsprogs: misc small fixes

[Eric Sandeen]

A handful of fixes from looking over a Coverity scan.

The strncpy fixes (1/8, 5/8) might not be too critical,
but I think they can't hurt.

The libxfs xfs_alloc_arg initialization (2/8) might clash with Dave's
libxfs syncup, it can be dropped if it makes things difficult.
Fixes a real bug though.

Tested by a quick run through xfstests -g auto

Thanks,
-Eric

[PATCH 1/8] libhandle: Guard against string overflow in path_to_fspath()
[PATCH 2/8] libxfs: initialize xfs_alloc_arg structures
[PATCH 3/8] libxfs: fix setup_cursor array allocation
[PATCH 4/8] xfs_repair: Fix free of uninit ptr in xfs_acl_valid() error path
[PATCH 5/8] xfs_fsr: guard against path string overflows
[PATCH 6/8] xfs_fsr: check strdup results properly in initallfs()
[PATCH 7/8] xfs_fsr: fix attribute no_change_count logic
[PATCH 8/8] xfs_fsr: remove extraneous close() in fsrallfs()

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

[PATCH 1/8] libhandle: Guard against string overflow in path_to_fspath()

[Eric Sandeen]

path_to_fspath does a blind strcpy into an array of
MAXPATHLEN; we should be sure to limit this so that it
does not go over the size of the array.

I don't think I see a way to get here today with a too-long
path, but I don't think it'll hurt to be defensive.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 libhandle/handle.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/libhandle/handle.c b/libhandle/handle.c
index b1ec5f2..9a232fa 100644
--- a/libhandle/handle.c
+++ b/libhandle/handle.c
@@ -158,7 +158,8 @@ path_to_fspath(char *path)
 	if (S_ISREG(statbuf.st_mode) || S_ISDIR(statbuf.st_mode))
 		return path;
 
-	strcpy(dirpath, path);
+	strncpy(dirpath, path, MAXPATHLEN);
+	dirpath[MAXPATHLEN-1] = '\0';
 	return dirname(dirpath);
 }
 
-- 
1.7.1

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

[PATCH 2/8] libxfs: initialize xfs_alloc_arg structures

[Eric Sandeen]

This is a merge-to-userspace of kernel commit a0041684
xfs: zero allocation_args on the kernel stack

When calling xfs_alloc_vextent args.userdata was uninitialized,
and if we had args.type==XFS_ALLOCTYPE_START_BNO, this uninit
structure member got tested, leading to some random allocator
behavior.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 libxfs/xfs_alloc.c  |    1 +
 libxfs/xfs_bmap.c   |    3 +++
 libxfs/xfs_ialloc.c |    1 +
 3 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/libxfs/xfs_alloc.c b/libxfs/xfs_alloc.c
index a76512d..831040a 100644
--- a/libxfs/xfs_alloc.c
+++ b/libxfs/xfs_alloc.c
@@ -1788,6 +1788,7 @@ xfs_alloc_fix_freelist(
 	/*
 	 * Initialize the args structure.
 	 */
+	memset(&targs, 0, sizeof(targs));
 	targs.tp = tp;
 	targs.mp = mp;
 	targs.agbp = agbp;
diff --git a/libxfs/xfs_bmap.c b/libxfs/xfs_bmap.c
index 5a626b0..883035e 100644
--- a/libxfs/xfs_bmap.c
+++ b/libxfs/xfs_bmap.c
@@ -2411,6 +2411,7 @@ xfs_bmap_btalloc(
 	 * Normal allocation, done through xfs_alloc_vextent.
 	 */
 	tryagain = isaligned = 0;
+	memset(&args, 0, sizeof(args));
 	args.tp = ap->tp;
 	args.mp = mp;
 	args.fsbno = ap->rval;
@@ -3044,6 +3045,7 @@ xfs_bmap_extents_to_btree(
 	 * Convert to a btree with two levels, one record in root.
 	 */
 	XFS_IFORK_FMT_SET(ip, whichfork, XFS_DINODE_FMT_BTREE);
+	memset(&args, 0, sizeof(args));
 	args.tp = tp;
 	args.mp = mp;
 	args.firstblock = *firstblock;
@@ -3205,6 +3207,7 @@ xfs_bmap_local_to_extents(
 		xfs_buf_t	*bp;	/* buffer for extent block */
 		xfs_bmbt_rec_host_t *ep;/* extent record pointer */
 
+		memset(&args, 0, sizeof(args));
 		args.tp = tp;
 		args.mp = ip->i_mount;
 		args.firstblock = *firstblock;
diff --git a/libxfs/xfs_ialloc.c b/libxfs/xfs_ialloc.c
index 1fcafb6..f97dc4d 100644
--- a/libxfs/xfs_ialloc.c
+++ b/libxfs/xfs_ialloc.c
@@ -232,6 +232,7 @@ xfs_ialloc_ag_alloc(
 					/* boundary */
 	struct xfs_perag *pag;
 
+	memset(&args, 0, sizeof(args));
 	args.tp = tp;
 	args.mp = tp->t_mountp;
 
-- 
1.7.1

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

Re: [PATCH 2/8] libxfs: initialize xfs_alloc_arg structures

[Dave Chinner]

On Sat, Jan 26, 2013 at 04:40:26PM -0600, Eric Sandeen wrote:
> This is a merge-to-userspace of kernel commit a0041684
> xfs: zero allocation_args on the kernel stack
> 
> When calling xfs_alloc_vextent args.userdata was uninitialized,
> and if we had args.type==XFS_ALLOCTYPE_START_BNO, this uninit
> structure member got tested, leading to some random allocator
> behavior.

Got that as part of the kernel-user sync I sent out a couple of
weeks back.

BTW, perhaps we want to get that sync sorted out before shovelling
a bunch of stuff into xfsprogs that will require fixing conflicts in
that sync up?

Chers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 2/8] libxfs: initialize xfs_alloc_arg structures

[Eric Sandeen]

On Jan 27, 2013, at 8:38 PM, Dave Chinner <david@fromorbit.com> wrote:

> On Sat, Jan 26, 2013 at 04:40:26PM -0600, Eric Sandeen wrote:
>> This is a merge-to-userspace of kernel commit a0041684
>> xfs: zero allocation_args on the kernel stack
>> 
>> When calling xfs_alloc_vextent args.userdata was uninitialized,
>> and if we had args.type==XFS_ALLOCTYPE_START_BNO, this uninit
>> structure member got tested, leading to some random allocator
>> behavior.
> 
> Got that as part of the kernel-user sync I sent out a couple of
> weeks back.
> 
> BTW, perhaps we want to get that sync sorted out before shovelling
> a bunch of stuff into xfsprogs that will require fixing conflicts in
> that sync up?
> 
I have no problem with this going 2nd, I can fix up as needed.

-Eric

> Chers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

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

Re: [PATCH 2/8] libxfs: initialize xfs_alloc_arg structures

[Ben Myers]

Hey,

On Sun, Jan 27, 2013 at 09:41:28PM -0500, Eric Sandeen wrote:
> On Jan 27, 2013, at 8:38 PM, Dave Chinner <david@fromorbit.com> wrote:
> 
> > On Sat, Jan 26, 2013 at 04:40:26PM -0600, Eric Sandeen wrote:
> >> This is a merge-to-userspace of kernel commit a0041684
> >> xfs: zero allocation_args on the kernel stack
> >> 
> >> When calling xfs_alloc_vextent args.userdata was uninitialized,
> >> and if we had args.type==XFS_ALLOCTYPE_START_BNO, this uninit
> >> structure member got tested, leading to some random allocator
> >> behavior.
> > 
> > Got that as part of the kernel-user sync I sent out a couple of
> > weeks back.
> > 
> > BTW, perhaps we want to get that sync sorted out before shovelling
> > a bunch of stuff into xfsprogs that will require fixing conflicts in
> > that sync up?
> > 
> I have no problem with this going 2nd, I can fix up as needed.

We need to do another userspace release to resolve the backward compatability
issue for Fugazzi and missing source file in xfsprogs for Arkadiusz before
shovelling in that ueber patch.  If any of these patches are bug fixes
appropriate for a release within the next week or so, give a heads up.

Thanks,
	Ben

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

Re: [PATCH 2/8] libxfs: initialize xfs_alloc_arg structures

[Mark Tinguely]

On 01/26/13 16:40, Eric Sandeen wrote:
> This is a merge-to-userspace of kernel commit a0041684
> xfs: zero allocation_args on the kernel stack
>
> When calling xfs_alloc_vextent args.userdata was uninitialized,
> and if we had args.type==XFS_ALLOCTYPE_START_BNO, this uninit
> structure member got tested, leading to some random allocator
> behavior.
>
> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
> ---

I remember seeing this in Dave's:

	[UBER-PATCH, RFC] xfsprogs: sync libxfs to 3.8-rc2 kernel code

http://patchwork.xfs.org/patch/4683/

--Mark.

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

[PATCH 3/8] libxfs: fix setup_cursor array allocation

[Eric Sandeen]

setup_cursor() wants an array of xfs_agbno_t's, but
it allocated a multiple of *pointers* to xfs_agbno_t's.
xfs_agbno_t is 4 bytes, so this is harmless other than
allocating twice as much memory as needed on a 64-bit
machine.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 repair/phase5.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/repair/phase5.c b/repair/phase5.c
index 7d5cd49..1f71cac 100644
--- a/repair/phase5.c
+++ b/repair/phase5.c
@@ -206,7 +206,7 @@ setup_cursor(xfs_mount_t *mp, xfs_agnumber_t agno, bt_status_t *curs)
 
 	ASSERT(big_extent_len > 0);
 
-	if ((curs->btree_blocks = malloc(sizeof(xfs_agblock_t *)
+	if ((curs->btree_blocks = malloc(sizeof(xfs_agblock_t)
 					* big_extent_len)) == NULL)
 		do_error(_("could not set up btree block array\n"));
 
-- 
1.7.1

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

[PATCH 4/8] xfs_repair: Fix free of uninit ptr in xfs_acl_valid() error path

[Eric Sandeen]

This error path:

xfs_acl_valid(xfs_acl_disk_t *daclp)
{
	xfs_acl_t	*aclp;
...
	if (daclp == NULL)
		goto acl_invalid;
...
acl_invalid:
	free(aclp);

attempts to free garbage; set it to NULL on init to make
it safe.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 repair/attr_repair.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/repair/attr_repair.c b/repair/attr_repair.c
index bab65b1..ec7f4a3 100644
--- a/repair/attr_repair.c
+++ b/repair/attr_repair.c
@@ -1004,7 +1004,7 @@ process_attributes(
 static int
 xfs_acl_valid(xfs_acl_disk_t *daclp)
 {
-	xfs_acl_t	*aclp;
+	xfs_acl_t	*aclp = NULL;
 	xfs_acl_entry_t *entry, *e;
 	int user = 0, group = 0, other = 0, mask = 0, mask_required = 0;
 	int i, j;
-- 
1.7.1

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

[PATCH 5/8] xfs_fsr: guard against path string overflows

[Eric Sandeen]

gettmpname() and getparent() blindly copy strings
into a target array; be sure we limit the copy to
the size of the target and null terminate it.

I don't see a way to get here with a too-long name,
since most paths try to open or stat the file already,
but it can't hurt to be defensive.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fsr/xfs_fsr.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
index 2db2224..843f57d 100644
--- a/fsr/xfs_fsr.c
+++ b/fsr/xfs_fsr.c
@@ -1452,7 +1452,8 @@ gettmpname(char *fname)
 
 	sprintf(sbuf, "/.fsr%d", getpid());
 
-	strcpy(buf, fname);
+	strncpy(buf, fname, PATH_MAX);
+	buf[PATH_MAX] = '\0';
 	ptr = strrchr(buf, '/');
 	if (ptr) {
 		*ptr = '\0';
@@ -1476,7 +1477,8 @@ getparent(char *fname)
 	static char	buf[PATH_MAX+1];
 	char		*ptr;
 
-	strcpy(buf, fname);
+	strncpy(buf, fname, PATH_MAX);
+	buf[PATH_MAX] = '\0';
 	ptr = strrchr(buf, '/');
 	if (ptr) {
 		if (ptr == &buf[0])
-- 
1.7.1

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

[PATCH 6/8] xfs_fsr: check strdup results properly in initallfs()

[Eric Sandeen]

initallfs() does 2 strdups, but then checks the result
of one of them twice, rather then checking each one.
Fix this.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fsr/xfs_fsr.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
index 843f57d..b5aa3db 100644
--- a/fsr/xfs_fsr.c
+++ b/fsr/xfs_fsr.c
@@ -470,10 +470,14 @@ initallfs(char *mtab)
 		fs->dev = strdup(mp->mnt_fsname);
 		fs->mnt = strdup(mp->mnt_dir);
 
-		if (fs->mnt == NULL || fs->mnt == NULL) {
+		if (fs->dev == NULL) {
 			fsrprintf(_("strdup(%s) failed\n"), mp->mnt_fsname);
 			exit(1);
 		}
+		if (fs->mnt == NULL) {
+			fsrprintf(_("strdup(%s) failed\n"), mp->mnt_dir);
+			exit(1);
+		}
 		mi++;
 		fs++;
 	}
-- 
1.7.1

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

[PATCH 7/8] xfs_fsr: fix attribute no_change_count logic

[Eric Sandeen]

As it stands today, if no_change_count++ isn't > 10,
we will reset it to 0.  There's no way to get above 1
(let alone 10) so this isn't working as intended.

If we see progress (last_forkoff != tbstat.bs_forkoff)
*then* we sould reset the no_change_count counter to 0.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fsr/xfs_fsr.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
index b5aa3db..5eaabb5 100644
--- a/fsr/xfs_fsr.c
+++ b/fsr/xfs_fsr.c
@@ -1099,8 +1099,8 @@ fsr_setup_attr_fork(
 		if (last_forkoff == tbstat.bs_forkoff) {
 			if (no_change_cnt++ > 10)
 				break;
-		}
-		no_change_cnt = 0;
+		} else /* progress! */
+			no_change_cnt = 0;
 		last_forkoff = tbstat.bs_forkoff;
 
 		/* work out which way to grow the fork */
-- 
1.7.1

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

[PATCH 8/8] xfs_fsr: remove extraneous close() in fsrallfs()

[Eric Sandeen]

After the fork(), the parent does a close(fd); on an
fd which is long-since closed (prior to the while loop).
Just remove it.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fsr/xfs_fsr.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
index 5eaabb5..3d017ca 100644
--- a/fsr/xfs_fsr.c
+++ b/fsr/xfs_fsr.c
@@ -627,7 +627,6 @@ fsrallfs(char *mtab, int howlong, char *leftofffile)
 			break;
 		default:
 			wait(&error);
-			close(fd);
 			if (WIFEXITED(error) && WEXITSTATUS(error) == 1) {
 				/* child timed out & did fsrall_cleanup */
 				exit(0);
-- 
1.7.1

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

Re: [PATCH 0/8] xfsprogs: misc small fixes

[Mark Tinguely]

On 01/26/13 16:40, Eric Sandeen wrote:
> A handful of fixes from looking over a Coverity scan.
>
> The strncpy fixes (1/8, 5/8) might not be too critical,
> but I think they can't hurt.
>
> The libxfs xfs_alloc_arg initialization (2/8) might clash with Dave's
> libxfs syncup, it can be dropped if it makes things difficult.
> Fixes a real bug though.
>
> Tested by a quick run through xfstests -g auto
>
> Thanks,
> -Eric
>
> [PATCH 1/8] libhandle: Guard against string overflow in path_to_fspath()
> [PATCH 2/8] libxfs: initialize xfs_alloc_arg structures
> [PATCH 3/8] libxfs: fix setup_cursor array allocation
> [PATCH 4/8] xfs_repair: Fix free of uninit ptr in xfs_acl_valid() error path
> [PATCH 5/8] xfs_fsr: guard against path string overflows
> [PATCH 6/8] xfs_fsr: check strdup results properly in initallfs()
> [PATCH 7/8] xfs_fsr: fix attribute no_change_count logic
> [PATCH 8/8] xfs_fsr: remove extraneous close() in fsrallfs()
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

patch 1, 3-8 do not interfere with the UBER patch. Patches look good to me.

Reviewed-by: Mark Tinguely <tinguely@sgi.com>

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

Re: [PATCH 0/8] xfsprogs: misc small fixes

[Mark Tinguely]

On 01/26/13 16:40, Eric Sandeen wrote:
> A handful of fixes from looking over a Coverity scan.
>
> The strncpy fixes (1/8, 5/8) might not be too critical,
> but I think they can't hurt.
>
> The libxfs xfs_alloc_arg initialization (2/8) might clash with Dave's
> libxfs syncup, it can be dropped if it makes things difficult.
> Fixes a real bug though.
>
> Tested by a quick run through xfstests -g auto

I committed the following patches of yours to OSS xfsprogs:

commit b3a5164e6e474f03cd2d5e6d5168d4ced3d3282b
Author: Eric Sandeen <sandeen@sandeen.net>
Date:   Fri Jan 25 21:10:22 2013 +0000

     xfsprogs: Fix possible unallocated memory access in fiemap

Coverity flagged series patches 1, 3-8:

commit 55d1fd480b7328f0c51a7de5ed64ea371927c933
Author: Eric Sandeen <sandeen@redhat.com>
Date:   Sat Jan 26 22:40:25 2013 +0000

     libhandle: Guard against string overflow in path_to_fspath()

commit 2c2e994d0a1b972d020d7633202bf2f4cfc9f91b
Author: Eric Sandeen <sandeen@redhat.com>
Date:   Sat Jan 26 22:40:27 2013 +0000

     libxfs: fix setup_cursor array allocation

commit 50c79b8df93c45495e81c15aca65abde469f1565
Author: Eric Sandeen <sandeen@redhat.com>
Date:   Sat Jan 26 22:40:28 2013 +0000

     xfs_repair: Fix free of uninit ptr in xfs_acl_valid() error path

commit 6063fecad207d13981dea3e20f89f7ac4c3fb26f
Author: Eric Sandeen <sandeen@redhat.com>
Date:   Sat Jan 26 22:40:29 2013 +0000

     xfs_fsr: guard against path string overflows

commit 758bcc928858b85743d2f466f41c80cb40b93214
Author: Eric Sandeen <sandeen@redhat.com>
Date:   Sat Jan 26 22:40:30 2013 +0000

     xfs_fsr: check strdup results properly in initallfs()

commit ff85ea3f1456f20ba21a67c652f245da6360db15
Author: Eric Sandeen <sandeen@redhat.com>
Date:   Sat Jan 26 22:40:31 2013 +0000

     xfs_fsr: fix attribute no_change_count logic

commit a0a28a6ac3005ec320ef73066e57d9ae1fd41754
Author: Eric Sandeen <sandeen@redhat.com>
Date:   Sat Jan 26 22:40:32 2013 +0000

     xfs_fsr: remove extraneous close() in fsrallfs()

All these patches look and test okay. None of them interferes with 
Dave's xfsprogs "UBER" kernel/user space sync patch.

Thank-you,

--Mark.

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