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@xxxxxxx 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@xxxxxxxxxxx > http://oss.sgi.com/mailman/listinfo/xfs > _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs