back to article 'Boss, I've got a bug fix: Nuke the whole thing from orbit, rewrite it all'

Hello, world. Welcome back to Line Break, our weekly column of terrible code readers have spotted in the wild – think of it as a group therapy session. Let's skip to the main() course. Beauty or the beast Since SSL/TLS is in the news again, let's start with some weird code spotted by Georgi in OpenSSL and LibreSSL, a fork of …

Silver badge
Facepalm

Yes, well...

That's the kind of code quality that gives us an OpenSSL with a switch to flick to disable SSLv2 that doesn't actually disable SSLv2.

10
0
Gold badge
Facepalm

Re: Yes, well...

....and also gives us the example memory handling fuckup from the first chapter of "C for dummies" known better as "Heartbleed".....

10
1
gv

Interesting

What's interesting about these anecdotes is that the project managers, "solution architects", et al., would be absolutely unable to triage or investigate these problems. It's risible that coders are seen as mere 'bricklayers' and are consequently outsourced.

32
0
JLV
Silver badge

Re: Interesting

Outsourcing might be a good idea with those particular coders, dontcha think? I mean, a suitably screened replacement is unlikely to do worse.

0
0

Well, this article'll cause some arguments, eh?

I guess a coder either belongs to the "ALL gotos are evil" school of thought, or the "This actually makes for very readable [and therefore easily auditable] code" one.

7
0

Re: Well, this article'll cause some arguments, eh?

Opinion please!

0
0
TRT
Silver badge

Re: Well, this article'll cause some arguments, eh?

I'd be happier if the conditional wrapping on the truncated handler exited that block with some kind of an unconditional break or an unconditional jump out of the block, just so it makes it obvious that the conditional wrapping is window dressing. Why isn't it being handled as a function call anyway? This is like some prehistoric implementation of function calling. It's a neat trick, I'll grant you that, but just why?

7
0
Silver badge

Re: Well, this article'll cause some arguments, eh?

I would use goto to get out of an if or a loop to the end of a routine to tidy up before returning but never into an if or a loop, that way lies madness.

Instead I would have set a variable holding the error condition before jumping forward to the end of the routine where it can be checked to see what has to be tidied up before doing that and returning.

The if(0) thing works but it's just a little too clever.

9
0
LDS
Silver badge

Re: Well, this article'll cause some arguments, eh?

That if {0} is actually a "goto err;" thus I don't understand why they didn't wrote in that way, it would have been at least clearer to read. Once you start to use gotos because of lack of an exception mechanism in C, use it clearly. The lack of proper comments is appalling too - if you attempt to do somthing "smart", explain it.

Anyway, it looks LibreSSL is just borrowing heavily from OpenSSL and probably just removing some little used stuff - but it's not a clean room rewrite...

12
0
Silver badge

Re: Well, this article'll cause some arguments, eh?

"Opinion please!"

I know I'm in the minority , but I think its an elegant solution. Ok, it'll throw some newbies but newbies won't be debugging complex encryption code to start with (in any sane project). If its too complex for managers to follow - tough. There's a reason they're managers and not coders. As someone else pointed out however, a few comments in the code wouldn't have hurt.

1
5
Silver badge
Linux

Re: Well, this article'll cause some arguments, eh?

Actually my favourite if I need a goto style fatal error handler is setjmp()/longjmp() and friends.

No matter where you are, or how deeply nested, you end up in a high level error handler that can examine the state of the machine, decide what to do, and do it.

Better still...

if(err=setjmp(Env))

DoErrorStuff(err);

...means that ALL your fatal error handling is well away from the code that is doing the real business of the program.

2
0
Silver badge

Re: Well, this article'll cause some arguments, eh?

"Once you start to use gotos because of lack of an exception mechanism in C, use it clearly. The lack of proper comments is appalling too - if you attempt to do somthing "smart", explain it."

Absolutely agreed. In fact if I had to pick the very worst thing about this code I'd say that the label err: is incorrectly named, everything that happens here seems to me (not a C programmer) about freeing resources. There seem to me to be three exit conditions: (a) success (b) packet length error (c) certificate length error. It looks to me like the first test looks for (b) error, the second for (c) then there is a block between the two snippets that is executed if those tests don't detect their errors.

Now I understand, from your comment and a quick Google, that there is no true exception handling in C, so we sometimes use the goto. So can't it work like this? (go easy on me, I'm not a coder)...

/* trap wrong packet length */

if (CBS_len(&cert_list) < 3) {

SSLerr(SSL_F_SSL3_GET_SERVER_CERTIFICATE, SSL_R_BAD_PACKET_LENGTH);

ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);

goto finalise;

}

/* trap cert length mismatch */

if (!CBS_get_u24_length_prefixed(&cert_list, &cert)) {

SSLerr(SSL_F_SSL3_GET_SERVER_CERTIFICATE, SSL_R_CERT_LENGTH_MISMATCH);

ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);

goto finalise;

}

/* code that gets executed if the exceptions above aren't encountered */

/* free resources */

finalise:

EVP_PKEY_free(pkey);

X509_free(x);

sk_X509_pop_free(sk, X509_free);

return (ret);

I'm not normally a fan of the goto and I might have preferred a nested conditional or maybe setting a variable to contain the current error type (or null if no error) and then branching on that lower down, depending on the prevailing style of the other code. But I understand they might have a place where there is no native exception handling.

3
0

Re: Well, this article'll cause some arguments, eh?

I know I'm in the minority...

Good, then you know the right answer.

but I think its an elegant solution

Or not.

Ok, it'll throw some newbies

And this is why you don't do it. The most important job source code has is to inform other programmers of your intent.

a few comments in the code wouldn't have hurt

All comments fundamentally say the same thing. 'I know I've not explained myself well, but I don't care enough to do it better - good luck!'

PS I'm in the gotos are bad camp. Does it show?

10
8

Re: Well, this article'll cause some arguments, eh?

"And this is why you don't do it. The most important job source code has is to inform other programmers of your intent."

Amen to that. Best thing I've read in a while. Pity upvotes can't be multiplied. You have one anyway.

"PS I'm in the gotos are bad camp. Does it show?"

I've always been taught gotos are fundamentally wrong, and out of 5 years of C programming, I can't recall using a single one.

Functions are the way to organise program flow in C, and if you feel you need to goto, that just means you got your functions flow wrong and need to try again.

Openssl is a typical example of complete kludge code, with no organised program flow. It's just hacked, works by sheer luck, out of try and fail, and certainly not because anyone devised a clear program flow through C code.

3
3
Silver badge

Re: Well, this article'll cause some arguments, eh?

"And this is why you don't do it. The most important job source code has is to inform other programmers of your intent."

No, the most important job it has is to solve the problem. Not every problem is "hello world". Some like video codecs or encryption are fundamentally hard and the code will be complex no matter what you do so newbies have no business going anywhere near it.

"All comments fundamentally say the same thing."

Thats probably the dumbest thing I've seen posted on El Reg for some time.

"PS I'm in the gotos are bad camp. Does it show?"

That and the fact that you're all theory and no practice.

8
2
Silver badge

Re: Well, this article'll cause some arguments, eh?

It's not elegant. It's appalling. You're welcome to disagree but I offer as exhibit A, the recent round of disastrous bugs in OpenSSL. Bugs emanating from the original designers, by the way, not some poor sap newbie who came along later and tried to maintain it.

First rule of engineering is you are not as clever as you think you are. Follow the principle of least surprise. Use constructs and terminology that others can follow. Be methodical to a fault. The only way to reduce the complexity of a system is to black-box its components, and the only way to do that is to make them predictable. These examples fail on every one of those points.

Oh, and personally I would have "assembly man" put against the wall and shot. Then I would shoot his kids, just to make sure it didn't happen again. But that's just me.

8
2
Bronze badge
Mushroom

Re: Well, this article'll cause some arguments, eh?

Isn't setjmp/longjmp a primitive form of exceptions? Sorry, exceptions suck, even Lisp's vaunted "conditions". Novel and mysterious ways to write spaghetti code.

In OpenSSL I would just rename "err:" to "end:", change the "if(0){" to "goto end;", and add "//fallthru" comments between goto's to clarify the intended control flow.

Better yet, put the nail in SSLv3's coffin and nuke the whole file. Or nuke the whole damn SSL/TLS protocol.

1
0
Silver badge

Re: Well, this article'll cause some arguments, eh?

> But that's just me.

Not just you. "Kill it" was my immediate thought reading this exquisitely horrible story.

I had to deal with a guy who did assembler style "optimizations" in Mathematica(!), proving once and for all that one can do assembler in any language.

0
0

@boltar Re: Well, this article'll cause some arguments, eh?

No, the most important job it has is to solve the problem.

Perhaps I'm not very good, but I've kind of come to accept that, on software complex enough to matter, I never solve the problem. It just works right up to the point that someone finds a bug.

The 'puter will do what it's been told, no worries there. So my concern is will the person debugging that new issue understand what I wanted to do? Because I'm obviously not doing it, if I was there wouldn't be a bug to fix in the first place.

It might be better to say the most necessary job it has is to solve the problem. Like the most necessary job of a car is to move. The most important job is to stop.

1
0
Silver badge

Re: Well, this article'll cause some arguments, eh?

All comments fundamentally say the same thing

Not true, at all, I'm afraid. Here are a few examples of the different things comments can do:

- Record the date / project reference a change was made under (useful for auditing)

- Explain the reason for doing something a certain way, for example to meet best practice, or to work around a known bug elsewhere that is beyond your control (useful for maintainability)

- Record what the code actually does on a trivial level, for example, 'increment x' (not useful)

- Declare what a section of code does, on a conceptual level, for example implementing an efficient sort algorithm a la Knuth (useful, but probably should be in the interface, not the implementation,and you have to be careful that later changes won't render the comment obsolete)

- Assist future programmers in avoiding the same pitfalls you just spent half a day programming your way out of.

8
0
Silver badge
Windows

Re: Well, this article'll cause some arguments, eh?

Anyway, it looks LibreSSL is just borrowing heavily from a fork of OpenSSL and probably just removing some little used stuff - but it's not a clean room rewrite...

*blinkblink*

0
0
Gold badge

Re: Well, this article'll cause some arguments, eh?

"First rule of engineering is you are not as clever as you think you are"

Nah, the first rule is that debugging is harder than writing it in the first place so if you write code that is as clever as you think you are then you won't be able to debug it.

The example shown can be handled more simply (and so, more reliably) by using nested if-statements and just accepting greater levels of indentation. Lexical scoping makes it manifestly obvious whether you've paired up creation/deletion steps and it is robust against someone coming along a year or so later and adding code in the middle. There is no excuse for using a goto statement in C. It ought to be removed from the language and any significant body of code that thereafter fails to compile was almost certainly buggy as hell beforehand but you didn't know it because you're in denial.

3
1
Silver badge
Windows

Re: Well, this article'll cause some arguments, eh?

@ boltar:

Dammit! did we *agree* on something?

If you're code is *informing other programmers* of anything, it is likely not accomplishing anything. I'll guess that the comments in *that* code amount to

# This glorious communication device was written by MEE!!!!!

0
0
TRT
Silver badge

Re: Well, this article'll cause some arguments, eh?

I thought the first rule of engineering is that you don't talk about engineering.

2
0
Anonymous Coward

Re: Well, this article'll cause some arguments, eh?

@TRT

goto skip_err_handling;

truncated: /*derp*/

f_err: /*more derp*/

skip_err_handling:

err: /*release resources and stuff*/

//Wouldn't this be "cleaner" and >definitely< not get cut away by overeager compilers?

2
0
Bronze badge

Re: Well, this article'll cause some arguments, eh?

I'm ambivalent about gotos but I do have an antipathy towards 'cute' code and that sample definitely belongs in the cute camp. This kind of coding indicates structural issues so its time to back out and consider an alternative way to implement the required functionality.

I used to have a pet theory about coding issues and bugs which roughly summarized suggested that the reason why code went pear shaped was because of most programmers' inability to type. That is once they'd put a chunk of code down they'd tinker with it, they'd bend the universe around it, they'd do anything to avoid actually having to rewrite it. (Some of the traditional editors didn't help -- 'vi', I'm looking at you...). These days with the fancy UIs (Eclipse or lookalikes) you still have problems typing in code, the editors seem to be more focused on cut-n-paste, hauling in snippets and throwing 'em into the pot, so once again we may still be sacrificing code design on the altar of poor editing.

1
0

Re: Well, this article'll cause some arguments, eh?

"No, the most important job it [the source code] has is to solve the problem"

BZZZZZT!

The most important job the PROGRAMME has is to solve the problem.

The job of the SOURCE CODE is to provide a human-readable account of how the programme was implemented in a form that a machine can turn into an actual programme, but more importantly in a form that a human can read, understand, and make changes to with a clear understanding of the implications of the change.

At the machine level (the running programme) there's little difference between a goto and a subroutine jump (both involve loading a specific value into the Programme Counter and loading some registers/well-defined memory locations with arguments in the latter case), but at the source code level it can make the world of difference between a source file a competent programmer with no former knowledge of the code is able to understand in an hour and one that requires the same programmer to spend a week on unravelling its logic.

2
0
Silver badge

Re: Well, this article'll cause some arguments, eh?

Anyway, it looks LibreSSL is just borrowing heavily from a fork of OpenSSL and probably just removing some little used stuff - but it's not a clean room rewrite...

It's always been a fork. A lot of stuff has been removed or rewritten, but one of the reasons for the fork was maintaining API compatibility.

Nevertheless, I find it interesting that this bit of code was kept around.

0
0
Silver badge

Re: Well, this article'll cause some arguments, eh?

"The job of the SOURCE CODE is to provide a human-readable account of how the programme was implemented in a form that a machine can turn into an actual programme, but more importantly in a form that a human can read,"

Do I take it you write all your programs in COBOL then?

0
4

Re: Well, this article'll cause some arguments, eh?

No, but I write them with the expectation that at some point I'll have to look at the source code again, possibly a decade or more since I last looked at it, and I'd like to be able to clue myself into what was going on in my head at the time I wrote it.

Why do you think writing readable code is something to sneer at?

1
0
Anonymous Coward

Pessimal sorting

"It passed through the entire symbol table once for each entry, found the next entry in alphabetical order, output the cross-reference line, marked that symbol as consumed, and went back to repeat, quitting only when it found no unconsumed symbol. It was using an O(n**2) sort!"

No biggie. I once had to deal with a sort routine along the lines of:

while (!is_sorted()) {

generate_random_permutation() ;

permute_data() ;

}

Of course, it was written in a slightly more complicated way, so it took a while to figure out what it was doing ...

4
0
Silver badge

Re: Pessimal sorting

Sounds like the very definition of a bogo-sort: http://www.jargon.net/jargonfile/b/bogo-sort.html

3
0

It's the kind of code contribution that a government agency would happily introduce!

2
0
FAIL

Beastly, Just Beastly

Goto's are evil, only bone idle spaghetti loving coders use them. Relying on compiler tricks is a dangerous game, compilers change, code gets ported, 'nuanced' techniques can get lost.

Code should not be clever, it should be maintainable and easy to read.

20
10
Silver badge

Re: Beastly, Just Beastly

>> Code should not be clever, it should be maintainable and easy to read.

Agreed. Let the compiler do the clever stuff.

It's interesting that MISRA standards ban all the clever stuff. I'm sure there's a reason for that...

5
0
Anonymous Coward

"MISRA standards ban all the clever stuff"

MISRA is a bit more pragmatic. They restrict use of "clever stuff" rather than ban it - if you need to do it, and can prove you understand how to do it, then you can by use of a deviation ;-)

The first version of MISRA C basically said that goto should not be used. The 2012 version allows it to be used, but only when certain restrictions are applied:

1) No back jumps, preventing goto from being used to create loops (use the loop constructs for that);

2) No jumping over initialisers, preventing undefined-bahaviour;

3) Only one goto may be used to exit a loop, reducing complexity;

4) No jumping into blocks (this would catch the OpenSSL example).

There is an Advisory rule saying that goto should not be used. This can be elevated to Required or Mandatory if local policy decides that further restriction or prohibition is required.

This basically acknowledges that there are valid uses of goto (such as error handling), but "incorrect" use can lead to code that is hard to maintain and that is more likely to contain control-flow related defects.

5
0
Silver badge

Re: Beastly, Just Beastly

"Goto's are evil, only bone idle spaghetti loving coders use them."

Stop parroting what you were taught in you programming classes and trying thinking for yourself instead. In C gotos are extremely useful for error handling, specifically for jumping out of deeply nested loops or switches instead of having to set a whole host of condition variables to exit status.

There's a reason gotos get used in highly complex C projects such as the linux kernel and its not because the coders are amateurs.

Also you should remember that in most assemblers gotos (jumps) are all you've got for doing flow control.

12
3
Silver badge

Re: Code should not be clever, it should be maintainable and easy to read(Beastly, Just Beastly)

Unfortunately sometimes clever code is precisely what is the most maintainable and easy to read.

If your compiler allows you to write it.

Years ago I had a situation where I wanted to a call one of three subroutines depending on a variable whose value was 0, 1 or 2. 6809 processor.

In assembler, the job was simple. Shift the value left one bit to turn the 8 bit number into a 16 bit offset, add to the base memory address of the call table in which three addresses of three subroutines are stored, and call the address so pointed to.

The C compiler didn't recognise an array of pointers to functions..

After a wasted day it became if then else if then else...

1
0
LDS
Silver badge

Re: Beastly, Just Beastly

gotos are not evil per se, are surely evil the way they were used in the early version of BASIC, when you had no other way to invoke different parts of code. In plain C, they could be useful if properly used. Some instructions like break/continue are actually very much alike gotos (they are unconditional jumps to a given location, albeit implicit), and in some ways exception handlers are also (although with a far better semantic and stack-unwinding capabilities). IMHO in C using goto *inside* a function to jump to error handling code is acceptable - if done properly. Other uses are usually just crazy.

Just don't mix semantic - if {0} is just a "smart" trick for another goto, just with an implicit meaning instead of an explicit one.

Otherwise you could make like an ex (luckily) colleague of mine, that resolved the issue of error handling just calling exit(1) every time he encountered an error... in a multithreaded application...

3
0

Re: Beastly, Just Beastly

You can get around the hatred of goto with the following;

do

{

if (!someOp())

break;

if (!someOtherOp())

break;

return SUCCESS;

}

while (false);

cleanup();

return FAILURE;

2
0
Silver badge

Re: Beastly, Just Beastly

"Goto's are evil, only bone idle spaghetti loving coders use them."

That's a little too black-and-white. tbh, GOTOs are a tool, and refusing to use them outright because they shouldn't be used for everything is like saying you should never use a bucket for anything because it's not suitable for frying bacon in. But sometimes I'm not making bacon, and a bucket is actually quite useful.

I don't see anything particularly wrong with the sparing use of GOTO - if the whole structure of your code relies on them then yeah, it's a horrific nightmare of spaghetti pain (I still wake up screaming after dreams of writing programs in BASIC back in the '80s), but legibility problems only really begin to creep in when you have GOTOs within GOTOs - if you're using GOTO to reach a block which just terminates the program then it's not hard to follow. If you end up following a string of GOTOs (goto 8, do x and goto 24, do y and goto 4, do z and goto 347) then that's when it becomes horrible code.

4
0
Anonymous Coward

Re: Beastly, Just Beastly

As a wise person (Parnas? Hoare?) once wrote: the problem with goto is that there is no comefrom.

1
0
Bronze badge

Re: Beastly, Just Beastly

> Also you should remember that in most assemblers gotos (jumps) are all you've got for doing flow control.

Actually... IF and WHILE macros are commonly used in assembly; they help a lot. Still, I fully agree with your point that gotos are very useful in low-level C/asm code.

1
0
Anonymous Coward

Re: Beastly, Just Beastly

Doesn't that fail to cleanup() on success?

It also has:

1) Multiple exits from a loop;

2) Multiple return paths from a function.

May be ok in some places, but probably not so in critical systems (e.g. safety or security).

1
0
Gold badge

Re: Beastly, Just Beastly

"There's a reason gotos get used in highly complex C projects such as the linux kernel and its not because the coders are amateurs."

Well actually that's a whole other can of worms you've got there, because the choice of C as the implementation language is highly questionable. For pretty much the entire lifetime of Linux as a project, a C++ compiler has been able to generate equivalent code with "no overhead for the stuff you don't use" and using RAII would eliminate most uses of goto, reduce the number of lines of code and be a darn sight more reliable when extra code gets added a year or two later by a different programmer.

But no. Instead we get a tired old list of excuses about how someone once heard that their friend had compiled a 10-line program (which on inspection turns out to be a carefully crafted straw man of no possible utility in the real world) and been rewarded with a 100KB executable, or some random wibbling about how floating point or exceptions aren't allowed in the kernel, apparently oblivious to the fact that you if you don't use them then any half-decent implementation will not link in the supporting library code (as has been the case for 20 years or more), or some other wibbling about the complexity of a C++ compiler which ignores the fact that GCC is a living demonstration that only the front-end is complex and that part is shared across every target platform. (Seriously, why in the name of fuck do embedded chip vendors write their own IDE and C compiler rather than just contribute a back-end to the GCC project? It makes their project more buggy and less flexible. Always. Predictably so.)

The Linux kernel devs clearly know their shit, but they are as prone to programming prejudice as the rest of us. They do what they are comfortable with and in the short term that does lead to better code. In the long term, it leads the whole project down a one-way street and at some point everyone will wish that "they" had bitten the bullet many years ago and used a more modern language.

1
2
Silver badge
Meh

Re: Beastly, Just Beastly

Some code that I've sort-of inherited uses GOTOs a fair bit, and I find it perfectly acceptable. It is always the same.

GOTO bailout;

It's used for totally unrecoverable errors.

1
0

Re: Stop parroting what you were taught in you programming classes

I understand where the terror of seeing a GoTo comes from. Way back in the dark ages when I dabbled in some programming, BASIC was the language of choice and Line Numbers were the only way to go. Pick the wrong combination of starting numbers and line increments and pair it with bad planning and you could be in a world of hurt. You'd either user colons to concatenate commands, run out of line numbers, or use a GoTo to jump to a new section of code. In point of fact, my first class taught us to write our program from 10 with increments of 10, use GoTo (or GoSubs when they became available) and put the functions in the 10,000 range with a jump of 5,000 for the next function. Done badly, it IS impossible to debug. Hence the terror.

But I agree the terror is now overdone. No one uses line numbers anymore, or at least line numbers in the limiting way they were used back then. So the really ugly problems that use to exist back then have gone away.

0
1
Facepalm

Re: Beastly, Just Beastly

"Also you should remember that in most assemblers gotos (jumps) are all you've got for doing flow control."

Actually, every processor I have ever worked with supports subroutines. The 6502 has JSR (jump to subroutine) and RTS (return from subroutine). Z80 based processors also have subroutines (CALL/RET), and I would assume their X86 successors do too.

I found out about how they work the hard way. I had a game which would randomly lock up the whole system. It took me awhile to find out why: I was using subroutines, but some of my collision/error handling included JMP statements that exited the routine. Of course after awhile, the stack overflowed and the machine melted. Took me awhile to figure out that little nasty bugger.

1
0
Bronze badge

Re: Code should not be clever, it should be maintainable and easy to read(Beastly, Just Beastly)

@itzman

"After a wasted day it became if then else if then else..."

You have a trailing else?!? WHY MAN?!

0
0

Re: Beastly, Just Beastly

The problem with your entire argument is that it's based on a false premise: that no one tried to write linux in C++.

It was tried, back in the time when I had a 386 PC and an entire distro fit on 2 floppies. Maybe it was before the time when "a C++ compiler has been able to generate equivalent code with "no overhead for the stuff you don't use" " But the result was a significantly bigger kernel which ran much slower.

1
1

Page:

POST COMMENT House rules

Not a member of The Register? Create a new account here.

  • Enter your comment

  • Add an icon

Anonymous cowards cannot choose their icon

Forums

Biting the hand that feeds IT © 1998–2017