Re: removing gotos considered harmful...

[David Miller <davem@davemloft.net>]

From: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Date: Wed, 3 Jan 2007 08:08:08 +0000

> However, I would also like to plead non-guilty. I have checked - what you
> are quoting is not the original patch. If you look at e.g. 2.6.17-mm1, the 
> previous code had the form (this is copied from 2.6.17-mm1 original):
> 
> 			size = 0;
> 			sk_for_each(sk2, node, list)
> 				if (++size >= best_size_so_far)
> 					goto next;
> 			best_size_so_far = size;
> 			best = result;
> 		next:;
> 
> |  and this got converted into:
> |  
> |  			sk_for_each(sk2, node, head)
> |  				if (++size < best_size_so_far) {
> |  					best_size_so_far = size;
> |  					best = result;
> |  				}
> |  
> |  Which does something very very different from the original.
>
> ===> Sorry, I fail to see where the two differ. They have the same postcondition
>      upon loop exit; sk2, node, size, and head are not referenced anywhere in the 
>      code that follows.
>      

Please go buy a pair of glasses then :-)

They are not at all the same.  Consider in what circumstances the two
variables "best_size_so_far" and "best" get updated in the two cases,
it's massively different.

You _ALWAYS_ update those two variables in your version if the loop
executes at least once, that's wrong and that's not what the original
code was trying to do.

It ONLY wants to update those two variables when we walk
a complete hash chain which is smaller than "best_size_so_far".

The fact that you continue to try and defend your version shows
that you really had no idea what you were doing when you made this
change.

You added an exploitable hole to our UDP protocol implementation
because you didn't understand this snippet of code and wanted to
'clean up the logic'.