3 ms·

It's not true that there was no other change in the code -- there was a change eight lines earlier. It could be a mismerge of the latest upstream code. There

It's not true that there was no other change in the code -- there was a change eight lines earlier. It could be a mismerge of the latest upstream code. There was refactoring nearby, and people can make stupid mistakes while refactoring. I disagree that there's no "reasonable explanation", and so do most other people who've looked at it. That doesn't mean it wasn't malicious, but it means that it has reasonably plausible deniability. (Of course, if you were being malicious, plausible deniability might be important to you.)

lyngvi 13y ago
++ on mismerge. The change likely won't impact the output binary - compiler should issue a warning, which devs might ignore; then the optimizer would remove it as unreachable. Perhaps the developer deliberately allowed that mismerge to give himself an aura of incompetence/sloppiness, enhancing his plausible deniability when it is discovered that one of his other changes led to an insidious vulnerability. But more likely it was just an accident - if he were malicious, the effect of bringing attention to his code would be highly undesirable.
- shawabawa3 13y ago
  > The change likely won't impact the output binary - compiler should issue a warning, which devs might ignore; then the optimizer would remove it as unreachable. It's not unreachable - it always executes if the if condition is false.
  - cjbprime 13y ago
    The unconditional "goto out" makes the code beneath that condition but before the "out:" label unreachable code.
  - lyngvi 13y ago
    D'oh. That's what I get for not paying attention and looking at the indentation instead of for braces. Still, my bet's on mismerge first - though it'd be worth reviewing that committer's other changes.
0x006A 13y ago
yes the code above looks like a search and replace of ReadyHash(&SSLHashSHA1, &hashCtx, ctx)) with ReadyHash(&SSLHashSHA1, &hashCtx, ctx)) but while doing that its unlikely you touch the goto line. I'm not saying there is no reasonable explanation, just that I am not able to come up with one. Many explanations I have seen so far, require additional code changes in that block and make no sense to me looking at the diff.
- judk 13y ago
  Your search and replace after copy and paste seems to have failed.
0x006A 13y ago
mismerge looks like a possible explanation, do you know what the upstream project/codebase for this is? Would be interesting to look at that.

lyngvi 13y ago

++ on mismerge. The change likely won't impact the output binary - compiler should issue a warning, which devs might ignore; then the optimizer would remove it as unreachable. Perhaps the developer deliberately allowed that mismerge to give himself an aura of incompetence/sloppiness, enhancing his plausible deniability when it is discovered that one of his other changes led to an insidious vulnerability. But more likely it was just an accident - if he were malicious, the effect of bringing attention to his code would be highly undesirable.

shawabawa3 13y ago
> The change likely won't impact the output binary - compiler should issue a warning, which devs might ignore; then the optimizer would remove it as unreachable. It's not unreachable - it always executes if the if condition is false.
- cjbprime 13y ago
  The unconditional "goto out" makes the code beneath that condition but before the "out:" label unreachable code.
- lyngvi 13y ago
  D'oh. That's what I get for not paying attention and looking at the indentation instead of for braces. Still, my bet's on mismerge first - though it'd be worth reviewing that committer's other changes.

0x006A 13y ago

yes the code above looks like a search and replace of ReadyHash(&SSLHashSHA1, &hashCtx, ctx)) with ReadyHash(&SSLHashSHA1, &hashCtx, ctx)) but while doing that its unlikely you touch the goto line. I'm not saying there is no reasonable explanation, just that I am not able to come up with one. Many explanations I have seen so far, require additional code changes in that block and make no sense to me looking at the diff.

judk 13y ago
Your search and replace after copy and paste seems to have failed.

0x006A 13y ago

mismerge looks like a possible explanation, do you know what the upstream project/codebase for this is? Would be interesting to look at that.