xfs
[Top] [All Lists]

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

To: Alex Elder <aelder@xxxxxxx>
Subject: Re: [PATCH 2/2] repair: fix some valgrind reported errors on i686
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 7 Oct 2011 11:06:27 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1317941658.2870.50.camel@doink>
References: <1317862891-3033-1-git-send-email-david@xxxxxxxxxxxxx> <1317862891-3033-3-git-send-email-david@xxxxxxxxxxxxx> <1317903472.3139.30.camel@doink> <20111006221542.GU3159@dastard> <1317941658.2870.50.camel@doink>
User-agent: Mutt/1.5.21 (2010-09-15)
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@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>