xfs
[Top] [All Lists]

Re: [PATCH] xfsprogs: Fix possible unallocated memory access in fiemap

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfsprogs: Fix possible unallocated memory access in fiemap
From: Lukáš Czerner <lczerner@xxxxxxxxxx>
Date: Thu, 13 Dec 2012 10:30:49 +0100 (CET)
Cc: Lukas Czerner <lczerner@xxxxxxxxxx>, xfs@xxxxxxxxxxx, hch@xxxxxxxxxxxxx
In-reply-to: <20121212231245.GY16353@dastard>
References: <1355319059-12111-1-git-send-email-lczerner@xxxxxxxxxx> <20121212231245.GY16353@dastard>
User-agent: Alpine 2.00 (LFD 1167 2008-08-23)
On Thu, 13 Dec 2012, Dave Chinner wrote:

> Date: Thu, 13 Dec 2012 10:12:45 +1100
> From: Dave Chinner <david@xxxxxxxxxxxxx>
> To: Lukas Czerner <lczerner@xxxxxxxxxx>
> Cc: xfs@xxxxxxxxxxx, hch@xxxxxxxxxxxxx
> Subject: Re: [PATCH] xfsprogs: Fix possible unallocated memory access in
>     fiemap
> 
> On Wed, Dec 12, 2012 at 02:30:59PM +0100, Lukas Czerner wrote:
> > Currently we could access unallocated memory in fiemap because we're
> > using uninitialized variable 'fiemap' in fiemap_f(). In fact this has
> > been spotted on x390s machine where xfs_io would segfault.
> > 
> > The problem happens in the for cycle which seems to be intended to
> > compute the header item spacing. However at that point the fiemap
> > structure has just been allocated and does not contain any extents
> > yet, so it is entirely useless and it never actually worked.
> > 
> > This patch removes this dead code mentioned above which also fixes
> > the possible unallocated memory access.
> 
> I think the correct fix is to move the formatting calculation to
> after the first fiemap call. The formatting is actually useful
> because it calculates column widths that make sure output is fairly
> nicely aligned, and that is definitely of value when you are looking
> at output thousands of extents long...

It might be, but simply moving the calculation is not enough. You
would have to store all the extents from the while cycle and then
walk the extents again to print them. Given that it actually never
worked before and no one seemed eager to fix this, I guess no-one
really cares (nor do I:)).

I think as a quick fix this is enough, we can always add something
more sophisticated later.

-Lukas


> 
> Cheers,
> 
> Dave.
> 

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