Re: [PATCH 2/2] repair: fix some valgrind reported errors on i686

[Dave Chinner <david@fromorbit.com>]

On Thu, Oct 06, 2011 at 05:54:18PM -0500, Alex Elder wrote:
> On Fri, 2011-10-07 at 09:15 +1100, Dave Chinner wrote:
> > On Thu, Oct 06, 2011 at 07:17:52AM -0500, Alex Elder wrote:
> > > On Thu, 2011-10-06 at 12:01 +1100, Dave Chinner wrote:
> > > > @@ -218,6 +244,15 @@ blkmap_grow(
> > > >  	}
> > > >  
> > > >  	blkmap->naexts += 4;
> > > 
> > > The check needs to go *before* you update naexts.
> > 
> > It's in the right place - adding 4 to the unchecked extent count can
> > push it over the limit, so we have to check it after adding 4 to it.
> 
> My point was that I'd rather see the check be whether it *will*
> overflow, rather than allowing it to overflow.
> 
> The other reason though, even if you do the calculation
> at that spot, is that you shouldn't update what blkmap
> records as the number of allocated extents unless you
> have actually updated it the amount of allocated memory.
> As it stands, you will have updated blkmap->naexts, then
> if it overflows you return before actually attempting
> to reallocate.
> 
> In other words, blkmap->naexts should reflect the actual
> amount of memory allotted in the blkmap->exts array, which
> is not going to be the case if it returns early (and this
> can be avoided).

Fair point.

And just checking the single caller of blkmap_grow indicates that it
doesn't handle allocation failure *at all*, so I need to fix that as
well.

> > IOWs, we can't use BLKMAP_SIZE to detect an overflow, because it
> > is the code that overflows...
> 
> I understand that argument.  I'm in a hurry at the moment so
> I haven't thought through this very well right now.
> 
> But if you do have BLKMAP_NENTS_MAX, you could check nex
> against that before attempting to use BLKMAP_SIZE to
> compute how many bytes need to be allocated.

Yup, that's what the patch does, just without the BLKMAP_NENTS_MAX
macro. Which I've already added. ;)

FWIW, because the extent count fields are all signed ints,
overflow checks also need to check for values <= 0. So I've added
that too.

And I think now I need to split this into separate patches for each
issue, as there are now about 5 different problems this one patch
fixes.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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