netdev - Re: removing gotos considered harmful...

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 03 Jan 2007 17:25:48 -0800 (PST)
From:	David Miller <davem@...emloft.net>
To:	gerrit@....abdn.ac.uk
Cc:	netdev@...r.kernel.org, arnaldo.melo@...il.com
Subject: Re: removing gotos considered harmful...

From: Gerrit Renker <gerrit@....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'.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html