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

11–20 of 86 posts

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

#11

Post by Cthulhu_ »

mistercow wrote: post #6

A lot of code editors (Xcode doesn't by default, but you can easily configure it to) have a keyboard shortcut for duplicating the current line. I have plenty of times accidentally made changes at the wrong place in a file because I was mistaken about where the cursor was. The lesson is to always make sure to look at your diffs before you commit your changes.

And to have someone else look at your diffs and sign off on them before it's merged into master.

Google / Chrome does this, and so should everyone else, even more so if they're dealing with security.

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

#12

Post by beachstartup »

i do this all the time in vim. all it takes is one extra keystroke 'p' to do this.

if i were to take a guess, the programmer first typed out all the if conditional expressions, then went in and pasted 'goto fail' out of a buffer after each one, and accidentally did it twice there.

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

#13

Post by mistercow »

Cthulhu_ wrote: post #11
mistercow wrote: post #6

A lot of code editors (Xcode doesn't by default, but you can easily configure it to) have a keyboard shortcut for duplicating the current line. I have plenty of times accidentally made changes at the wrong place in a file because I was mistaken about where the cursor was. The lesson is to always make sure to look at your diffs before you commit your changes.

And to have someone else look at your diffs and sign off on them before it's merged into master. Google / Chrome does this, and so should everyone else, even more so if they're dealing with security.

That's a good measure too, although I think with a mistake like this, the person writing the code might be more likely to spot the mistake than someone else.

It also makes me wonder if the diff display shouldn't have a specific notation for duplicated lines, since they are a special case that can easily be mistaken for a trivial change to a line with the way diffs are typically shown.

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

#14

Post by danpalmer »

homakov wrote: post #7

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

Test driven development is actually quite uncommon in many parts of the industry.

Sure the startup culture and new breed (new bubble?) tech companies mostly emphasise it, but among older companies it's much less common for a few reasons:

1. It just wasn't considered normal practice until fairly recently. 2. They often use 'lower level' languages, and as @raverbashing mentions, they're more difficult to test. 3. The companies are much slower moving, so arguably have less of a risk with not having automated testing. Automated testing has more benefits in agile than it does in the waterfall methodology.

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

#15

Post by nailer »

raverbashing wrote: post #8
homakov wrote: post #7

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

Because TDD in low level languages is a pain in the behind. And sometimes impossible In Python/Ruby sure, it's easy. In Java, or even C++, it's doable, but not great, and it requires some heavy tricks. But they surely need tests, but not necessarily TDD.

Ack re: TDD, but integration tests with a bad certificate would have picked this up.

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

#16

Post by lyngvi »

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…

++ 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.

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

#17

Post by gcb0 »

I'm 100% convinced it's malicious... But otherwise, it could be explained by the coder adding another check

   If(err==my_dev_string)
     goto fail;
Then before committing, sending the diff to a colleague who emails back "you forgot a debug there, besides that ship it"

He removes only one line of two and ships.

And of course i always try to attribute those things to incompetent other than malice, but since in this case there was probably money and bribes involved so i tend to the later.

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

#18

Post by homakov »

raverbashing wrote: post #8
homakov wrote: post #7

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

Because TDD in low level languages is a pain in the behind. And sometimes impossible In Python/Ruby sure, it's easy. In Java, or even C++, it's doable, but not great, and it requires some heavy tricks. But they surely need tests, but not necessarily TDD.

Not testing everywhere, but "smoking test" that your SSL implementation protects from MITM, at all, and none of your programmers screwed it (with gotofail or anything else).

This is what I expect from an OS developers

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

#19

Post by stcredzero »

raverbashing wrote: post #8
homakov wrote: post #7

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

Because TDD in low level languages is a pain in the behind. And sometimes impossible In Python/Ruby sure, it's easy. In Java, or even C++, it's doable, but not great, and it requires some heavy tricks. But they surely need tests, but not necessarily TDD.

And sometimes impossible

I've been in that position. Sometimes with embedded, you can't have software fixtures, only hardware ones. (Bit banging)

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

#20

Post by jeremyjh »

beachstartup wrote: post #12

i do this all the time in vim. all it takes is one extra keystroke 'p' to do this. if i were to take a guess, the programmer first typed out all the if conditional expressions, then went in and pasted 'goto fail' out of a buffer after each one, and accidentally did it twice there.

That didn't happen - look at the diff in the OP.
Post reply on HN