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

[Rich Johnston <rjohnston@sgi.com>]

Dave,

On 08/26/2015 04:37 PM, Dave Chinner wrote:
> On Wed, Aug 26, 2015 at 12:36:42PM -0500, 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.
>>
>> 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.
>>
Per your other comment I will add a bounds check after calculating numsegs:
	if (numsegs < 0)
		return -1;

The description above will change to:

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.

>> numsegs is used to calculate an array index, change it from a
>> signed int (intgen_t) to an unsigned (uint32_t).
>
I will remove the above description and leave it as is (intgen_t)
> Description does not match code:
>
>> -			intgen_t numsegs;
>> -			intgen_t oldsize;
>> +			int32_t numsegs;
>
> It's still a signed int here. And, really, just a plain old "int" is
> fine here.
>
> Cheers,
>
> Dave.
>

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