Re: [PATCH V3] xfsdump: prevent segfault in cb_add_inogrp

[Eric Sandeen <sandeen@sandeen.net>]

On 8/26/15 5:29 PM, rjohnston@sgi.com wrote:
> The call to memset will segfault because the offset for the first
> parameter is done twice. We are using pointer math to do the
> calculation.
> 
> The first time is when calculating oldsize, the size of i2gseg_t
> is accounted for.
> 	oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t);
> Then in the call to memset, oldsize is again multiplied by the size
> of i2gmap_t.
> 	memset(inomap.i2gmap + oldsize, ...)
> 	
> i2gmap holds the used inodes in each chunk. When there are 2^31 chunk
> entries, it could describe 2^31 (1 inode/chunk)- 2^40 (64 inodes/chunk).
> 
> With 100s of millions of inodes there are enough entries to wrap the
> 32 bit variable oldsize.
> 
> Adding a bounds check (numsegs < 0) and switching to use array
> index notation instead of calculating the pointer address twice
> would resolve this issue. The unneeded local variable oldsize
> can be removed as well.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

thanks,
-Eric

> ---
> V3:
> Per review comments:
> 	add a bounds check after calculating numsegs:
> 		if (numsegs < 0)
> 			return -1;
> 
> 	Remove the description that does not match the code and
> 	leave numsegs as is (intgen_t) 
> 
>  dump/inomap.c |    8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> Index: b/dump/inomap.c
> ===================================================================
> --- a/dump/inomap.c
> +++ b/dump/inomap.c
> @@ -1126,13 +1126,14 @@ cb_add_inogrp( void *arg1, intgen_t fsfd
>  
>  		if (lastsegp->hnkoff == inomap.hnkmaplen) {
>  			intgen_t numsegs;
> -			intgen_t oldsize;
>  
>  			inomap.hnkmaplen++;
>  			inomap.hnkmap = (hnk_t *)
>  				realloc(inomap.hnkmap, inomap.hnkmaplen * HNKSZ);
>  
>  			numsegs = inomap.hnkmaplen * SEGPERHNK;
> +			if (numsegs < 0)
> +				return -1;
>  			inomap.i2gmap = (i2gseg_t *)
>  				realloc(inomap.i2gmap, numsegs * sizeof(i2gseg_t));
>  
> @@ -1140,10 +1141,7 @@ cb_add_inogrp( void *arg1, intgen_t fsfd
>  				return -1;
>  
>  			/* zero the new portion of the i2gmap */
> -			oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t);
> -
> -			memset(inomap.i2gmap + oldsize,
> -			       0,
> +			memset(&inomap.i2gmap[numsegs - SEGPERHNK], 0,
>  			       SEGPERHNK * sizeof(i2gseg_t));
>  		}
>  
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

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