Why is goto dangerous?
goto
doesn't cause instability by itself. Despite about 100,000 goto
s, the Linux kernel is still a model of stability.
goto
by itself should not cause security vulnerabilities. In some languages however, mixing it with try
/catch
exception management blocks could lead to vulnerabilities as explained in this CERT recommendation. Mainstream C++ compilers flag and prevent such errors, but unfortunately, older or more exotic compilers don't.
goto
causes unreadable and unmaintainable code. This is also called spaghetti code, because, like in a spaghetti plate, it's very difficult to follow the flow of control when there are too many gotos.
Even if you manage to avoid spaghetti code and if you use only a few gotos, they still facilitate bugs like and resource leaking:
- Code using structure programming, with clear nested blocks and loops or switches, is easy to follow; its flow of control is very predictable. It's therefore easier to ensure that invariants are respected.
- With a
goto
statement, you break that straightforward flow, and break the expectations. For example, you might not notice that you have still to free resources.
- Many
goto
in different places can send you to a single goto target. So it's not obvious to know for sure the state you are in when reaching this place. The risk of making wrong/unfounded assumptions is hence quite big.
Additional information and quotes:
C provides the infinitely-abusable goto
statement and labels to
branch to. Formally the goto
is never necessary, and in practice it
is almost always easy to write code without it. (...)
Nonetheless we will suggest a few situations
where goto 's may find a place. The most common use is to abandon
processing in some deeply nested structures, such as breaking out of
two loops at once. (...)
Although we are not dogmatic about the matter, it does seem that goto statements should be used sparingly, if at all.
When could goto be used?
Like K&R I'm not dogmatic about gotos. I admit that there are situations where goto could be ease one's life.
Typically, in C, goto allows multilevel loop exit, or error handling requiring to reach an appropriate exit point that frees/unlocks all the resources that were allocated so far (i.e.multiple allocation in sequence means multiple labels). This article quantifies the different uses of the goto in the Linux kernel.
Personally I prefer to avoid it and in 10 years of C, I used maximum 10 gotos. I prefer to use nested if
s, which I think are more readable. When this would lead to a too deep nesting, I'd opt either to decompose my function in smaller parts, or use an boolean indicator in cascade. Today's optimizing compilers are clever enough to generate almost the same code than the same code with goto
.
The use of goto heavily depends on the language:
In C++, proper use of RAII causes the compiler to automatically destroy objects that go out of scope, so that the resources/lock will be cleaned anyway, and no real need for goto any more.
In Java there's no need for goto (see Java's author quote above and this excellent Stack Overflow answer): the garbage collector that cleans the mess, break
,continue
, and try
/catch
exception handling cover all the case where goto
could be helpful, but in a safer and better manner. Java's popularity proves that goto statement can be avoided in a modern language.
Zoom on the famous SSL goto fail vulnerability
Important Disclaimer: in view of the fierce discussion in the comments, I want to clarify that I don't pretend that the goto statement is the only cause of this bug. I don't pretend that without goto there would be no bug. I just want to show that a goto can be involved in a serious bug.
I don't know how many serious bugs are related to goto
in the history of programming: details are often not communicated. However there was a famous Apple SSL bug that weakened the security of iOS. The statement that led to this bug was a wrong goto
statement.
Some argue that the root cause of the bug was not the goto statement in itself, but a wrong copy/paste, a misleading indentation, missing curly braces around the conditional block, or perhaps the working habits of the developer. I can't neither confirm any of them: all these arguments are probable hypotheses and interpretation. Nobody really knows. (meanwhile, the hypothesis of a merge that went wrong as someone suggested in the comments seems to be a very good candidate in view of some other indentation inconsistencies in the same function).
The only objective fact is that a duplicated goto
led to exit the function prematurely. Looking at the code, the only other single statement that could have caused the same effect would have been a return.
The error is in function SSLEncodeSignedServerKeyExchange()
in this file:
if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0)
goto fail;
if ((err =...) !=0)
goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
goto fail;
goto fail; // <====OUCH: INDENTATION MISLEADS: THIS IS UNCONDITIONDAL!!
if (...)
goto fail;
... // Do some cryptographic operations here
fail:
... // Free resources to process error
Indeed curly braces around the conditional block could have prevented the bug:
it would have led either to a syntax error at compilation (and hence a correction) or to a redundant harmless goto. By the way, GCC 6 would be able to spot these errors thanks to its optional warning to detect inconsistent indentation.
But in first place, all these gotos could have been avoided with more structured code. So goto is at least indirectly a cause of this bug. There are at least two different ways that could have avoided it:
Approach 1: if clause or nested if
s
Instead of testing lots of conditions for error sequentially, and each time sending to a fail
label in case of problem, one could have opted for executing the cryptographic operations in an if
-statement that would do it only if there was no wrong pre-condition:
if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) == 0 &&
(err = ...) == 0 ) &&
(err = ReadyHash(&SSLHashSHA1, &hashCtx)) == 0) &&
...
(err = ...) == 0 ) )
{
... // Do some cryptographic operations here
}
... // Free resources
Approach 2: use an error accumulator
This approach is based on the fact that almost all the statements here call some function to set an err
error code, and execute the rest of the code only if err
was 0 (i.e., function executed without error). A nice safe and readable alternative is:
bool ok = true;
ok = ok && (err = ReadyHash(&SSLHashSHA1, &hashCtx))) == 0;
ok = ok && (err = NextFunction(...)) == 0;
...
ok = ok && (err = ...) == 0;
... // Free resources
Here, there is not a single goto: no risk to jump to quickly to the failure exit point. And visually it would be easy to spot a misaligned line or a forgotten ok &&
.
This construct is more compact. It is based on the fact that in C, the second part of a logical and (&&
) is evaluated only if the first part is true. In fact, the assembler generated by an optimizing compiler is almost equivalent to the original code with gotos: The optimizer detects very well the chain of conditions and generate code, which at the first non null return value jumps to the end (online proof).
You could even envisage a consistency check at the end of the function that could during the testing phase identify mismatches between the ok flag and the error code.
assert( (ok==false && err!=0) || (ok==true && err==0) );
Mistakes such of a ==0
inadvertently replaced with a !=0
or logical connector errors would easily be spotted during the debugging phase.
As said: I don't pretend that alternative constructs would have avoided any bug. I just want to say that they could have made the bug more difficult to occur.