Re: [dm-crypt] goto patch for cryptsetup?

[Arno Wagner <arno@wagner.name>]

On Fri, Mar 07, 2014 at 17:11:32 CET, Milan Broz wrote:
> On 03/07/2014 11:52 AM, Lars Winterfeld wrote:
> > Am 03/07/2014 06:23 AM, schrieb Milan Broz:
> >> On 03/06/2014 09:39 PM, Lars Winterfeld wrote:
> >>> Hi,
> >>>
> >>> in light of the latest "goto fail"s out there, would you reject a patch
> >>> replacing all 328 gotos with their semantic equivalents from structured
> >>> C programming?
> >>
> >> cryptsetup code uss goto for only one purpose: to have only one exit
> >> place from a function (with check for errors, release memory).
> >>
> >> Replacing it with anything else means duplicating of lot of code.
> > 
> > I agree that code duplication is undesirable. I can think of two
> > alternatives to goto that do not duplicate code. The first is to
> > introduce a one-time loop like this:
> 
> No please. Using goto here the style I prefer (you would use exception
> in other languages but this is intentionally low level C).
> 
> I really do no understand which problem you are trying to solve here
> - goto can be used very wrong, but this is not the case.

I would like to add that when reading "Goto considered harmful", 
the historic context needs to be taken into account: People used
goto wherever posible. Places where we would use while, for, 
else or even function calls today, people used goto. At the same
time, structured programming was not widely used at all and
regarded with suspicion by many. That is an entirely different 
situation than the one we are facing here: There are places where 
a goto is a natural fit and using anything else decrease code 
quality. For example, the "do { } while(0);" construct is a 
pure abomination and no expert would ever even consider using 
it as it violates good style on several levels.

So please stop and have a look at why and when "goto" is 
a problem and when not. The recent iOS case does not show 
that goto is a problem. It shows that missing code-review,
incompetent testing and missing analysis for dead code
can even overlook otherwise obvious errors. If they had
had a single test case for the code after the second goto, 
the error would have been immediately caught.
 
> > do {
> > // foo
> > if(a) break; // was: goto out;
> > // bar
> > if(b) break; // was: goto out;
> > // "all good" code
> > return 0;
> > } while(0);
> > // was: out:
> > // "on error" code
> > return -1;
> 
> Which is not only ugly but adds one level of indentation.

It is also quite unclear and hard to read. Anybody seriously 
using such a thing cannot be considered an expert programmer. 
The only legitimate use of it is for an entry in the obfuscated 
C contest.

> I am basically using kernel code style I like, read
> https://www.kernel.org/doc/Documentation/CodingStyle
> 
> (specifically chapter 7 - goto)

I see nothing wrong with that style. It is not for beginners,
but programming security-critical code is not either.

Arno
-- 
Arno Wagner,     Dr. sc. techn., Dipl. Inform.,    Email: arno@wagner.name
GnuPG: ID: CB5D9718  FP: 12D6 C03B 1B30 33BB 13CF  B774 E35C 5FA1 CB5D 9718
----
A good decision is based on knowledge and not on numbers. -  Plato