Messages in this thread Patch in this message | | | Date | Wed, 19 May 2004 22:06:03 +0100 | From | Matthew Wilcox <> | Subject | Re: [Ext2-devel] Re: [PATCH] use-before-uninitialized value in ext3(2)_find_ goal |
| |
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |