Goto fail;

mikeTheLiar

Original Wired article.

[code]goto fail;[/code] Strangely apt. Short story shorter, Apple fails at optional curly braces and gotos.

Ben L.

It's not so much a problem of with braces or goto, but more that they copied and pasted code too many times.

gu3st

@Ben L. said:

It's not so much a problem of with braces or goto, but more that they copied and pasted code too many times.

Samsung is gonna copy this bug in about 2 weeks.

Ben L.

@gu3st said:

@Ben L. said:
It's not so much a problem of with braces or goto, but more that they copied and pasted code too many times.

Samsung is gonna copy and paste this bug in about 2 weeks.

FTFY

Maciejasjmj

Guess they just got a .BAT file programmer for cheap.

Besides, the better question is - if this function always fails, how does that introduce a security hole? What would happen in case of an actual failure? I smell a bigger WTF hiding behind this one.

@Maciejasjmj said:

Besides, the better question is - if this function always fails, how does that introduce a security hole?

Nope. If it gets to that second goto, it goes to the end of the function with err == 0, while skipping all the tests that come afterwards. That's the security hole.

Weng

And thus we learn that Apple doesn't unit-test (or if they do, they have terrible code coverage)

random

@Weng said:

And thus we learn that Apple doesn't unit-test (or if they do, they have terrible code coverage)

And they obviously don't use static code analysis either.

Weng

On further analysis, they might unit-test, but they might fall into the antipattern of building the tests to the performance of the original version of the codebase, not to the spec.

LoremIpsum

I wonder if they would have rejected this crappy code if I had submitted it in an app?

Sir_Twist

I notice the Wired author also perpetuates the myth that “Goto Considered Harmful” means “never use goto.”

anotherusername

@Zecc said:

@Maciejasjmj said:
Besides, the better question is - if this function always fails, how does that introduce a security hole?
Nope. If it gets to that second goto, it goes to the end of the function with err == 0, while skipping all the tests that come afterwards. That's the security hole.

Right, so "goto fail" doesn't actually fail the function, it just releases its resources and then returns the value of err, which could be 0, which presumably will mean that the function [i]didn't[/i] fail (even though it skipped a bunch of checks that could have caused it to fail).

LoremIpsum

@Sir Twist said:

I notice the Wired author also perpetuates the myth that “Goto Considered Harmful” means “never use goto.”

There's always an elegant way to avoid gotos, so I see no reason not to follow that advice!

esoterik

@Weng said:

On further analysis, they might unit-test, but they might fall into the antipattern of building the tests to the performance of the original version of the codebase, not to the spec.

that would mean someone wrote a test that tested that the function always failed and didn't question it; or they were created automatically ?

its also an example of the single exit pattern, which is only vallid in c w/malloc. (which is why that patter inits err to an actual error code and puts err = succsss right before the fail: lable) I can't tell by the fragment what facilities they have, i.e. C or C++, etc.

PJH

@LoremIpsumDolorSitAmet said:

@Sir Twist said:
I notice the Wired author also perpetuates the myth that “Goto Considered Harmful” means “never use goto.”
There's always an elegant way to avoid gotos, so I see no reason not to follow that advice!

I'll just leave this here...

blakeyrat

@LoremIpsumDolorSitAmet said:

@Sir Twist said:
I notice the Wired author also perpetuates the myth that “Goto Considered Harmful” means “never use goto.”
There's always an elegant way to avoid gotos, so I see no reason not to follow that advice!

Loop within a loop, inner loop hits an error condition and flow needs to break out of both loops.

Note that the "goto" command in JavaScript was created explicitly to handle this case "elegantly", and exists for no other purpose*. Also, in C# "goto" is used for that purpose, as well as directing one case of a switch() statement to another case of it. (That second use is more dubious, but hey.)

*) before you pedantic dickweeds "correct" me, the syntax for "goto" in JavaScript is "break {label}" Yes it looks different. But it's the same damned thing.

mikeTheLiar

You know, the [code]goto[/code]s don't bother me unduly. It's more the fact that using the (optional) curlies would have made this obvious in a second. Or the fact that any halfway decent auto-formatter (Ctrl+E, D mothafuckas!) also would have made this obvious in a second.

TBH, I've always be somewhat lackadaisical about using "optional" curlies (especially in C# when dealing with multiple [code]using[/code] blocks, but I think that's a valid argument). This bug has demonstrated to me why that is a Very Bad Thing™. It has also given Python fanboys ammunition for years to come.

Ben L.

@mikeTheLiar said:

You know, the gotos don't bother me unduly. It's more the fact that using the (optional) curlies would have made this obvious in a second. Or the fact that any halfway decent auto-formatter (Ctrl+E, D mothafuckas!) also would have made this obvious in a second.

TBH, I've always be somewhat lackadaisical about using "optional" curlies (especially in C# when dealing with multiple using blocks, but I think that's a valid argument). This bug has demonstrated to me why that is a Very Bad Thing™. It has also given Python fanboys ammunition for years to come.

Even if they had used curly braces, an extra { goto fail; } would do the same thing.

flabdablet

@mikeTheLiar said:

TBH, I've always be somewhat lackadaisical about using "optional" curlies (especially in C# when dealing with multiple using blocks, but I think that's a valid argument). This bug has demonstrated to me why that is a Very Bad Thing™.

I don't always use optional braces, but when I don't it's because the whole statement fits neatly on one line. Had they formatted their code as

if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0) goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) goto fail;
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) goto fail;

it would have been every bit as robust in the face of merge or copy/paste failures as any curly-braced version.

Generating an error return code and jumping to a fail label if it's nonzero is such a common idiom that I'm not even sure the != 0 stuff is adding clarity value:

if (err = SSLHashSHA1.update(&hashCtx, &serverRandom)) goto fail;
if (err = SSLHashSHA1.update(&hashCtx, &signedParams)) goto fail;
if (err = SSLHashSHA1.final(&hashCtx, &hashOut)) goto fail;

looks clearer to me for the lowered noise, despite the potential misread of = as ==.

PJH

@flabdablet said:

if (err = SSLHashSHA1.update(&hashCtx, &serverRandom)) goto fail;

if (err = SSLHashSHA1.update(&hashCtx, &signedParams)) goto fail;

if (err = SSLHashSHA1.final(&hashCtx, &hashOut)) goto fail;

looks clearer to me for the lowered noise, despite the potential misread of = as ==.

-Wparentheses is the generally accepted solution to that particular problem. In C at least.

dkf

@mikeTheLiar said:

It has also given Python fanboys ammunition for years to come.

To be fair, Python doesn't really have auto-reformat in any sensible way (and can't; you've got to get the indentation right the first time because any code about has to assume the indentation it's got is meaningful).

@mikeTheLiar said:

TBH, I've always be somewhat lackadaisical about using "optional" curlies

Doing NodeJS, I've never hated automatic semicolon insertion so much:

var query = "UPDATE thingamabobs SET " +
                "frizzle_count = ?, " + 
                "frobs_state = 'tweezled' "
            "WHERE id = ?";

jslint is a friend.

LoremIpsum

@PJH said:

I'll just leave this here...

I prefer the IF-block version, actually. Also the first conditional can just return straight away without a label. Plus, there are other language features you can use to make this even better. Try/Catch/Finaly, anyone? And if your language doesn't support it, there are still ways to achieve similar patterns... I mean, some clever people have even found ways to implement try/catch/finally in classic ASP VB!

@blakeyrat said:

before you pedantic dickweeds "correct" me, the syntax for "goto" in JavaScript is "break {label}" Yes it looks different. But it's the same damned thing.

I don't consider 'break {label}' to be a true goto, as they are only used to escape some number of block levels, and cannot simply jump around to arbitrary places in the code. Therefore, they're just syntactical sugar, which is perfectly fine.

PJH

@LoremIpsumDolorSitAmet said:

@PJH said:
I'll just leave this here...
I prefer the IF-block version, actually.

Others detest them.@LoremIpsumDolorSitAmet said:

Plus, there are other language features you can use to make this even better. Try/Catch/Finaly, anyone?

I can't see that happening in core kernel code any time soon.

flabdablet

-Wparentheses is probably exactly why that construction doesn't get used, even in cases like these where (I think) it would make sense to do so.

Perhaps a macro would be better:

#define check(expr) err = expr; if (err) goto fail;
...
check(SSLHashSHA1.update(&hashCtx, &serverRandom))
check(SSLHashSHA1.update(&hashCtx, &signedParams))
check(SSLHashSHA1.final(&hashCtx, &hashOut))

though I am sure many people would have visceral objections to the implicit variable assignment and hidden control flow.

LoremIpsum

@PJH said:

Others detest them.

That's a shame. I guess they need to get used to programming languages made since the 1990s.

@PJH said:

I can't see that happening in core kernel code any time soon.

Someone should make a new version of C, really. However, you could probably implement it quite nicely with macros.

@LoremIpsumDolorSitAmet said:

Someone should ~~make a new version of C~~ either do a full rewrite of the kernel or come up with a code transpiler that can produce equally optimized/documented/readable code, in either instance being able to prove beyond all doubt that no new bugs were introduced

FTFY.

PJH

@LoremIpsumDolorSitAmet said:

@PJH said:
Others detest them.
That's a shame. I guess they need to get used to programming languages made since the 1990s.

Maybe it's the natural aversion to seeing code nested 20 levels deep that most experienced programmers have.

LoremIpsum

@PJH said:

@LoremIpsumDolorSitAmet said:
That's a shame. I guess they need to get used to programming languages made since the 1990s.
Maybe it's the natural aversion to seeing code nested 20 levels deep that most experienced programmers have.

20 levels deep is a sign of a bad programmer, not a bad programming language. I'm sure you'd agree that if you could have only ifs or gotos, you'd want ifs.

PJH

@LoremIpsumDolorSitAmet said:

I'm sure you'd agree that if you could have only ifs or gotos, you'd want ifs.

Since that's an unlikely scenario to occur, I'll stick with having both and using them as appropriate.

LoremIpsum

@PJH said:

that's an unlikely scenario to occur

until you start using Java or JavaScript!

PJH

@LoremIpsumDolorSitAmet said:

@PJH said:
that's an unlikely scenario to occur
until you start using Java or JavaScript!

How soon are you expecting kernel code to be written in either of those languages?

boomzilla

@PJH said:

@LoremIpsumDolorSitAmet said:
@PJH said:
that's an unlikely scenario to occur

until you start using Java or JavaScript!

How soon are you expecting kernel code to be written in either of those languages?

He never said it was easy. So stop bitching and get to work remaking the world in his image!

PJH

@boomzilla said:

@PJH said:
@LoremIpsumDolorSitAmet said:
@PJH said:
that's an unlikely scenario to occur

until you start using Java or JavaScript!

How soon are you expecting kernel code to be written in either of those languages?

He never said it was easy. So stop bitching and get to work remaking the world in his image!

Ok, once I've dealt with all these straw men he keeps dishing out...

@PJH said:

@boomzilla said:
@PJH said:
@LoremIpsumDolorSitAmet said:
@PJH said:
that's an unlikely scenario to occur
until you start using Java or JavaScript!
How soon are you expecting kernel code to be written in either of those languages?
He never said it was easy. So stop bitching and get to work remaking the world in his image!
Ok, once I've dealt with all these straw men he keeps dishing out...

Are you turning this into a demonstration of why too much indentation is a bad thing? Is that what we're doing?

PJH

@Zecc said:

@PJH said:
@boomzilla said:
@PJH said:
@LoremIpsumDolorSitAmet said:
@PJH said:
that's an unlikely scenario to occur

until you start using Java or JavaScript!

How soon are you expecting kernel code to be written in either of those languages?

He never said it was easy. So stop bitching and get to work remaking the world in his image!

Ok, once I've dealt with all these straw men he keeps dishing out..
.

Are you turning this into a demonstration of why too much indentation is a bad thing? Is that what we're doing?

I have no idea what you're getting at...

Weng

@esoterik said:

that would mean someone wrote a test that tested that the function always failed and didn't question it; or they were created automatically ?

Either way.

@esoterik said:

I can't tell by the fragment what facilities they have, i.e. C or C++, etc.

Being Apple, this is going to be Objective-C.

Had dinner with my boss, who used to be an Apple engineer. He was unsurprised but gave no details (presumably for fear that they would harm his family).

LoremIpsum

@PJH said:

@Zecc said:
@PJH said:
@boomzilla said:
@PJH said:
@LoremIpsumDolorSitAmet said:
@PJH said:
that's an unlikely scenario to occur
until you start using Java or JavaScript!
How soon are you expecting kernel code to be written in either of those languages?
He never said it was easy. So stop bitching and get to work remaking the world in his image!
Ok, once I've dealt with all these straw men he keeps dishing out.. .
Are you turning this into a demonstration of why too much indentation is a bad thing? Is that what we're doing?
I have no idea what you're getting at...

Let's use gotos with link hashes instead!

boomzilla

@PJH said:

that's an unlikely scenario to occur

@LoremIpsumDolorSitAmet said:

until you start using Java or JavaScript!

@PJH said:

How soon are you expecting kernel code to be written in either of those languages?

@boomzilla said:

He never said it was easy. So stop bitching and get to work remaking the world in his image!

@PJH said:

Ok, once I've dealt with all these straw men he keeps dishing out..
.

@Zecc said:

Are you turning this into a demonstration of why too much indentation is a bad thing? Is that what we're doing?

@PJH said:

I have no idea what you're getting at...

@boomzilla said:

That would be silly.

The_Assimilator

@flabdablet said:

Perhaps a macro would be better:

I agree.

#define APPLE goto fail
if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0) APPLE;

if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) APPLE;

if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) APPLE;

LoremIpsum

@The_Assimilator said:

@flabdablet said:

Perhaps a macro would be better:

I agree.

<font size="+1">#define APPLE goto fail
if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0) APPLE;

if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)</font><font size="+1"><font size="+1"> APPLE;</font></font><font size="+1"><font size="+1"><font size="+1"> APPLE;</font></font>

if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) APPLE;

</font>

FTFY

PJH

@The_Assimilator said:

@flabdablet said:

Perhaps a macro would be better:

I agree.

<font size="+1">#define APPLE goto fail
if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0) APPLE;

if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) APPLE; APPLE;

if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) APPLE;

</font>

HTH...

Buttembly_Coder

@PJH said:

@The_Assimilator said:

@flabdablet said:

Perhaps a macro would be better:

I agree.

<font size="+1">#define APPLE goto fail
if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0) APPLE;

if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) APPLE; APPLE;

if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) APPLE;

</font>

HTH...

Easy solution:

[code]#define APPLE goto fail;//[/code]

john

The OS X fix finally came out today, and Apple are very coy about the whole thing. First, the update notes in OS X Software Update don't say anything at all about the fix. The "more info" link mentions it in one bullet point at the bottom of a list of 20+ bullet points.

And that "Apple Security Updates" link? Doesn't have any info about 10.9.2 whatsoever.

In other words:

The fix was delayed because they were putting the finishing touches on a bunch of other non-critical "improvements"
Their corporate image is more important to them than informing users that for their security it might be a good idea to change passwords if they've used public Wi-Fi ~~recently~~ in the past few months.

But hey, what's this?

- Improves compatibility with Gmail Archive mailboxes

Better late than never I guess. Someone should invent a word for that feeling of relief one gets when Apple finally decide to fix something they broke horribly in a months-prior update, calling it an "improvement".

This just in: Live feed from Apple HQ

dkf

@LoremIpsumDolorSitAmet said:

I'm sure you'd agree that if you could have only ifs or gotos, you'd want ifs.

Provided I can have computed gotos, I'll go with gotos. Making loops with just ifs requires recursion.

Buttembly_Coder

@dkf said:

@LoremIpsumDolorSitAmet said:
I'm sure you'd agree that if you could have only ifs or gotos, you'd want ifs.
Provided I can have computed gotos, I'll go with gotos. Making loops with just ifs requires recursion.

I thought the idea around here was to use longjmp for speed?

dkf

@Buttembly Coder said:

I thought the idea around here was to use longjmp for speed?

I was saving that for version 2…

OzPeter

Just saw a report about the interesting time of when Apple introduced this bug, vs when the NSA purportedly added Apple to their PRISM network. Feel free to break out your tinfoil hats.

On the Timing of iOS’s SSL Vulnerability and Apple’s ‘Addition’ to the NSA’s PRISM Program

The SSL vulnerability was introduced in iOS 6.0

iOS 6.0 shipped on 24 September 2012.

According to slide 6 in the leaked PowerPoint deck on NSA’s PRISM program, Apple was “added” in October 2012.

savar

@Weng said:

@esoterik said:
I can't tell by the fragment what facilities they have, i.e. C or C++, etc.
Being Apple, this is going to be Objective-C.

It's C. http://www.opensource.apple.com/source/Security/Security-55471/libsecurity_ssl/lib/sslKeyExchange.c

dkf

@savar said:

It's C.

To be fair, that bit of code would look identical in Obj-C; if you're not using objects and classes, Obj-C is identical to C (and if you're not using any objc features at all, you'd want to compile as C as that's going to compile in less time; objc compilation is slower).