Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Why was "goto fail;" added without any other change to that part of the code?

news.ycombinator.com

Post reply on HN
Print view View original

41–50 of 86 posts

Re: Why was "goto fail;" added without any other change to that part of the code?

#41

Post by tptacek »

pessimizer wrote: post #30
matthewmacleod wrote: post #24

Stop this nonsense. There are plainly dozens of reasons that this could have happened accidentally. Literally every single developer has made similar stupid mistake, and literally every organisation has released stupidly broken code. Yes, there should have been code review, static analysis, and testing in place to prevent that. That'll probably start happening, and you can bet that there will be serious discussions i…

>it's hard to imagine a situation where such an action would be worthwhile, given the resources at the disposal of any such actor. This needs more defense. I find it hard to imagine a situation where such a minimal and deniable action with such a massive effect wouldn't be worthwhile.

No, it doesn't need a defense. There is no evidence to back up the idea that the bug was malicious. People that have worked on large C codebases have seen crazier things than this. Routinely.

I understand that Rails developers believe that all the TLS stacks must have dense rspec test coverage and zero-warnings static analysis passes in their builds, but that isn't the reality for any TLS stack. These aren't Rails apps (and, hate to break it to some of you, but your Rails test coverage isn't doing as much for your security as you'd like to think it is).

This is a nutball conspiracy theory, another example of the tech "community" eating its own rather than focusing on anything that would help mitigate state-sponsored surveillance. And it's happening solely in the service of an "exciting" narrative. The people promoting this bullshit conspiracy theory are, by and large, doing it because they want it to be true.

Re: Why was "goto fail;" added without any other change to that part of the code?

#42

Post by jokoon »

that's like telling programmers that Apple doesn't use a repo system, so there could not be any trace of this.

And why don't we hear anything the guy who might have done this mistake IF IT WAS AN ACTUAL MISTAKE ?

I don't like conspiracy theories, but I'd be apple, I'd fire the guy for incompetence. I mean of all the code that has to be secure, you have to check this one.

Re: Why was "goto fail;" added without any other change to that part of the code?

#44

Post by judk »

0x006A wrote: post #27
cjbprime wrote: post #2

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 plausi…

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 dif…

Your search and replace after copy and paste seems to have failed.

Re: Why was "goto fail;" added without any other change to that part of the code?

#45

Post by wmgries »

DanHulton wrote: post #43

Braces, y'all! This is why you always, always, ALWAYS put braces around your if()s. (I'm not religious about what line the open brace goes on, but I will reject any code review that skips the braces entirely.)

Also, you almost never need to use goto.

Re: Why was "goto fail;" added without any other change to that part of the code?

#46

Post by dllthomas »

laureny wrote: post #21
homakov wrote: post #7

More importantly, why Apple doesn't run test suites, basic checks? Test driven development must be everywhere

TDD might be useful for unit tests (even that I'm not convinced of) but not for integration tests. The bug happens very deep inside the handshake protocol and it's extremely hard to test it automatically.

Right, this is a much stronger case for static analysis and warning-free builds. This change caused dead code. If that code was supposed to be dead, comment it out.

Re: Why was "goto fail;" added without any other change to that part of the code?

#47

Post by Dewie »

DanHulton wrote: post #43

Braces, y'all! This is why you always, always, ALWAYS put braces around your if()s. (I'm not religious about what line the open brace goes on, but I will reject any code review that skips the braces entirely.)

Does no one use a pretty printer before committing code? Pretty printing would eliminate any doubt as to what code belongs to what if-statement and the like.

Re: Why was "goto fail;" added without any other change to that part of the code?

#48

Post by stcredzero »

beachstartup wrote: post #36
stcredzero wrote: post #22

Earlier quoted context omitted.

Do you understand diff format?

do YOU understand how diff works? it tracks changes between two points in time, it doesn't track how you got there or what you did in between. he could have easily yanked a goto line 'yy' for use elsewhere or by accident and pasted it by accident with 'p' right after he yanked it. i see two very similar lines, 49 and 53, which were probably copy/pasted and edited. it is completely reasonable to assume he accidentally…

It's just that the code had been around for years by that point, and given that, that's not the story the diff tells.

Re: Why was "goto fail;" added without any other change to that part of the code?

#49

Post by chrismcb »

wmgries wrote: post #45
DanHulton wrote: post #43

Braces, y'all! This is why you always, always, ALWAYS put braces around your if()s. (I'm not religious about what line the open brace goes on, but I will reject any code review that skips the braces entirely.)

Also, you almost never need to use goto.

This isn't goto's fault. A return, or a "finished = true;" or a ton of other code could have had the same result. Using a goto means it is easier to find it than some other similar mistakes.

Re: Why was "goto fail;" added without any other change to that part of the code?

#50

Post by micro-ram »

Dewie wrote: post #47
DanHulton wrote: post #43

Braces, y'all! This is why you always, always, ALWAYS put braces around your if()s. (I'm not religious about what line the open brace goes on, but I will reject any code review that skips the braces entirely.)

Does no one use a pretty printer before committing code? Pretty printing would eliminate any doubt as to what code belongs to what if-statement and the like.

I use Astyle to enforce both braces and indentation automatically. In VC Pre-Build Event add:

Astyle -A1 -j -c -v asterisk.cpp asterisk.h (Sorry asterisks italicize on HN) http://astyle.sourceforge.net/

Post reply on HN