From: Gerrit Renker <gerrit@erg.abdn.ac.uk>
To: David Miller <davem@davemloft.net>
Cc: Herbert Xu <herbert@gondor.apana.org.au>, netdev@vger.kernel.org
Subject: Re: removing gotos considered harmful...
Date: Thu, 4 Jan 2007 10:02:59 +0000 [thread overview]
Message-ID: <200701041002.59906@strip-the-willow> (raw)
In-Reply-To: <20070103.172548.17866275.davem@davemloft.net>
| > 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'.
|
|
You are right, I made a stupid error by considering a single construct out of context.
I only understood fully what you were saying above after doing a lengthy paper-and-pencil
analysis of the entire algorithm: the exploit is in the assignment of `best', I was arguing
about `best_size_so_far', which is of no consequence here.
I apologise for the regression that this caused - in future submissions I make sure that I
do the paper and pencil analysis before. Thanks for patience with the explanation.
prev parent reply other threads:[~2007-01-04 10:02 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2006-12-22 20:04 removing gotos considered harmful David Miller
2006-12-22 21:11 ` Arnaldo Carvalho de Melo
2007-01-03 8:08 ` Gerrit Renker
2007-01-04 0:35 ` Herbert Xu
2007-01-04 1:25 ` David Miller
2007-01-04 10:02 ` Gerrit Renker [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=200701041002.59906@strip-the-willow \
--to=gerrit@erg.abdn.ac.uk \
--cc=davem@davemloft.net \
--cc=herbert@gondor.apana.org.au \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.