PDA

Archiv verlassen und diese Seite im Standarddesign anzeigen : Goto fail


Milton
2014-02-25, 23:39:59
http://www.slate.com/articles/technology/bitwise/2014/02/apple_security_bug_a_critical_flaw_was_extraordinarily_simple.html

Also das hier ist angeblich der Code, der fuer Apple's SSL bug verantwortlich ist:

OSStatus err;

if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0)
goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0)
goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
goto fail;
goto fail;
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
goto fail;

fail:
return err;


Ich finde, dass das selbst ohne den Bug sehr schlechter Code ist. Die Zuweisung innerhalb des if finde ich bescheuert. Von den gotos ganz zu schweigen. Hab zwar ewig nicht mehr in C programmiert, aber kann kaum glauben, dass Profis das so coden.

noid
2014-02-26, 00:02:15
http://www.slate.com/articles/technology/bitwise/2014/02/apple_security_bug_a_critical_flaw_was_extraordinarily_simple.html

Also das hier ist angeblich der Code, der fuer Apple's SSL bug verantwortlich ist:

OSStatus err;

if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0)
goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0)
goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
goto fail;
goto fail;
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
goto fail;

fail:
return err;


Ich finde, dass das selbst ohne den Bug sehr schlechter Code ist. Die Zuweisung innerhalb des if finde ich bescheuert. Von den gotos ganz zu schweigen. Hab zwar ewig nicht mehr in C programmiert, aber kann kaum glauben, dass Profis das so coden.

Auch wenn es hier manche nicht gerne hören, aber die "fehlenden" {} hätten das ggf in einem review sichtbar gemacht, oder dieser Bug wäre so nicht kleben geblieben.
Ich selbst habe schon solche Bugs behoben, wo Klammern einfach nicht gesetzt wurden, weil man es kann. Zusammen mit lausigen Tests kommt das halt durch. Halte den Code für extrem schlecht, aber seltsamerweise für keine Seltenheit.

Coda
2014-02-26, 00:36:27
Das man Klammern weglassen kann ist eines der Todsünden der C-Grammatik.

Shink
2014-02-26, 07:33:13
Formatiert XCode so etwas tatsächlich so unintuitiv? (Das 2. Goto gehört ja wohl einen Tab nach links, dann fällt so etwas sofort auf.)
Davon abgesehen: Ja, Scheißcode.
http://imgs.xkcd.com/comics/goto.png

Pinoccio
2014-02-26, 08:11:10
Fefe hat in seinem Blog (http://blog.fefe.de/?ts=adf5a1ff) dazu auch was recht interessantes angemerkt. Im Prinzip könnte ein Compiler meckern, dass alles nach dem zweiten goto fail; unreachable Code ist und eine Warnung ausgeben.
[Im von Apple benutzten Compiler clang] gibt es die Warnung zwar, aber sie muss manuell angeschaltet werden und ist nicht Teil von -W -Wall -Wextra
[...]
[Die Alternative] gcc hat die Option vor ein paar Jahren deaktiviert (http://gcc.gnu.org/ml/gcc-help/2011-05/msg00360.html). Ja, ganz weg. Die Kommandozeilen-Option gibt es noch, aber sie tut nichts mehr.Schon irgendwie blöd, wenn man diese Möglichkeiten moderner Softwareerstellung nicht nutzt.

mfg

PatkIllA
2014-02-26, 08:16:31
Im Prinzip könnte ein Compiler meckern, dass alles nach dem zweiten goto fail; unreachable Code ist und eine Warnung ausgeben.
Schon irgendwie blöd, wenn man diese Möglichkeiten moderner Softwareerstellung nicht nutzt.
Der C# Compiler macht das auch in der Standardeinstellung. Die einfachen Compilerwarnungen behandeln wir auch hart als Fehler.
Davon auch mal die härteren Möglichkeiten die eine "richtige" Code-Analyse bietet laufen zu lassen konnte ich allerdings praktisch niemanden überzeugen.

#44
2014-02-26, 08:19:02
Ich frage mich gerade, wie ihr den Code umbauen würdet, damit er euren Ansprüchen genügt.

Denn ich muss sagen, dass den Code bis auf die fehlenden Klammern recht nett finde.
Er ist kompakt und vermittelt beim ersten Lesen was da los ist und das ohne Kommentare.

Man könnte die "goto fail" durch "return err" ersetzen.
Dann wäre dem Leser aber nicht mehr unbedingt sofort klar, dass in dem Moment ein Fehlerfall vorliegt. Ausser das dogmatische Vermeiden des goto hat man nichts gewonnen.

Man könnte die Logikoperation und die Ergebnisauswertung trennen.
Was hat man davon, ausser den Code durch 5 zusätzliche Zeilen ohne wirklichen Inhalt schwieriger zu erfassen zu machen?

Man könnte eine Ergebnisvariable nutzen.
Dafür müsste man alle Konditionalen um eine Prüfung dieser erweitern, damit im Fehlerfall auch tatsächlich korrekt abgebrochen wird. Wieder nicht wirklich lesbarer.

Ganon
2014-02-26, 08:28:38
Nein, das ist nicht der Fehlercode von Apple, sondern irgendwas modifiziertes, damit Apple schlechter da steht ;). Denn "goto fail" springt im Originalcode nicht zu "return err", sondern zu einem Part, der den Speicher wieder aufräumt.


static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
uint8_t *signature, UInt16 signatureLen)
{
OSStatus err;
...

if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
goto fail;
goto fail;
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
goto fail;
...

fail:
SSLFreeBuffer(&signedHashes);
SSLFreeBuffer(&hashCtx);
return err;
}


http://opensource.apple.com/source/Security/Security-55471/libsecurity_ssl/lib/sslKeyExchange.c

Das ist der Code und man beachte die "...".

Hier der ganze Block:


static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
uint8_t *signature, UInt16 signatureLen)
{
OSStatus err;
SSLBuffer hashOut, hashCtx, clientRandom, serverRandom;
uint8_t hashes[SSL_SHA1_DIGEST_LEN + SSL_MD5_DIGEST_LEN];
SSLBuffer signedHashes;
uint8_t *dataToSign;
size_t dataToSignLen;

signedHashes.data = 0;
hashCtx.data = 0;

clientRandom.data = ctx->clientRandom;
clientRandom.length = SSL_CLIENT_SRVR_RAND_SIZE;
serverRandom.data = ctx->serverRandom;
serverRandom.length = SSL_CLIENT_SRVR_RAND_SIZE;


if(isRsa) {
/* skip this if signing with DSA */
dataToSign = hashes;
dataToSignLen = SSL_SHA1_DIGEST_LEN + SSL_MD5_DIGEST_LEN;
hashOut.data = hashes;
hashOut.length = SSL_MD5_DIGEST_LEN;

if ((err = ReadyHash(&SSLHashMD5, &hashCtx)) != 0)
goto fail;
if ((err = SSLHashMD5.update(&hashCtx, &clientRandom)) != 0)
goto fail;
if ((err = SSLHashMD5.update(&hashCtx, &serverRandom)) != 0)
goto fail;
if ((err = SSLHashMD5.update(&hashCtx, &signedParams)) != 0)
goto fail;
if ((err = SSLHashMD5.final(&hashCtx, &hashOut)) != 0)
goto fail;
}
else {
/* DSA, ECDSA - just use the SHA1 hash */
dataToSign = &hashes[SSL_MD5_DIGEST_LEN];
dataToSignLen = SSL_SHA1_DIGEST_LEN;
}

hashOut.data = hashes + SSL_MD5_DIGEST_LEN;
hashOut.length = SSL_SHA1_DIGEST_LEN;
if ((err = SSLFreeBuffer(&hashCtx)) != 0)
goto fail;

if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0)
goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0)
goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
goto fail;
goto fail;
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
goto fail;

err = sslRawVerify(ctx,
ctx->peerPubKey,
dataToSign, /* plaintext */
dataToSignLen, /* plaintext length */
signature,
signatureLen);
if(err) {
sslErrorLog("SSLDecodeSignedServerKeyExchange: sslRawVerify "
"returned %d\n", (int)err);
goto fail;
}

fail:
SSLFreeBuffer(&signedHashes);
SSLFreeBuffer(&hashCtx);
return err;

}



Das einzig kritikwürdige an dem Code ist meiner Meinung nach, dass man die geschweiften Klammern weggelassen hat. Code ohne das goto wäre ekliger. Wer einen besseren Vorschlag hat, nur zu. Würde mich interessieren ;)

Ich tippe übrigens auf einen Merge-Fehler.

noid
2014-02-26, 09:12:24
Nein, das ist nicht der Fehlercode von Apple, sondern irgendwas modifiziertes, damit Apple schlechter da steht ;). Denn "goto fail" springt im Originalcode nicht zu "return err", sondern zu einem Part, der den Speicher wieder aufräumt.


static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
uint8_t *signature, UInt16 signatureLen)
{
OSStatus err;
...

if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
goto fail;
goto fail;
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
goto fail;
...

fail:
SSLFreeBuffer(&signedHashes);
SSLFreeBuffer(&hashCtx);
return err;
}


http://opensource.apple.com/source/Security/Security-55471/libsecurity_ssl/lib/sslKeyExchange.c

Das ist der Code und man beachte die "...".

Hier der ganze Block:


static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
uint8_t *signature, UInt16 signatureLen)
{
OSStatus err;
SSLBuffer hashOut, hashCtx, clientRandom, serverRandom;
uint8_t hashes[SSL_SHA1_DIGEST_LEN + SSL_MD5_DIGEST_LEN];
SSLBuffer signedHashes;
uint8_t *dataToSign;
size_t dataToSignLen;

signedHashes.data = 0;
hashCtx.data = 0;

clientRandom.data = ctx->clientRandom;
clientRandom.length = SSL_CLIENT_SRVR_RAND_SIZE;
serverRandom.data = ctx->serverRandom;
serverRandom.length = SSL_CLIENT_SRVR_RAND_SIZE;


if(isRsa) {
/* skip this if signing with DSA */
dataToSign = hashes;
dataToSignLen = SSL_SHA1_DIGEST_LEN + SSL_MD5_DIGEST_LEN;
hashOut.data = hashes;
hashOut.length = SSL_MD5_DIGEST_LEN;

if ((err = ReadyHash(&SSLHashMD5, &hashCtx)) != 0)
goto fail;
if ((err = SSLHashMD5.update(&hashCtx, &clientRandom)) != 0)
goto fail;
if ((err = SSLHashMD5.update(&hashCtx, &serverRandom)) != 0)
goto fail;
if ((err = SSLHashMD5.update(&hashCtx, &signedParams)) != 0)
goto fail;
if ((err = SSLHashMD5.final(&hashCtx, &hashOut)) != 0)
goto fail;
}
else {
/* DSA, ECDSA - just use the SHA1 hash */
dataToSign = &hashes[SSL_MD5_DIGEST_LEN];
dataToSignLen = SSL_SHA1_DIGEST_LEN;
}

hashOut.data = hashes + SSL_MD5_DIGEST_LEN;
hashOut.length = SSL_SHA1_DIGEST_LEN;
if ((err = SSLFreeBuffer(&hashCtx)) != 0)
goto fail;

if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0)
goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0)
goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
goto fail;
goto fail;
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
goto fail;

err = sslRawVerify(ctx,
ctx->peerPubKey,
dataToSign, /* plaintext */
dataToSignLen, /* plaintext length */
signature,
signatureLen);
if(err) {
sslErrorLog("SSLDecodeSignedServerKeyExchange: sslRawVerify "
"returned %d\n", (int)err);
goto fail;
}

fail:
SSLFreeBuffer(&signedHashes);
SSLFreeBuffer(&hashCtx);
return err;

}



Das einzig kritikwürdige an dem Code ist meiner Meinung nach, dass man die geschweiften Klammern weggelassen hat. Code ohne das goto wäre ekliger. Wer einen besseren Vorschlag hat, nur zu. Würde mich interessieren ;)

Ich tippe übrigens auf einen Merge-Fehler.

Es gibt an dieser Stelle keine Notwendigkeit eines gotos. Ich habe keine Lust jetzt das groß umzuschreiben, aber das Aufräumen hätte man auch anders lösen können.

Coda
2014-02-26, 09:17:24
Formatiert XCode so etwas tatsächlich so unintuitiv? (Das 2. Goto gehört ja wohl einen Tab nach links, dann fällt so etwas sofort auf.)
Davon abgesehen: Ja, Scheißcode.
http://imgs.xkcd.com/comics/goto.png
Das goto an sich finde ich für diese Verwendung okay. Das ist in C üblich zum Aufräumen im Abruchsfall, weil man keine Destruktoren hat.

Ganon
2014-02-26, 09:28:04
Es gibt an dieser Stelle keine Notwendigkeit eines gotos.

Die Alternative die mir einfällt ist eine logische Variable zu setzen und an jedem Block-Ende zu prüfen, ob man aufhören sollte. Das verschachelt die Struktur mal eben um 1-2 Ebenen tiefer und macht die Sache weniger lesbar und fehleranfälliger (man könnte Fälle übersehen).

Und den ganzen Spaß zu verUNDen und verODERn hilft zwar vllt. in dem kleinen Auszug, aber nicht im gesamten Code.

Exxtreme
2014-02-26, 09:33:45
Nein, das ist nicht der Fehlercode von Apple,
Das einzig kritikwürdige an dem Code ist meiner Meinung nach, dass man die geschweiften Klammern weggelassen hat. Code ohne das goto wäre ekliger. Wer einen besseren Vorschlag hat, nur zu. Würde mich interessieren ;)

Ekliger zwar nicht aber länger.

Statt:


if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0)
goto fail;


müsste man:


if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0) {
SSLFreeBuffer(&signedHashes);
SSLFreeBuffer(&hashCtx);
return err;
}


für jedes goto machen. Alternativ könnte man sich eine eigene Aufräum-Prozedur schreiben und die jedes Mal einsetzen.

noid
2014-02-26, 09:36:34
Die Alternative die mir einfällt ist eine logische Variable zu setzen und an jedem Block-Ende zu prüfen, ob man aufhören sollte. Das verschachelt die Struktur mal eben um 1-2 Ebenen tiefer und macht die Sache weniger lesbar und fehleranfälliger (man könnte Fälle übersehen).

Und den ganzen Spaß zu verUNDen und verODERn hilft zwar vllt. in dem kleinen Auszug, aber nicht im gesamten Code.

Ob es fehleranfälliger wäre kann man nicht sagen, immerhin hat die goto-Varianten wohl ein Problem verursacht.

Aber eigentlich kann man sagen, dass as 2 Dinge gibt, die wirklich schlecht sind:
1) {}, so schwer ist das nicht und eigentlich liest man viel zu viel Code, welcher keine solche bei single-line ifs verwendet
2) keine ausreichenden unittests, oder keine. Sicherlich kann man nicht alles testen, aber bei so einer Funktion halte ich das eigentlich für unabdingbar. Wenn ich überlege für welchen simplen Käse hier tests geschrieben werden. Weil man sicher sein muss.

Monger
2014-02-26, 09:41:59
Ich hatte erst vor kurzem auf Arbeit einen Fall der mich daran erinnert hat. Ein Switch/Case ohne Case..Else - wenn man einen ungültigen Wert angegeben hat, ist die Funktion immer mit einem gültig vorbesetzten Ergebnis rausgesprungen. Furchtbar.
Allerdings war das im Gegensatz zu Apple hier pure Blödheit, da hat jemand aktiv geschlampt.

Schon richtig: das hier sieht verdächtig nach Mergefehler o.ä. aus. Das kommt schon mal vor. Trotzdem: in einer Hochsprache wäre sowas wahrscheinlich nicht passiert.

Ganon
2014-02-26, 11:02:02
Ob es fehleranfälliger wäre kann man nicht sagen, immerhin hat die goto-Varianten wohl ein Problem verursacht.

Ich würde eher sagen die fehlenden {} haben den Fehler verursacht und nicht das goto an sich.

Auch ein
if(bla)
return cleanup(var1, var2);
return cleanup(var1, var2);

oder ein

if(bla)
exit = true;
exit = true;

und ein:

if(bla)
clean();
clean();

hätte den Fehler verursacht. Letzteres wäre sogar noch schlimmer gewesen, ggf. Egal welche Methode du wählst... überall der gleiche Fehler.

@Exxtreme

Das wäre eine massive Code-Dopplung im gesamten Quelltext.

Monger
2014-02-26, 11:17:16
if(bla)
return cleanup(var1, var2);
return cleanup(var1, var2);

Das hätte je nach Compilereinstellung mindestens mal ne Warnung oder einen Fehler wegen unerreichbarem Code gegeben. Goto ist in anderen Hochsprachen nicht grundlos ausgestorben.

Ganon
2014-02-26, 11:19:05
Das hätte je nach Compilereinstellung mindestens mal ne Warnung oder einen Fehler wegen unerreichbarem Code gegeben. Goto ist in anderen Hochsprachen nicht grundlos ausgestorben.

Siehe dazu ein paar Beiträge weiter oben. In gcc ist die Warnung nicht mehr vorhanden und in clang muss man sie erst aktivieren.

Exxtreme
2014-02-26, 11:21:30
@Exxtreme

Das wäre eine massive Code-Dopplung im gesamten Quelltext.
Das ist richtig. Aber in dem Falle müsste man Klammern setzen und der Bug würde nicht auftreten.

Wobei diese Routine sowieso noch eine andere Merkwürdigkeit hat.

Oben wird die Variable "err" zwar definiert, hat aber erstmal keinen definierten Wert. Im Fehlerfall bekommt diese Variable nie einen definierten Wert weil der Block, der ihr einen Wert zuweist per goto übersprungen wird. Trotzdem wird "err" per return zurück gegeben, auch im Fehlerfall. Hmmm ...

Ups, übersehen, dass die in der if-Anweisung "err" was zuweisen.

#44
2014-02-26, 11:24:15
Oben wird die Variable "err" zwar definiert, hat aber erstmal keinen definierten Wert. Im Fehlerfall bekommt diese Variable nie einen definierten Wert weil der Block, der ihr einen Wert zuweist per goto übersprungen wird. Trotzdem wird "err" per return zurück gegeben, auch im Fehlerfall. Hmmm ...
err wird vor jedem goto ein Wert zugewiesen.

€: Du hast's ja schon bemerkt :)

mekakic
2014-02-26, 11:45:17
Goto ist in anderen Hochsprachen nicht grundlos ausgestorben.
Aber das ist ja keine andere sondern eben C. :) Finde den Code ansonsten auch brauchbar... evtl. die Zuweise aus dem if Teil rausziehen, aber ansonsten neben eben dem offensichtlichen Fehler kein Problem

Ectoplasma
2014-02-26, 12:21:38
Wenn man Code-Zeilen sparen möchte, kann man es auch so schreiben. Auch nicht der beste Stil, aber es geht knapper formuliert und ohne goto. Oder man packt den Aufräm-Code in eine eigene Funktion.


OSStatus err;

err = ReadyHash(&SSLHashSHA1, &hashCtx);
err = !err ? SSLHashSHA1.update(&hashCtx, &clientRandom) : err;
err = !err ? SSLHashSHA1.update(&hashCtx, &serverRandom) : err;
err = !err ? SSLHashSHA1.update(&hashCtx, &signedParams) : err;
err = !err ? SSLHashSHA1.final(&hashCtx, &hashOut) : err;

if (err) {
SSLFreeBuffer(&signedHashes);
SSLFreeBuffer(&hashCtx);
}

return err;

Gast
2014-02-26, 13:57:56
Dann doch gleich

err = SSLHashSHA1.update(&hashCtx, &clientRandom) || ... ;

Ganon
2014-02-26, 14:00:48
Für die beiden letzten Vorschläge: Guckt euch einfach mal den ganzen Code an und nicht nur den kleinen Auszug ;)

Ectoplasma
2014-02-26, 14:09:47
Ja, aber was ändert der ganze Code am Prinzip der letzten beiden Vorschläge? Wie dem auch sei, falls das zusätzliche GOTO tatsächlich ein Merge-Fehler war (wobei ich selbst in 18 Jahren Nutzung solcher Tools eine Zeilenverdopplung noch nie gesehen habe), hat man sowieso Probleme, ob der Code nun häßlich ist oder nicht.

Ganon
2014-02-26, 14:29:49
Ja, aber was ändert der ganze Code am Prinzip der letzten beiden Vorschläge?

Dass du jetzt überall hinter jedem Block "if err" abfragen musst, damit du nicht weitermachst. Und irgendwelche hard-returns mittem im Code sind imo auch nicht besser für die Lesbarkeit/Fehleranfälligkeit. Zusätzlich zur weiteren Code-Dopplung natürlich.

Wie dem auch sei, falls das zusätzliche GOTO tatsächlich ein Merge-Fehler war (wobei ich selbst in 18 Jahren Nutzung solcher Tools eine Zeilenverdopplung noch nie gesehen habe), hat man sowieso Probleme, ob der Code nun häßlich ist oder nicht.

Muss ja nicht zwangsläufig automatisch passiert sein. Das kann ja auch ein manueller Merge gewesen sein. git fügt dir da ja z.B.

<<<<<<HEAD
code1
>>>>>>>>>>
code2
<<<<<<<<<<

in den Code ein, für's mergen. Hier einfach vergessen eine Zeile zu löschen, oder was auch immer.

#44
2014-02-26, 14:59:05
Dass du jetzt überall hinter jedem Block "if err" abfragen musst, damit du nicht weitermachst. Und irgendwelche hard-returns mittem im Code sind imo auch nicht besser für die Lesbarkeit/Fehleranfälligkeit. Zusätzlich zur weiteren Code-Dopplung natürlich.
Nein, Ectoplasmas Ansatz lässt sich mit nur einem einzigen weiteren if() (nämlich bei sslRawVerify) umsetzen.

Trotzdem hast du recht, das macht es nicht besser: Denn man versteckt nur die Intention des Codes. Nämlich im Fehlerfall einfach abbrechen.
Man muss dadurch die komplette Methode lesen und verstehen um sicher zu sein, dass abgebrochen wird und nicht etwa später doch noch etwas passiert.

Sowas ist ein Minenfeld bei Wartungsarbeiten.

Das selbe gilt für eine Verkettung mittels ||. Man will ja keine Logikoperation ausführen, sondern nur konditional abbrechen - für den Leser ebenfalls schwerer nachzuvollziehen als im Original.

Clevere Lösungen verstoßen halt in der Regel gegen KISS. Und waren die eigentliche Ursache, warum goto heute verpöhnt ist.
An der Verwendung von goto in diesem Code ist aber nichts cleveres, im Gegenteil - es ist simpel.

Ectoplasma
2014-02-26, 15:00:51
Dass du jetzt überall hinter jedem Block "if err" abfragen musst ...


Schau doch mal richtig hin, muss man dann nur einmal, wenn man sich Mühe gibt :wink:


Muss ja nicht zwangsläufig automatisch passiert sein. Das kann ja auch ein manueller Merge gewesen sein. git fügt dir da ja z.B.

<<<<<<HEAD
code1
>>>>>>>>>>
code2
<<<<<<<<<<

in den Code ein, für's mergen. Hier einfach vergessen eine Zeile zu löschen, oder was auch immer.

Ja, wenn man komplett blind ist und den Pragrammierschlampentitel hat, dann passiert einem das. Bei manuellem Mergen sollte man ja wohl dreimal Vorsicht walten lassen. Aber ich glaube, darüber könnte man jetzt eine Endlosdiskussion führen. Das lass ich mal lieber.

Ectoplasma
2014-02-26, 15:03:53
Trotzdem hast du recht, das macht es nicht besser: Denn man versteckt nur die Intention des Codes. Nämlich im Fehlerfall einfach abbrechen.

Richtig, aber ich sagte ja, auch kein guter Stil.Das Beste wäre es, man könnte mit Exceptions arbeiten. Das wäre sicher, robust und sehr gut wartbar. Aber ist halt leider "nur" C.

Milton
2014-02-26, 18:10:37
Also ich bin wie gesagt kein Programmierer, aber goto nach einem einzeiligen if scheint mir bescheuert. Da C kein elseif unterstuetzt, wuerde ich das als totaler Amateur so schreiben:


if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) == 0)
{
if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) == 0)
{
if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) == 0)
{
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) == 0)
{
err = SSLHashSHA1.final(&hashCtx, &hashOut));
}
}
}
}

if(err !=0)
{
SSLFreeBuffer(&signedHashes);
SSLFreeBuffer(&hashCtx);
return err;
}


Oder hab ich da einen Denkfehler?

EDIT: Hier noch die Version ohne Zuweisung im If, was wie ich finde die Lesbarkeit erschwert (siehe thread)


err = ReadyHash(&SSLHashSHA1, &hashCtx);

if (err == 0)
{
err = SSLHashSHA1.update(&hashCtx, &clientRandom);

if (err == 0)
{

err = SSLHashSHA1.update(&hashCtx, &serverRandom);

if (err == 0)
{
err = SSLHashSHA1.update(&hashCtx, &signedParams);

if (err == 0)
{
err = SSLHashSHA1.final(&hashCtx, &hashOut));
}
}
}
}

if(err !=0)
{
SSLFreeBuffer(&signedHashes);
SSLFreeBuffer(&hashCtx);
return err;
}

PHuV
2014-02-27, 09:43:02
Also ich bin wie gesagt kein Programmierer, aber goto nach einem einzeiligen if scheint mir bescheuert. Da C kein elseif unterstuetzt, wuerde ich das als totaler Amateur so schreiben:

Wenn die Fehler unabhängig voneinander auftauchen können, hast Du mit dieser Konstruktion verloren.
Wenn man Code-Zeilen sparen möchte, kann man es auch so schreiben. Auch nicht der beste Stil, aber es geht knapper formuliert und ohne goto. Oder man packt den Aufräm-Code in eine eigene Funktion.
So hätte ich es auch gemacht.

Gast
2014-02-27, 11:38:45
Hab mir den Code nicht komplett durchgelesen aber

SSLHashSHA1.update

klingt wie eine Funktion die eine direkte Operation durchführt oder ein Objekt mit Daten versorgt und nicht einfach einen Status abfragt, aber keine Ahnung.

Auf alle Fälle sieht es danach aus als hätte man die Fehlerbehandlung erst im Nachhinein hinzugefügt.

mekakic
2014-02-27, 14:10:18
Also ich bin wie gesagt kein Programmierer, aber goto nach einem einzeiligen if scheint mir bescheuert. Da C kein elseif unterstuetzt, woot? Vielleicht kein "elseif" aber "else if" immerhin schon. :freak:

wuerde ich das als totaler Amateur so schreiben:Das ist nicht äquivalent. Und ich verstehe gar nicht was alle da mit dem goto für ein Problem haben, das ist doch in dem Zusammenhang absolut okay. Oder diejenigen sollten sich am besten nie den Linux Kernel anschauen oder man wird nie wieder ruhig schlafen.

HajottV
2014-02-27, 14:47:08
Das man Klammern weglassen kann ist eines der Todsünden der C-Grammatik.

if (foo) bar;

als Einzeiler(!) finde ich prima. Das fail ist, dass der Compiler oder eine andere Form der statischen Analyse das nicht bemerkt hat.

Milton
2014-02-27, 14:57:40
woot? Vielleicht kein "elseif" aber "else if" immerhin schon. :freak:


Ja, aber man kann doch nicht mehrere else if hintereinander weg schreiben, sondern muss schachteln. Also if else {if else {if else}}, nicht if else if {} else if {}. Oder täuscht mich da die Erinnerung?


Das ist nicht äquivalent. Und ich verstehe gar nicht was alle da mit dem goto für ein Problem haben, das ist doch in dem Zusammenhang absolut okay. Oder diejenigen sollten sich am besten nie den Linux Kernel anschauen oder man wird nie wieder ruhig schlafen.

Wieso ist das nicht äquivalent? Der Programmfluss ist doch genau der gleiche, oder täusche ich mich da? Bitte klär mich auf, lerne ja gerne dazu.

mekakic
2014-02-27, 15:11:54
if (foo) bar;

als Einzeiler(!) finde ich prima. Das fail ist, dass der Compiler oder eine andere Form der statischen Analyse das nicht bemerkt hat.
Normalerweise hätte eigentlich nach dem Checkin und erstem Build bereits ein Continuous Integration Test losheulen müssen. Oder ein nightly test oder was auch immer. Derartige Fehler können beim mergen ruhig passieren, aber das muß auffallen. Wäre aber auch nicht das erste Mal, dass man erst Durch einen Bug merkt, dass der Test fehlerhaft ist.

Ja, aber man kann doch nicht mehrere else if hintereinander weg schreiben, sondern muss schachteln. Also if else {if else {if else}}, nicht if else if {} else if {}. Oder täuscht mich da die Erinnerung?
klar, geht das selbst in C89.


Wieso ist das nicht äquivalent? Der Programmfluss ist doch genau der gleiche, oder täusche ich mich da? Bitte klär mich auf, lerne ja gerne dazu.Hab mich da gerade selber versehen. :redface: Dennoch würde ich goto bevorzugen, weil die Verschachtelung es eher schwieriger lesbar macht. Das ich mich da gerade auch wieder versehen habe, sollte das Argument genug sein. :freak:

HajottV
2014-02-27, 16:02:05
Normalerweise hätte eigentlich nach dem Checkin und erstem Build bereits ein Continuous Integration Test losheulen müssen. Oder ein nightly test oder was auch immer. Derartige Fehler können beim mergen ruhig passieren, aber das muß auffallen.

In der Tat. Dass es zu so kritischen Code keine oder unzureichende automatische Tests gibt, lässt einem die Haare zu Berge stehen. Wenn ich mir überlege zu was für eigentlich Trivialcode ich automatische Tests schreibe... :confused:

Letztendlich ist es egal/nicht so wichtig, ob man jetzt klammert oder nicht (das zeigen auch die unterschiedlichen Stile, die hier als Alternativen präsentiert wurden)... es muss eine wie auch immer geartete Qualitätssicherung geben, die ein bisschen mehr macht als: "compiliert => :uup:".

Gauron Kampeck
2014-02-27, 18:08:55
Wenn man Code-Zeilen sparen möchte, kann man es auch so schreiben. Auch nicht der beste Stil, aber es geht knapper formuliert und ohne goto. Oder man packt den Aufräm-Code in eine eigene Funktion.


OSStatus err;

err = ReadyHash(&SSLHashSHA1, &hashCtx);
err = !err ? SSLHashSHA1.update(&hashCtx, &clientRandom) : err;
err = !err ? SSLHashSHA1.update(&hashCtx, &serverRandom) : err;
err = !err ? SSLHashSHA1.update(&hashCtx, &signedParams) : err;
err = !err ? SSLHashSHA1.final(&hashCtx, &hashOut) : err;

if (err) {
SSLFreeBuffer(&signedHashes);
SSLFreeBuffer(&hashCtx);
}

return err;

Find ich persönlich schlechter lesbar, als das Apple-Beispiel. Die Verwendung des gotos dort ist ein übliches Idiom in C und findet sich beispielsweise im Linux-Kernel geschätzt tausendfache Verwendung.

Viel viel wichtiger ist, dass nach dem if die Klammern gesetzt werden, dann kommen solche Themen gar nicht erst auf.

noid
2014-02-27, 19:43:15
Find ich persönlich schlechter lesbar, als das Apple-Beispiel. Die Verwendung des gotos dort ist ein übliches Idiom in C und findet sich beispielsweise im Linux-Kernel geschätzt tausendfache Verwendung.

Viel viel wichtiger ist, dass nach dem if die Klammern gesetzt werden, dann kommen solche Themen gar nicht erst auf.

Keine Klammern ist aber auch im LinuxKernel ein Mittel welches extrem oft Anwendung findet. Weil Klammern nehmen ja Platz weg und sind Tipparbeit.

Milton
2014-02-27, 19:43:57
In Bruce Schneier's Blog gibt es eine Diskussion, ob das ganze ein absichtlich platzierter Bug war (NSA etc.): https://www.schneier.com/blog/archives/2014/02/was_the_ios_ssl.html#comments

Und da hat jemand code gepostet, der fast identisch zu meinem ist. :-)

Zu gotos: Ich finde gotos ok, wenn man unbedingt aus tief verschachtelten Ebenen ausbrechen muss. Wenn man da auf mehreren Ebenen sequentiell eine Abbruchsvariable auswerten muss, wird das schnell kompliziert. Aber ein goto in der selben Ebene finde ich persönlich sehr unelegant und einfach schlechten Stil. Es sei denn, man programmiert in GW BASIC.

Gauron Kampeck
2014-02-27, 19:57:50
Keine Klammern ist aber auch im LinuxKernel ein Mittel welches extrem oft Anwendung findet. Weil Klammern nehmen ja Platz weg und sind Tipparbeit.
Mit dem Unterschied, dass goto ein bewusst propagiertes Konstrukt ist und fehlende Klammern nur hingenommen werden.

HajottV
2014-02-27, 20:17:35
...

Eine sehr gute Lösung (ich würde sogar noch ein paar Spaces einfügen,um , und : zu alignieren).

Deine Formattierung zeigt den Flow und die Unterschiede. Finde ich top! :up:

Guter Code schmeichelt dem Hirn und dem Auge.

Monger
2014-02-27, 21:19:01
Zu gotos: Ich finde gotos ok, wenn man unbedingt aus tief verschachtelten Ebenen ausbrechen muss. Wenn man da auf mehreren Ebenen sequentiell eine Abbruchsvariable auswerten muss, wird das schnell kompliziert. Aber ein goto in der selben Ebene finde ich persönlich sehr unelegant und einfach schlechten Stil. Es sei denn, man programmiert in GW BASIC.
Ich will jetzt keinen GOTO Glaubenskrieg vom Zaun brechen, aber ich würde genau umgekehrt argumentieren. Wenn es der Lesbarkeit innerhalb einer Methode dient - was solls. Deshalb erlaubt Java ja z.B. auch gelabelte Schleifen.
Aber die große Gefahr an Goto ist ja eben, dass man damit die Isolierung von Methoden und damit deren Kontrollfluss durchbricht. Man schafft Abhängigkeiten zwischen Methoden die sich anhand ihrer Signaturen nicht erkennen lassen.
Vielleicht bin ich da zu sehr hochsprachenverwöhnt, aber ich halte das für Teufelszeug.

Lokadamus
2014-02-28, 20:52:39
Ich will jetzt keinen GOTO Glaubenskrieg vom Zaun brechen, aber ich würde genau umgekehrt argumentieren. Wenn es der Lesbarkeit innerhalb einer Methode dient - was solls. Deshalb erlaubt Java ja z.B. auch gelabelte Schleifen.
Aber die große Gefahr an Goto ist ja eben, dass man damit die Isolierung von Methoden und damit deren Kontrollfluss durchbricht. Man schafft Abhängigkeiten zwischen Methoden die sich anhand ihrer Signaturen nicht erkennen lassen.
Vielleicht bin ich da zu sehr hochsprachenverwöhnt, aber ich halte das für Teufelszeug.Goto ist selbst unter einer Batch böse. Da brauch man nicht drüber reden, dass es unter "höchsprachen" (man nehme bitte sächsisch an dieser Stelle) zu unerwarteten Fehlern kommt. Hier setzt wieder die Fähigkeit des Coders ein, der erkennt, dass hier irgendwas krumm ist.

maximAL
2014-02-28, 22:20:51
OSStatus err;

if((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0 ||
(err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0 ||
(err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0 ||
(err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0 ||
(err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) {
SSLFreeBuffer(&signedHashes);
SSLFreeBuffer(&hashCtx);
return err;
}

Milton
2014-02-28, 22:40:09
OSStatus err;

if((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0 ||
(err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0 ||
(err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0 ||
(err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0 ||
(err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) {
SSLFreeBuffer(&signedHashes);
SSLFreeBuffer(&hashCtx);
return err;
}

Welchen Wert hat err denn dann am Ende? Und was wenn err nicht nur 1/0 ist, sondern integer Fehler codes? Und, was wenn aus Laufzeitgruenden die sequentielle Fehlerabfrage beabsichtigt ist?

maximAL
2014-02-28, 23:01:26
Welchen Wert hat err denn dann am Ende? Und was wenn err nicht nur 1/0 ist, sondern integer Fehler codes? Und, was wenn aus Laufzeitgruenden die sequentielle Fehlerabfrage beabsichtigt ist?
Ich bin mir fast sicher, dass der Standard vorschreibt, dass die Ausdrücke in der Reihenfolge ausgewertet werden in der sie dastehen und zudem bei einer Oder-Verknüpfung nach dem ersten wahren Ausdruck nichts mehr ausgewertet wird. Und wo siehst du das Problem bei Integern (natürlich sind das Integer)?

#44
2014-03-01, 07:57:14
OSStatus err;

if((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0 ||
(err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0 ||
(err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0 ||
(err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0 ||
(err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) {
}
SSLFreeBuffer(&signedHashes);
SSLFreeBuffer(&hashCtx);
return err;

fix'd. Wie du siehst ist so ein Konstrukt missverständlich in seiner Aussage.

Abgesehen davon ist es so nicht ohne Codedopplung auf das Original übertragbar.

Oid
2014-03-01, 09:36:19
Wenn man Code-Zeilen sparen möchte, kann man es auch so schreiben. Auch nicht der beste Stil, aber es geht knapper formuliert und ohne goto. Oder man packt den Aufräm-Code in eine eigene Funktion.


OSStatus err;

err = ReadyHash(&SSLHashSHA1, &hashCtx);
err = !err ? SSLHashSHA1.update(&hashCtx, &clientRandom) : err;
err = !err ? SSLHashSHA1.update(&hashCtx, &serverRandom) : err;
err = !err ? SSLHashSHA1.update(&hashCtx, &signedParams) : err;
err = !err ? SSLHashSHA1.final(&hashCtx, &hashOut) : err;

if (err) {
SSLFreeBuffer(&signedHashes);
SSLFreeBuffer(&hashCtx);
}

return err;


Also das finde ich noch schlimmer als das Original :D Ich bin zwar auch kein Freund von goto, aber in seltenen Fällen kann es Sinn machen. Und sehr viel schlimmer als break/continue-Gemurkse ist es auch nicht.

Man kann mit goto ziemlich viel Schindluder betreiben. Deswegen ist es auch aus gutem Grund verpöhnt. Aber in C kann man sich auch mit vielen anderen Dingen ins Knie schießen, da steht goto nicht alleine da.

mekakic
2014-03-03, 10:40:22
Ich bin mir fast sicher, dass der Standard vorschreibt, dass die Ausdrücke in der Reihenfolge ausgewertet werden in der sie dastehen und zudem bei einer Oder-Verknüpfung nach dem ersten wahren Ausdruck nichts mehr ausgewertet wird.Jepp. short circuiting bool ops sind im Standard drin für && und ||. Allerdings nicht, falls Du dort Instanzen verknüpfst und irgendwer auf die Tolle Idee kommt operator&& oder operator|| für diese Klassen zu implementieren.

Aber in C kann man sich auch mit vielen anderen Dingen ins Knie schießen, da steht goto nicht alleine da.Demnach müsste man die Verwendung von Pointern ächten, wenn es nach dem Potential für möglichen Mistbau geht.

medi
2014-03-03, 13:20:53
Ich frage mich gerade, wie ihr den Code umbauen würdet, damit er euren Ansprüchen genügt.

Denn ich muss sagen, dass den Code bis auf die fehlenden Klammern recht nett finde.
Er ist kompakt und vermittelt beim ersten Lesen was da los ist und das ohne Kommentare.

Also bei uns in der Firma gibts die Regel sprechenden Code zu schreiben. Solche If Konstrukte sind hier verpönt, da man beim debuggen erst einmal herausfinden muss was sich der Programmierer da gedacht hat. Nur weil da ne Anweisung steht heisst das nicht, dass es auch die ist die man will.

z.B.
bool isEingabeFalsch = ...
bool isServerNichtErreichbar = ...
bool isWasAuchImmerNichtErfüllt = ..

if ( isEingabeFalsch ||
isServerNichtErreichbar ||
isWasAusImmerNichtErfüllt )
{
[optional] Logeintrag
[optional] User feedback
Abbruch
}

<eigentlicher code>

Bei dem Code von Apple da oben muss ich immer erst einmal gucken was der eigentlich macht und überlegen ob das auch Sinn macht. Finde ich bei der Wartung unvorteilhaft.

Ganon
2014-03-03, 14:55:40
@medi

Der Ansatz würde dann aber nach einem Code verlangen den Milton oben geschrieben hat. Weil wenn der erste Befehl fehl schlägt, sollen ja alle anderen nicht mehr ausgeführt werden.

Oder in Code ausgedrückt:

bool darfNutzerSichEinloggen = can_login(user);
bool loginErfolgreich = login(user);
bool hatZugriff = has_access(user);


Macht so einfach keinen Sinn. ;) Wenn dann muss es irgendwie so aussehen wie hier vorgeschlagen wurde, oder wie bei Apple.

Ectoplasma
2014-03-03, 16:41:44
Erst einmal würde ich euch dieses Denglisch um die Ohren hauen. :freak:

Was nützt es mir, das Fehlschlagen von z.B. 'ReadyHash', in eine weitere Variable zu pressen, wenn daraus sowieso ein Komplettabbruch erfolgt. Ok, man könnte es evtl. zum Loggen im Aufräumblock verwenden. Irgendwie macht es das Ganze aber nicht schöner und wenn ich Debugge, dann weiss ich auch so, welche der Funktionen fehlgeschlagen ist. Nach meiner Meinung wäre die sauberste Lösung, dass Ganze in ein C++ File zu packen, und die SSLHashSHA1.<Methoden> dazu zu bewegen, dass sie Exceptions werfen (Evtl. mit einem Wrapper).


try {
ReadyHash(&SSLHashSHA1, &hashCtx);
SSLHashSHA1.update(&hashCtx, &clientRandom);
SSLHashSHA1.update(&hashCtx, &serverRandom);
SSLHashSHA1.update(&hashCtx, &signedParams);
SSLHashSHA1.final(&hashCtx, &hashOut);
} catch (const SSLHashSHA1Exception &ex) {
// log exception for example
printf("%s\n", ex.what());
// cleanup
SSLFreeBuffer(&signedHashes);
SSLFreeBuffer(&hashCtx);
// and throw
throw ex;
}


Den Try/Catch Block könnten man natürlich auch ganz weglassen, wenn sich der Aufrufer um das Fangen kümmert. Bei modernen Compilern haben Exceptions übrigens meistens Zero Overhead.

noid
2014-03-03, 17:30:39
Der cleanup muss immer erfolgen. Daran merkt man auch wie unintuitiv der Foto code eigentlich ist :)

Ectoplasma
2014-03-03, 17:39:21
Der cleanup muss immer erfolgen. Daran merkt man auch wie unintuitiv der Foto code eigentlich ist :)

Ha, hast Recht, zum Label kommt er ja immer. Gut aufgepasst :biggrin:

Aber auch das könnte man mit C++ ähnlich wie bei einem Finally lösen. Man bräuchte eine Cleanup Klasse, bei der du im DTOR alles bereinigst.

medi
2014-03-03, 18:18:49
Erst einmal würde ich euch dieses Denglisch um die Ohren hauen. :freak:

Was nützt es mir, das Fehlschlagen von z.B. 'ReadyHash', in eine weitere Variable zu pressen, wenn daraus sowieso ein Komplettabbruch erfolgt. Ok, man könnte es evtl. zum Loggen im Aufräumblock verwenden. Irgendwie macht es das Ganze aber nicht schöner und wenn ich Debugge, dann weiss ich auch so, welche der Funktionen fehlgeschlagen ist. Nach meiner Meinung wäre die sauberste Lösung, dass Ganze in ein C++ File zu packen, und die SSLHashSHA1.<Methoden> dazu zu bewegen, dass sie Exceptions werfen (Evtl. mit einem Wrapper).


try {
ReadyHash(&SSLHashSHA1, &hashCtx);
SSLHashSHA1.update(&hashCtx, &clientRandom);
SSLHashSHA1.update(&hashCtx, &serverRandom);
SSLHashSHA1.update(&hashCtx, &signedParams);
SSLHashSHA1.final(&hashCtx, &hashOut);
} catch (const SSLHashSHA1Exception &ex) {
// log exception for example
printf("%s\n", ex.what());
// cleanup
SSLFreeBuffer(&signedHashes);
SSLFreeBuffer(&hashCtx);
// and throw
throw ex;
}


Den Try/Catch Block könnten man natürlich auch ganz weglassen, wenn sich der Aufrufer um das Fangen kümmert. Bei modernen Compilern haben Exceptions übrigens meistens Zero Overhead.

Also ich hab mal gelernt, dass man Exceptions meiden sollte wie der Teufel das Weihwasser da die enorm kosten ... zumindest bei C#.

@Ganon
Nein ich würde eher in diese Richtung gehen:

bool darfNutzerSichEinloggen = can_login(user);

bool loginErfolgreich = darfNutzSichEinloggen && login(user);

bool hatZugriff = loginErfolgreich && has_access(user);

if ( !Zugriff )
{
return;
}

<eigentlicher Code>


Beim Prüfen sollte das Programm abbrechen sobald der vorherige Wert false ergab. So spart man Laufzeit und der Code ist trotzdem leserlich

Marscel
2014-03-03, 19:00:04
Also ich hab mal gelernt, dass man Exceptions meiden sollte wie der Teufel das Weihwasser da die enorm kosten ... zumindest bei C#.

Ach Käse. Wenn du bei echt performancekritischen Sachen lieber Exceptions als Rückgabecodes nutzt, dann ist das sicherlich etwas Cache- und Page-unfreundlicher.

Aber sonst ist das eher eine Urban Legend.

Monger
2014-03-03, 19:58:21
Also ich hab mal gelernt, dass man Exceptions meiden sollte wie der Teufel das Weihwasser da die enorm kosten ... zumindest bei C#.
...
Beim Prüfen sollte das Programm abbrechen sobald der vorherige Wert false ergab. So spart man Laufzeit und der Code ist trotzdem leserlich
Ist das ernst gemeint? In einem anderen Unterforum hätte ich sowas als Trollversuch gewertet.

Exceptions sind nur dann etwas teurer wenn sie tatsächlich geworfen werden. Sie sind für Ausnahmefälle gedacht, nicht zur regulären Signalisierung. Sie entwirren den Kontrollfluss aber mitunter drastisch, und sparen damit im Endeffekt Laufzeit für unnötige Überprüfungen.

Status Codes als Rückgabewerte gelten dagegen mittlerweile als Antimuster. Die Aussagekraft die man aus ihnen im Fehlerfall gewinnt ist oft nahe Null, und es passiert schnell dass man deren Auswertung vergisst.
Das gilt für dein Codebeispiel im übrigen ja nochmal ganz besonders, weil a) du mit jeder Zuweisung die Information des vorherigen Methodenaufruf ignorierst und du b) den Rückgabewert schlicht still schluckst. Wenn entweder can_login oder Login oder has_access scheitern, stehst du anschließend genauso schlau da wie zuvor.

PatkIllA
2014-03-03, 20:00:38
Ach Käse. Wenn du bei echt performancekritischen Sachen lieber Exceptions als Rückgabecodes nutzt, dann ist das sicherlich etwas Cache- und Page-unfreundlicher.

Aber sonst ist das eher eine Urban Legend.
Nope. Wenn die unter Umständen wirklich dauernd fliegen (und damit auch keine Ausnahmen mehr sind) ist das deutlich langsamer.
Schon selbst gebencht und auch auf der Arbeit bei einem Parser erlebt, der ein Subset konnte und den nicht unterstützten Teil ausgelassen hat. Das war nicht mehr zu benutzen, wenn da die passenden Daten ankamen.

Als wirkliche Fehlersignalisierung käme ich aner auch nicht auf die Idee Rückgabe codes zu nehmen.

Wir bis neulich noch eine Exception drin, die im nicht konfiguriert Fall in Modulen auftrat. Das war einfach nur nervig beim Debuggen, weil man dann nicht bei allen Exceptions anhalten konnte, um "seinen" Fehler zu finden, da die Exceptions gefangen und weitergeworfen oder eingepackt wurden.

Trap
2014-03-03, 20:03:31
Also ich hab mal gelernt, dass man Exceptions meiden sollte wie der Teufel das Weihwasser da die enorm kosten ... zumindest bei C#.
Das haben die Performance-Leute von MS auf Channel9 auch vor 1-2 Jahren noch gesagt, ab 100 Exceptions pro Sekunde sollte man die Auswirkung der Exceptions auf die Laufzeit untersuchen, meistens lohnt es sich.

Marscel
2014-03-03, 20:34:22
Nope. Wenn die unter Umständen wirklich dauernd fliegen (und damit auch keine Ausnahmen mehr sind) ist das deutlich langsamer.

Ja, dauernd vs. performancekritisch. Wie Monger schon schrieb, Exceptions sollten Ausnahmen bleiben, obwohl eine generelle Trennung von Code- und Rescuepfaden recht nett wirkt.

Nichtsdestotrotz ist ein genereller Verzicht von Exceptions in C# Unsinn. Ein Parser kann meinetwegen eine Exception werfen, wenn er danach aufhört oder gar nicht erst anfangen kann. Aber als (internes?) Signalkonzept Exceptions: Ürgs.

medi
2014-03-03, 20:42:58
Ist das ernst gemeint? In einem anderen Unterforum hätte ich sowas als Trollversuch gewertet.


Ich hat das mal vor einiger Zeit an einem Beispiel getestet wo es zu vielen Exceptions kam und die Kosten waren enorm. Ich weiss nur nicht mehr so genau obs in PML oder C# oder gar beiden Sprachen war.


Exceptions sind nur dann etwas teurer wenn sie tatsächlich geworfen werden. Sie sind für Ausnahmefälle gedacht, nicht zur regulären Signalisierung.


Sorry aber das:
Nach meiner Meinung wäre die sauberste Lösung, dass Ganze in ein C++ File zu packen, und die SSLHashSHA1.<Methoden> dazu zu bewegen, dass sie Exceptions werfen
klang für mich nach "Es darauf anlegen, dass der Code in ne Exception rennt. Ich kenn das halt von dem einen oder anderen Kollegen, der so mit dem Vorschlaghammer programmiert ("Mach das jetzt und wenns nicht geht dann fang die Exception." und ich reg mich jedes mal drüber auf da es eben nicht nur genutzt wird um absolute Ausnahmefälle zu fangen. Sorry wenn das ein Missverständnis war.


Sie entwirren den Kontrollfluss aber mitunter drastisch, und sparen damit im Endeffekt Laufzeit für unnötige Überprüfungen.


Ansichtssache. Wenn die Überprüfung schnell erledigt ist muss ich nicht die Exception anstreben.


Status Codes als Rückgabewerte gelten dagegen mittlerweile als Antimuster. Die Aussagekraft die man aus ihnen im Fehlerfall gewinnt ist oft nahe Null, und es passiert schnell dass man deren Auswertung vergisst.
Das gilt für dein Codebeispiel im übrigen ja nochmal ganz besonders, weil a) du mit jeder Zuweisung die Information des vorherigen Methodenaufruf ignorierst und du b) den Rückgabewert schlicht still schluckst. Wenn entweder can_login oder Login oder has_access scheitern, stehst du anschließend genauso schlau da wie zuvor.
Ich wusste nicht, dass es hier darum ging die Abbruchbedingung auswerten zu können. Das macht der Originalcode ja schließlich auch nicht. ;) Kann mich aber auch irren, meine letzte C++ Berührung ist >10 Jahre her.

medi
2014-03-03, 20:45:28
Das haben die Performance-Leute von MS auf Channel9 auch vor 1-2 Jahren noch gesagt, ab 100 Exceptions pro Sekunde sollte man die Auswirkung der Exceptions auf die Laufzeit untersuchen, meistens lohnt es sich.

Ich hatte mal gelesen, dass nur die erste geworfene Exception richtig teuer ist. Jede weitere dieses Typs würde dann nur noch nen Bruchteil davon kosten. Hab diese Aussage aber nicht weiter geprüft.

Ectoplasma
2014-03-03, 21:38:18
Also ich hab mal gelernt, dass man Exceptions meiden sollte wie der Teufel das Weihwasser da die enorm kosten ... zumindest bei C#.

In C++ kosten die gar nichts. Bzw. es kommt darauf an, welchen Compiler du nimmst und ob er das Exception ABI vom OS unterstützt.

Edit 1.
Ah, vorherige Posts nicht gelesen.

Edit 2.
In Java beispielsweise gibt es nur extrem wenig Code, der mit einem Error Code als Return-Wert arbeitet. Dort haben Exceptions ebenfalls Zero Overhead.

Monger
2014-03-03, 22:50:38
Ich hat das mal vor einiger Zeit an einem Beispiel getestet wo es zu vielen Exceptions kam und die Kosten waren enorm. Ich weiss nur nicht mehr so genau obs in PML oder C# oder gar beiden Sprachen war.

Grundsätzlich: ja, Exceptions sind mittelmäßig teuer. Das instanziieren des passenden Objekts und z.B. das Auslesen des Stacktraces kosten etwas Zeit. Das ist spürbar teurer als wenn man nur mit Value Types wie int32 o.ä. arbeitet, aber im Vergleich zum instanziieren von anderen Objekten auch wieder nicht so schwergewichtig.
Man soll nicht tausende von Exceptions in kurzer Zeit werfen. Darum geht's aber auch nicht: Exceptions sollen den Code robuster und lesbarer machen. Außerdem behandelt ein vernünftiges Exception Handling nicht nur die Probleme die man erwartet, sondern auch die die man nicht erwartet. Du brauchst es normalerweise eh.


Ansichtssache. Wenn die Überprüfung schnell erledigt ist muss ich nicht die Exception anstreben.

Wenn du Funktionen wie "Login" hast, sind die nicht zeitkritisch. Du loggst dich ja nicht tausende Male pro Sekunde ein. Da geht Sicherheit vor.

Ectoplasma
2014-03-03, 23:00:34
Grundsätzlich: ja, Exceptions sind mittelmäßig teuer.

Aber nur, wenn eine geworfen wird, sonst kosten die gar nichts. Das Auslösen einer Exception sollte wohl nicht der Normalfall sein und daher ist es egal.

Edit:
Ein entwickler kann den größten Scheiß-Code bauen, niemand regt sich auf, aber bei sowas wie Excpetions werden alle gleich unruhig. Zum totlachen ist das manchmal.

Ganon
2014-03-03, 23:13:26
Beim Prüfen sollte das Programm abbrechen sobald der vorherige Wert false ergab. So spart man Laufzeit und der Code ist trotzdem leserlich

Nunja, solche Sachen halte ich nun nicht für sonderlich leserlich und stark fehleranfällig, da du ggf. massig Variablen im Scope hast.

medi
2014-03-04, 06:02:55
Grundsätzlich: ja, Exceptions sind mittelmäßig teuer. Das instanziieren des passenden Objekts und z.B. das Auslesen des Stacktraces kosten etwas Zeit. Das ist spürbar teurer als wenn man nur mit Value Types wie int32 o.ä. arbeitet, aber im Vergleich zum instanziieren von anderen Objekten auch wieder nicht so schwergewichtig.
Man soll nicht tausende von Exceptions in kurzer Zeit werfen. Darum geht's aber auch nicht: Exceptions sollen den Code robuster und lesbarer machen. Außerdem behandelt ein vernünftiges Exception Handling nicht nur die Probleme die man erwartet, sondern auch die die man nicht erwartet. Du brauchst es normalerweise eh.

Deswegen geht bei mir auch nichts ohne Exception Handling. Nur fokussiere ich es nicht wie hier im Thread angedeutet.

Nunja, solche Sachen halte ich nun nicht für sonderlich leserlich und stark fehleranfällig, da du ggf. massig Variablen im Scope hast.

Keine Ahnung wo du arbeitest aber wir sind 6 Entwickler im Team und haben mit diesem Programmierstil gute Erfahrungen gemacht was Code Wartung betrifft denn wir müssen oft alten Code überarbeiten und auf neue Anforderungen hin anpassen und selten ist der Entwickler, der den Code geschrieben hat auch der, der ihn überarbeitet. Und wo das fehleranfälliger sein soll erschließt mir auch nicht wirklich.

Naja, eigentlich auch egal. Die Vorschriften gibts bei uns, daran halten wir uns und unsere Arbeit hat sich dadurch sehr verbessert. Und ändern kann ich die Regeln eh nicht, da reisst mir mein Chef der Kopf ab :D

PHuV
2014-03-04, 11:02:24
Keine Ahnung wo du arbeitest aber wir sind 6 Entwickler im Team und haben mit diesem Programmierstil gute Erfahrungen gemacht was Code Wartung betrifft denn wir müssen oft alten Code überarbeiten und auf neue Anforderungen hin anpassen und selten ist der Entwickler, der den Code geschrieben hat auch der, der ihn überarbeitet. Und wo das fehleranfälliger sein soll erschließt mir auch nicht wirklich.

Naja, eigentlich auch egal. Die Vorschriften gibts bei uns, daran halten wir uns und unsere Arbeit hat sich dadurch sehr verbessert.
Sehe ich genau so, hab die gleiche Erfahrung gemacht. Gerade wenn man - wie Du es sagst - viel Altcode von anderen hat und im Team arbeitet, geht das nicht ohne Styleguides.

sei laut
2014-03-04, 17:07:14
Wo es hier gerade um "goto fail" Bugs geht, da hat Gnutls auch noch einen:
https://www.gitorious.org/gnutls/gnutls/commit/6aa26f78150ccbdf0aec1878a41c17c41d358a3b?diffmode=sidebyside

Der commit hatte den verifiy dazu bewogen, bei speziellen Zertifikaten die Prüfung zu umgehen.

Gast
2014-03-05, 11:59:59
Sehe ich genau so, hab die gleiche Erfahrung gemacht. Gerade wenn man - wie Du es sagst - viel Altcode von anderen hat und im Team arbeitet, geht das nicht ohne Styleguides.
Wollen wir wetten? Wir sind hier insgesamt ca. 3000 Programmierer....und es gibt nicht mal "echte" Styleguides pro Abteilung.
Klar, es gibt ein grobes Gesamtkonzept...aber wie das jede Abteilung umsetzt ist deren Ding.

Exceptions sollen bei uns nur dann geworfen werden wenn etwas so knallen würde das das Programm sowieso abstürzt bzw. die Daten/das Programm danach in einem inkonsitenten zustand wäre.
Anders gesagt: nur wenn irgendein Service nicht angezogen werden kann der auch zwingend notwendig ist.
Wird haben erst letztens quasi 90% der vorhandenen Exceptions aus dem Code rausgeschmissen.

Ectoplasma
2014-03-06, 07:46:20
Wird haben erst letztens quasi 90% der vorhandenen Exceptions aus dem Code rausgeschmissen.

Und warum?

Dr.Doom
2014-03-07, 09:32:15
Weil die in 90% der Anwendungsfälle nicht aufgetreten sind.

Grundkurs
2014-03-07, 12:34:55
Mal interessehalber zum Thema "fehlende Klammern".
Ich dachte immer bei einer Einzelanweisung (bei der man eben im if-Statement Klammern weglassen kann) gehört es eh zum guten Ton sie innerhalb der selben Zeile zu schreiben um solche Fehler wie im goto-fail zu vermeiden. Sprich:

if(Bedingung) Anweisung1;
Anweisung2;

statt:
if(Bedingung)
Anweisung1;
Anweisung2;

im ersten Beispiel ist deutlich, dass Anweisung1 bei Vorliegen der if-Bedinung ausgeführt wird und Anweisung2 in jedem Fall ausgeführt wird. Im zweiten Beispiel übersieht man dies viel schneller.

Thunderhit
2014-03-07, 17:11:49
Es gehört eher zum guten Ton, immer auch wenn es nur eine Anweisung ist, sie bei einem if in einen {} Block einzuschließen. Dass man die Anweisung direkt an das if anfügt mag ok sein, wenn man mehrere solche Anweisungen hintereinander hat, aber normal sollte das nicht sein.

sei laut
2014-03-07, 17:53:51
Sehe das wie Thunderhit, sauber formatiert und alles ist in Butter.

if (a=a) {
print "a ist doof";
if (b=b) {
print "b ist auch doof";
}
}
Anweisung 2;
Braucht mehr Zeilen, aber an Zeilen zu sparen um Fehler leichter zu übersehen ist irgendwie kontraproduktiv.
Manche machen die öffnende Klammer in die Zeile drunter, aber das muss ein Coding-Guide klären.

noid
2014-03-07, 21:04:57
Sehe das wie Thunderhit, sauber formatiert und alles ist in Butter.

if (a=a) {
print "a ist doof";
if (b=b) {
print "b ist auch doof";
}
}
Anweisung 2;
Braucht mehr Zeilen, aber an Zeilen zu sparen um Fehler leichter zu übersehen ist irgendwie kontraproduktiv.
Manche machen die öffnende Klammer in die Zeile drunter, aber das muss ein Coding-Guide klären.

coding style guidelines sind Käse, astyle ist besser.

Brillus
2014-03-08, 01:46:27
In Java beispielsweise gibt es nur extrem wenig Code, der mit einem Error Code als Return-Wert arbeitet. Dort haben Exceptions ebenfalls Zero Overhead.

DAS ist wohl die größte Legende die ich hier im Thread gelesen habe, erst vor kurzen hatten meine Studenten gemeint, in der Funktion wird ja sowieo der Wert überprüft brauchen wir keine Abfrage, fangen einfach nur die Exception wenn eine kommt, und kamen dann zu mir warum ihr Program unsagbar langslam läuft. Nachdem ich ihnen gesagt haben sie sollen vor dem Aufruf testen gab einen Speedup von einem Bild alle 10 Sekunden zu absolute flüssig. In java das werfen einer Exception (aka generieren des Stacktraces) is sowas von so brutal teuer das glaubst du nicht.


*Und es war nur ein Integertest ob er in einer Bestimmenten range ist und die funktion hat es ganz am anfang getest, also keine andere Unnötige arbeit.

Brillus
2014-03-08, 01:52:14
Mal interessehalber zum Thema "fehlende Klammern".
Ich dachte immer bei einer Einzelanweisung (bei der man eben im if-Statement Klammern weglassen kann) gehört es eh zum guten Ton sie innerhalb der selben Zeile zu schreiben um solche Fehler wie im goto-fail zu vermeiden. Sprich:

if(Bedingung) Anweisung1;
Anweisung2;

statt:
if(Bedingung)
Anweisung1;
Anweisung2;

im ersten Beispiel ist deutlich, dass Anweisung1 bei Vorliegen der if-Bedinung ausgeführt wird und Anweisung2 in jedem Fall ausgeführt wird. Im zweiten Beispiel übersieht man dies viel schneller.

Also das finde ich nur als unleserlich, weil man da gerne mal das if übersieht. Nein immer schön klammern setzen.

hell_bird
2014-03-08, 02:34:24
Bin ich eigentlich der einzige, der if Klauseln ohne geschweifte Klammern gar nicht schlecht findet? Ich glaub auch nicht, dass es den Fehler hier vermieden hätte wenn das nicht erlaubt wäre. Das hier ist ein dermaßen dämlicher copy&paste fehler dass der code sonst warscheinlich so ausgesehen hätte:


if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0) {
goto fail;
}
if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0) {
goto fail;
}
if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0) {
goto fail;
}
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) {
goto fail;
} {
goto fail;
}
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) {
goto fail;
}


Und das läuft wenn ich mich richtig erinnere auf genau das selbe Ergebnis hinaus.

noid
2014-03-08, 08:38:47
Bin ich eigentlich der einzige, der if Klauseln ohne geschweifte Klammern gar nicht schlecht findet? Ich glaub auch nicht, dass es den Fehler hier vermieden hätte wenn das nicht erlaubt wäre. Das hier ist ein dermaßen dämlicher copy&paste fehler dass der code sonst warscheinlich so ausgesehen hätte:


if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0) {
goto fail;
}
if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0) {
goto fail;
}
if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0) {
goto fail;
}
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) {
goto fail;
} {
goto fail;
}
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) {
goto fail;
}


Und das läuft wenn ich mich richtig erinnere auf genau das selbe Ergebnis hinaus.

Duplizierte Zeilen bei einem merge ja, zusätzliche Klammern wie bei dir? Nein.
Und eines weiss ich genau: die Kosten für ein Codereview mit commit ist billiger als die image und patch kosten später. Es ist eine Unsitte von einigen bei kritischem Code erst ein review zu machen, Punkte zu finden und dann kurz vor dem commit nach Änderungen/Aufräumen keinen kleinen review mit einer anderen Person zu machen.
Der Prozess ist hier falsch. Review und Tests wie üblich nicht richtig angewendet.

Ectoplasma
2014-03-08, 09:21:39
DAS ist wohl die größte Legende die ich hier im Thread gelesen habe ...

Ich weiss nicht, was ich zu deiner Erklärung sagen soll. Ich rede nicht vom Werfen einer Exception, wenn ich von Zero Overhead spreche. Und wie ich ja bereits an einer anderen Stelle sagte, sollte das Werfen einer solchen, eher die Ausnahme sein.

Bei einem Integer-Test, so wie du ihn beschreibst, ist der negative Testfall eine rein fachliche Angelegenheit. Da würde ich auch keine Exception werfen.

sei laut
2014-03-08, 13:19:59
if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0) {
goto fail;
}
if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0) {
goto fail;
}
if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0) {
goto fail;
}
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) {
goto fail;
goto fail;
}
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) {
goto fail;
}


Und das läuft wenn ich mich richtig erinnere auf genau das selbe Ergebnis hinaus.
Was soll denn der Quatsch? Ich hab deinen Quellcode mal korrigiert. So sähe der Code vorne mit Klammern aus. Eben klassisch duplizierte Zeile, was du mit den Klammern gemacht hast, sieht sehr realtitätsfern aus.
Vielleicht dupliziert sich die Zeile auch unter die schließende Klammer, trotzdem sieht man es leichter.

hell_bird
2014-03-12, 06:03:43
Was soll denn der Quatsch? Ich hab deinen Quellcode mal korrigiert. So sähe der Code vorne mit Klammern aus. Eben klassisch duplizierte Zeile, was du mit den Klammern gemacht hast, sieht sehr realtitätsfern aus.
Vielleicht dupliziert sich die Zeile auch unter die schließende Klammer, trotzdem sieht man es leichter.
Stimmt doch gar nicht! Die einfachste Art den Code zu vervielfältigen ist das ganze mit den Klammern zu kopieren. Offensichtlich hat der Originalcodehersteller ja auch alles inklusive Zeilenumbruch in die Zwischenablage genommen.