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

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.

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?

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.

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

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;

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.

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.

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.

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?!