goto patch for cryptsetup?

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

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

Arno Wagner,     Dr. sc. techn., Dipl. Inform.,    Email: [email protected]
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

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

Am 03/07/2014 06:23 AM, schrieb Milan Broz:

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

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.

And there you will get scoping problems and need to put stuff in structs just so you can pass them as argument. Not good.

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

Arno Wagner,     Dr. sc. techn., Dipl. Inform.,    Email: [email protected]
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

i prefer to fo it this way,example[1]:

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

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.

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

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.

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 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: [email protected]
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