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

[Eric Sandeen <sandeen@sandeen.net>]

On 8/26/15 5:19 PM, Eric Sandeen wrote:
> 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...

sorry, then * SEGPERHNK (511), so overflow around 1 billion inodes.

Yeah, ok, closer to a possibility, maybe worth being defensive after
all.  :)

thanks,
-Eric

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

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