homeGeek CultureWebstoreeCards!Forums!Joy of Tech!AY2K!webcam

The Geek Culture Forums


Post New Topic  New Poll  Post A Reply
my profile | directory login | | search | faq | forum home
  next oldest topic   next newest topic
» The Geek Culture Forums   » All about the comics!   » The Joy of Tech   » Poor Goto!

 - UBBFriend: Email this page to someone!    
Author Topic: Poor Goto!
maxomai
Alpha Geek
Member # 781

Member Rated:
5
Icon 9 posted February 25, 2014 11:45      Profile for maxomai   Author's Homepage     Send New Private Message       Edit/Delete Post   Reply With Quote 
It really isn't the goto's fault. Check out the code on http://opensource.apple.com/source/Security/Security-55471/libsecurity_ssl/lib/sslKeyExchange.c?txt

In particular:

quote:
if ((err = SSLFreeBuffer(&hashCtx)) != 0)
goto fail;

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

If the author(s) of this code had followed the usual style rules and bracketed the if statements:

quote:
if ((err = SSLFreeBuffer(&hashCtx)) != 0) {
goto fail;
}
if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0) {
goto fail;
}
if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0) {
goto fail;
}
if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0) {
goto fail;
}
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) {
goto fail;
goto fail;
}
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) {
goto fail;
}

and the errant goto statement never happens.

Goto is defensible in some contexts. This is just bad style.

Posts: 343 | From: Portland, OR | Registered: May 2001  |  IP: Logged
dragonman97

SuperFan!
Member # 780

Member Rated:
4
Icon 1 posted February 26, 2014 02:48      Profile for dragonman97   Author's Homepage     Send New Private Message       Edit/Delete Post   Reply With Quote 
Yeah...I really hate one line if statements. Technically they work in many languages, but it's so risky to write code like that lest you make a stupid mistake (like Apple did). As such, I always* use statement blocks.

As for the goto, I suppose I'd need to see the whole section of code, but I'd like to think that it could be better done with a function (i.e. handle_error(foo)), a return (maybe?), or a variable that is acted on before the return. (i.e. error_type=foo; /*more code[...]*/ /*at end: */ if (error_type) {handle_error(error_type))

I'm a Perl guy, and even though 'next' is kind of 'dirty,' it's oh-so-handy during certain file read scenarios. I'm sure there are some legit uses of goto, but barring better source code practices, maybe Apple should have their goto permissions revoked for awhile. [Wink]

*Never say always...but I'm fairly certain...

--------------------
There are three things you can be sure of in life: Death, taxes, and reading about fake illnesses online...

Posts: 9210 | From: Westchester County, New York | Registered: May 2001  |  IP: Logged
Ugh, MightyClub
BlabberMouth, the Next Generation
Member # 3112

Member Rated:
5
Icon 1 posted February 26, 2014 15:17      Profile for Ugh, MightyClub     Send New Private Message       Edit/Delete Post   Reply With Quote 
I bracket all my if/while/for/etc blocks. And it's easy to point and laugh at this code. But I'm not convinced curly braces would have avoided this. Certainly it would be less likely, but you could still end up with this:
code:
if ((err = SSLFreeBuffer(&hashCtx)) != 0) {
goto fail;
}
if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0) {
goto fail;
}
if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0) {
goto fail;
}
if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0) {
goto fail;
}
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) {
goto fail;
}
goto fail;
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) {
goto fail;
}

Any decent human code reviewer would pick that up, but I doubt a static analysis tool would catch the code smell.

What they really need is good unit tests. All the curly braces in the world can't substitute for a set of tests that prove the correct behavior.

Well let me back that up just a hair. They need at least three of these things in place: curly braces, code review, and unit tests. Nothing is a panacea.

--------------------
Ugh!

Posts: 1669 | From: Ithaca, NY | Registered: Dec 2004  |  IP: Logged
The Famous Druid

Gold Hearted SuperFan!
Member # 1769

Member Rated:
4
Icon 1 posted February 26, 2014 19:07      Profile for The Famous Druid     Send New Private Message       Edit/Delete Post   Reply With Quote 
quote:
Originally posted by Ugh, MightyClub:
Any decent human code reviewer would pick that up, but I doubt a static analysis tool would catch the code smell.

Any decent static analysis tool would have flagged the last 'if' as unreachable code, with or without the brackets.

The trouble with static analysis tools is they're usually too picky, programmers get sick of fixing 'problems' that aren't problems at all, so they stop using the analysis tool.

But I'm confused, the code snippets above would *always* fail, isn't that the exact reverse of what happened in the Apple case?

[update] Just checked the code. 'fail' frees a buffer then returns the err value, which would be 0 (indicating no error, i.e. success)

--------------------
If you watch 'The History Of NASA' backwards, it's about a space agency that has no manned spaceflight capability, then does low-orbit flights, then lands on the Moon.

Posts: 10460 | From: Melbourne, Australia | Registered: Oct 2002  |  IP: Logged
Ugh, MightyClub
BlabberMouth, the Next Generation
Member # 3112

Member Rated:
5
Icon 1 posted February 27, 2014 16:27      Profile for Ugh, MightyClub     Send New Private Message       Edit/Delete Post   Reply With Quote 
quote:
Originally posted by The Famous Druid:
quote:
Originally posted by Ugh, MightyClub:
Any decent human code reviewer would pick that up, but I doubt a static analysis tool would catch the code smell.

Any decent static analysis tool would have flagged the last 'if' as unreachable code, with or without the brackets.

Good point. But that's true of the code without curly brackets as well.

Clearly this codebase is (or at least was) lacking many safety measures.

--------------------
Ugh!

Posts: 1669 | From: Ithaca, NY | Registered: Dec 2004  |  IP: Logged
quantumfluff
BlabberMouth, a Blabber Odyssey
Member # 450

Member Rated:
5
Icon 1 posted February 28, 2014 00:37      Profile for quantumfluff     Send New Private Message       Edit/Delete Post   Reply With Quote 
The compiler should have flagged this as unreachable code. At work we compile C++ with warning == error. Anything less is going to come back to bite you one day.
Posts: 2867 | From: 5 to 15 meters above sea level | Registered: Jun 2000  |  IP: Logged


All times are Eastern Time  
Post New Topic  New Poll  Post A Reply Close Topic    Move Topic    Delete Topic next oldest topic   next newest topic
 - Printer-friendly view of this topic
Hop To:

Contact Us | Geek Culture Home Page

© 2014 Geek Culture® All Rights Reserved.

Powered by Infopop Corporation
UBB.classicTM 6.4.0



homeGeek CultureWebstoreeCards!Forums!Joy of Tech!AY2K!webcam