Re: [v2] ext4: fix possible non-initialized variable

[Eric Sandeen <sandeen@redhat.com>]

On 9/15/12 1:30 PM, Theodore Ts'o wrote:
> On Mon, Sep 10, 2012 at 11:55:48AM -0000, Carlos Maiolino wrote:
>> htree_dirblock_to_tree() declares a non-initialized 'err' variable,
>> which is passed as a reference to another functions expecting them
>> to set this variable with thei error codes.  It's passed to
>> ext4_bread(), which then passes it to ext4_getblk(). If
>> ext4_map_blocks() returns 0 due to a lookup failure, leaving the
>> ext4_getblk() buffer_head uninitialized, it will make ext4_getblk()
>> return to ext4_bread() without initialize the 'err' variable, and
>> ext4_bread() will return to htree_dirblock_to_tree() with this
>> variable still uninitialized.
> 
> Hi Carlos,
> 
> Thanks for noticing this problem!
> 
> In the case where there is no block mapping for a particular block,
> ext4_bread() can return NULL, and with your patch, *err will now be
> zero instead of some uninitialized value.  That's an improvement, and
> in the case of htree_dirblock_to_tree(), when we return 0 as an
> "error", the caller will do the right thing.

Hm, sorry, I had counseled Carlos to do that.  I figured a bmap
call w/o create set, returning a NULL bh was perfectly valid - it simply
means that it's not mapped there, right? - so a 0 retval made sense
to me.

> But there are other places where when ext4_bread() returns NULL with
> err set to 0, the function ends up returning err, i.e., in ext4_add_entry:
> 
> 		bh = ext4_bread(handle, dir, block, 0, &retval);
> 		if(!bh)
> 			return retval;
> 
> ... which will cause the caller of ext4_add_entry() to think that the
> function had succeeded.
> 
> In the case of directories, there is never supposed to "holes" in
> directories, so the right thing to do is to check to see if err = 0
> and in that case to call ext4_error() to mark the file system as being
> inconsistent, and then returning some kind of error like -EIO.

Hm good point.  Yeah, callers need to understand what that means.

> So your patch is an improvement, but I'm worried that there were cases
> where we had been returning some uninitialized, non-zero stack
> garbage, we had been serendipously treating the case of a directory
> hole as an "error", now we we consider that situation as a "success"
> even though the calling function (such as ext4_add_entry) had not
> completed its processing.  Which is a very long-winded way of saying
> that we need to audit all of the functions which call ext4_bread() so
> that they do the right thing when ext4_bread() returns NULL and err is
> set to zero.

I agree with that.  :)

-Eric

> Regards,
> 
> 						- Ted
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>