[dm-crypt] goto patch for cryptsetup?

[dm-crypt] goto patch for cryptsetup?

[Lars Winterfeld]

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?

Best wishes,
Lars

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

[Arno Wagner]

I don't think that is a good idea. The "goto fail" was a 
problem due to either a high level of incompetence, or 
due to intent disguised as a high level of incompetence. 
I think well-placed gotos are actually clearer and less 
risky than the structured programming equivalents. 

What happened with iOS would have been blatantly 
obvious with even minimal code review or a single 
test-case for the skipped functionality. That these
were not done is pretty bad. 

Of course, the final decision is Milan's. But I think 
what would be far better is that you review all these 
goto's as to whether any of them is a problem. That is
still less useful than a real review, but at least it
may alleviate the concerns of the "syntactic matching
people" that see the "goto" as a problem (or "sha-1", 
or the like) instead of the specific use.

Arno




On Thu, Mar 06, 2014 at 21:39:11 CET, 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?
> 
> Best wishes,
> Lars
> _______________________________________________
> dm-crypt mailing list
> dm-crypt@saout.de
> http://www.saout.de/mailman/listinfo/dm-crypt

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

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

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

Read e.g. kernel code, you will se the same patern on many places.

Milan

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

[Lars Winterfeld]

[-- Attachment #1: Type: text/plain, Size: 1339 bytes --]

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:

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;

The second alternative would be to introduce a new function like

int foo_failed(struct bar* p) {
	// clean p
	return -1;
}

and then inside the "int foo()":
if(a) return foo_failed(p);
instead of
if(a) goto fail;


Like with indentation, I think this is just a question of coding style.
Changes there don't make the program better, it just makes the code
nicer to read in the eyes of some people.

It's up to you. I would do the work. What do you think?

Cheers,
Lars



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

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

[Arno Wagner]

On Fri, Mar 07, 2014 at 11:52:12 CET, 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:
> 
> 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;

Urgh. That is exceedingly ugly and even less readable than 
with goto, as it misuses a loop as goto.Pleas do not do such
things, the cure is worse than the disease.
 
> The second alternative would be to introduce a new function like
> 
> int foo_failed(struct bar* p) {
> 	// clean p
> 	return -1;
> }
> 
> and then inside the "int foo()":
> if(a) return foo_failed(p);
> instead of
> if(a) goto fail;

And there you will get scoping problems and need to put stuff in 
structs just so you can pass them as argument. Not good.
 
> Like with indentation, I think this is just a question of coding style.
> Changes there don't make the program better, it just makes the code
> nicer to read in the eyes of some people.

Not me. Both your examples are harder to read. I do not
think getting rid of goto helps in any way as long as
goto is used competently and with restraint. In this
case, it is often superiour to other constructs.
 
Arno

> It's up to you. I would do the work. What do you think?
> 
> Cheers,
> Lars
> 
> 



> _______________________________________________
> dm-crypt mailing list
> dm-crypt@saout.de
> http://www.saout.de/mailman/listinfo/dm-crypt


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

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

[.. ink ..]

[-- Attachment #1: Type: text/plain, Size: 474 bytes --]

On Fri, Mar 7, 2014 at 5:52 AM, Lars Winterfeld <
lars.winterfeld@tu-ilmenau.de> wrote:



> The second alternative would be to introduce a new function like
>
> int foo_failed(struct bar* p) {
>         // clean p
>         return -1;
> }
>
> and then inside the "int foo()":
> if(a) return foo_failed(p);
> instead of
> if(a) goto fail;
>
>
>
i prefer to fo it this way,example[1]:

[1]
https://raw.github.com/mhogomchungu/zuluCrypt/master/zuluCrypt-cli/lib/open_tcrypt.c

[-- Attachment #2: Type: text/html, Size: 1014 bytes --]

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

[Milan Broz]

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.


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

I am basically using kernel code style I like, read
https://www.kernel.org/doc/Documentation/CodingStyle

(specifically chapter 7 - goto)

...

Please do not spend time with replacing goto this way.

If you want help - write a tests, there is a lot of things which
should have regression tests and still missing there.

I would also appreciate script for fuzzy testing, run time checks
(to avoid things like gcrypt KDF fail) etc.

Running various static/dynamic analyzers and filter out false positives
(and send patches for real problems) helps as well.

And if anyone understand recent changes in automake, I would appreciate
fixes for warnings. (I am thinking also if we could replace it with Cmake:)

Thanks,
Milan
p.s.
If the reason for this is recent misapplied patch for OpenSSL,
the same problem can happen even without goto. Testsuite should detect
such problems...
see e.g. http://www.tedunangst.com/flak/post/a-brief-history-of-one-line-fixes

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

[Arno Wagner]

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