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