Re: [Ext2-devel] Re: [PATCH] use-before-uninitialized value in ext3(2)_find_ goal

[Matthew Wilcox <willy@debian.org>]

On Wed, May 19, 2004 at 12:53:28PM -0700, Chris Wright wrote:
> I know it's a slightly bigger patch, but would it make sense to just enforce
> this as part of api?  Just a thought...(patch untested)

No, that doesn't work.  Look:

reread:
	...

        if (ext2_find_goal(inode, iblock, chain, partial, &goal) < 0)
                goto changed;

changed:
        while (partial > chain) {
                brelse(partial->bh);
                partial--;
        }
        goto reread;

So it's spaghetti code that can modify goal.  Yuck.

5 labels in one function?  3 backwards jumps?  Disgusting.

> --- linux-2.6.6-mm3/fs/ext2/inode.c~goal	2004-05-09 19:32:00.000000000 -0700
> +++ linux-2.6.6-mm3/fs/ext2/inode.c	2004-05-19 12:27:11.968054560 -0700
> @@ -366,6 +366,7 @@ static inline int ext2_find_goal(struct 
>  				 unsigned long *goal)
>  {
>  	struct ext2_inode_info *ei = EXT2_I(inode);
> +	unsigned long _goal = 0;
>  	write_lock(&ei->i_meta_lock);
>  	if (block == ei->i_next_alloc_block + 1) {
>  		ei->i_next_alloc_block++;
> @@ -377,10 +378,11 @@ static inline int ext2_find_goal(struct 
>  		 * failing that at least try to get decent locality.
>  		 */
>  		if (block == ei->i_next_alloc_block)
> -			*goal = ei->i_next_alloc_goal;
> -		if (!*goal)
> -			*goal = ext2_find_near(inode, partial);
> +			_goal = ei->i_next_alloc_goal;
> +		if (!_goal)
> +			_goal = ext2_find_near(inode, partial);
>  		write_unlock(&ei->i_meta_lock);
> +		*goal = _goal;
>  		return 0;
>  	}
>  	write_unlock(&ei->i_meta_lock);
> --- linux-2.6.6-mm3/fs/ext3/inode.c~goal	2004-05-13 11:19:42.000000000 -0700
> +++ linux-2.6.6-mm3/fs/ext3/inode.c	2004-05-19 12:25:48.441752488 -0700
> @@ -461,6 +461,7 @@ static int ext3_find_goal(struct inode *
>  			  Indirect *partial, unsigned long *goal)
>  {
>  	struct ext3_inode_info *ei = EXT3_I(inode);
> +	unsigned long _goal = 0;
>  	/* Writer: ->i_next_alloc* */
>  	if (block == ei->i_next_alloc_block + 1) {
>  		ei->i_next_alloc_block++;
> @@ -474,9 +475,10 @@ static int ext3_find_goal(struct inode *
>  		 * failing that at least try to get decent locality.
>  		 */
>  		if (block == ei->i_next_alloc_block)
> -			*goal = ei->i_next_alloc_goal;
> -		if (!*goal)
> -			*goal = ext3_find_near(inode, partial);
> +			_goal = ei->i_next_alloc_goal;
> +		if (!_goal)
> +			_goal = ext3_find_near(inode, partial);
> +		*goal = _goal;
>  		return 0;
>  	}
>  	/* Reader: end */
> 
> 
> -------------------------------------------------------
> This SF.Net email is sponsored by: SourceForge.net Broadband
> Sign-up now for SourceForge Broadband and get the fastest
> 6.0/768 connection for only $19.95/mo for the first 3 months!
> http://ads.osdn.com/?ad_id=2562&alloc_id=6184&op=click
> _______________________________________________
> Ext2-devel mailing list
> Ext2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/ext2-devel

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain