On Thu, Dec 13, 2012 at 10:30:49AM +0100, Lukáš Czerner wrote: > 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. No, that's not necessary. it's best effort, so setting up the formatting based on the first set of extents extracted is sufficient. This is all the bmap command does, an dit works just fine for 99.9% of cases out there. > 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've actually noticed it, and simply used the bmap command instead because it formats correctly. So, yes, I care about it working properly. > I think as a quick fix this is enough, we can always add something > more sophisticated later. I'd prefer we fix it to work as it was intended to work (i.e. same as the bmap command), not slap a bandaid on it and justify it by saying "but it never worked". Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
