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

[Eric Sandeen <sandeen@sandeen.net>]

On 8/26/15 4:57 PM, Rich Johnston wrote:
> 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;

probably fine, but probably not really necessary.

numsegs = inomap.hnkmaplen * SEGPERHNK;

hnkmaplen initializes as:

inomap.hnkmaplen = (igrpcnt + SEGPERHNK - 1) / SEGPERHNK;

>From my prior email, if I was right,

> so I guess that means that if we have more than 2^31 inode groups,
> i.e. 2^31 * 256 = 500 billion inodes, (int) igrpcnt could overflow.

so unless you have > 500 billion inodes, it's not going to be a problem...

I'd just focus on the single problem at hand (extending a pointer
to an array by number of bytes, not number of elements) and leave it
at that.

-Eric

> 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
> 

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