Fragen? Antworten! Siehe auch: Alternativlos
mtu -= hlen + sizeof(struct frag_hdr);Und hier ist, was der neue Code tut:
if (overflow_usub(mtu, hlen + sizeof(struct frag_hdr), &mtu) || mtu <= 7)Als erstes fällt auf, dass der Code nicht das selbe tut. Nehmen wir mal an, dass overflow_usub zwei unsigned ints subtrahiert und dann meckert, wenn das Ergebnis negativ würde. Dann fängt dieser Code nicht nur den Overflow ab, sondern er prüft auch, dass mtu danach mindestens 8 ist. Allerdings prüft der Code nicht, ob hlen + sizeof(struct frag_hdr) bereits überläuft. Und das ist an sich schon ein Problem. Komme ich gleich drauf.
goto fail_toobig;
Linus' Kritik ist im Wesentlichen: Ist schlecht lesbar, ist ineffizient und immer noch unsicher.
Ich glaube, dass er bei allen drei Kritikpunkten irrt. Hier ist Linus' Gegenvorschlag, wie der Code lesbarer aussehen könnte:
if (mtu < hlen + sizeof(struct frag_hdr) + 8)Ich finde das lesbar. Aber ich mache das auch beruflich, in Code nach Integer Overflows zu checken. Viele Programmierer werden sich das angucken und gar nicht verstehen, was hier das Problem ist, das da geprüft wird. Wenn das Label nicht "fail_toobig" hieße, wäre gar nicht klar, was hier eigentlich geprüft werden soll. Linus' Version ist daher meiner Meinung nach nicht lesbarer. Und sie hat eine gewisse Fragilität. Wenn hlen ein size_t ist und beliebig groß sein kann, schlägt der Test fehl. Wird hier nicht so sein, so gut wie niemand hat 64-bit Headerlängen in der Praxis in seinen Protokollen. Aber es gibt Ausnahmen. Gut, nehmen wir mal an, hlen ist ein unsigned short (oder wir sind auf einer 64-bit-Plattform und hlen ist ein unsigned int). Wenn der Compiler das evaluiert, gibt es keinen Overflow, wenn in der Reihenfolge, in der der Compiler das evaluiert, zuerst eine Addition mit dem sizeof-Wert gemacht wird. Dann promoted C den Typ des Ergebnisses zu size_t, das ist an der Stelle groß genug und hat keine Overflow-Probleme. Aber was wenn ein weniger fitter Programmierer so einen Test zu bauen versucht und die Reihenfolge verkackt?
goto fail_toobig;
mtu -= hlen + sizeof(struct frag_hdr);
if (mtu < hlen + 8 + sizeof(struct frag_hdr))Wenn das sorum dasteht, und hlen groß genug reinkommt, und der Compiler hlen+8 zuerst evaluiert, dann kann der Teil überlaufen und wir addieren auf das abgeschnittene Ergebnis das sizeof drauf und das ist dann size_t, aber hatte schon einen Überlauf. Meines Wissens gibt es keinen Compiler, der Linus' Ausdruck böswillig umsortieren würde.
Die Compiler-Builtins, die hinter diesem Makro stehen, sind hier beschrieben. gcc hat das ursprünglich von clang geklaut, aber hat es an einer wichtigen Stelle besser und damit überhaupt erst ordentlich benutzbar gemacht. Bei clang musste man anfänglich je nach Datentyp ein anderes Builtin aufrufen, und das ist natürlich auch voll für den Arsch, denn diese Subtilitäten sind ja gerade die Fehlerquelle, die wir durch sowas ausschließen wollen. Heute gibt es auch bei clang __builtin_add_overflow, und das sieht in Beispielcode so aus:
#include <stdlib.h>Der Compiler erzeugt für diese Funktion diesen Code:int s(int a,int b) {
int c;
if (__builtin_add_overflow(a,b,&c))
exit(1);
return c;
}
s:Aus meiner Sicht ist dieser Code an dieser Stelle optimal. Ich wüsste jedenfalls nicht, wie man das besser machen könnte. Das "rep ret" ist übrigens ein Fix für einen Bug in einem der ersten 64-bit Athlons damals. Wusstet ihr nicht? Ja, so ist das, die Compiler sind ziemlich smart heutzutage. Smarter als die meisten Programmierer :-)
movl %edi, %eax
addl %esi, %eax
jo .L9
rep ret
.L9:
[… Error handling …]
Was ich damit sagen will: Nein, Linus, die Builtins generieren keinen schlechten Code. Wenn der Code an der Stelle mehr tut als einen Conditional Jump einfügen, der im Normalfall nicht genommen wird (und damit fast kostenlos ist), dann war der Compiler möglicherweise smarter als du an der Stelle und hat erkannt, dass die beiden reinkommenden Typen nicht gleich breit waren und einen zusätzlichen Check eingefügt, ob ein Zwischenergebnis abgeschnitten wird.
So. Also nochmal zu den drei Argumenten. Unlesbar bestreite ich, weil bei dem Makro explizit und offensichtlich ist, was der Programmierer erreichen will. Das ist immer besser als eine Packung Ultrasmart-Bitpfriemel-Kram, bei dem der Auditor zehn Minuten braucht, bis er versteht, was hier der Trick und das gewünschte Ergebnis ist, und ob der Weg dahin überhaupt korrekt ist.
Das zweite Argument war Ineffizienz. Das habe ich gerade widerlegt.
Das dritte Argument war Unsicherheit. Nun, Linus, das mag jetzt schmerzen, aber der Code ist sicherer als der vorgeschlagene Ersatzcode :-)
Daher, liebe Leser: Wenn ihr C programmiert, und euch fragt, ob ihr lieber die Builtins verwenden sollt oder auch alte Compiler unterstützten, dann NEHMT DIE BUILTINS. Ich bin sogar dafür, gar keinen Fallback-Pfad für alte gcc-Versionen anzubieten. Wer neuen Code mit alten Compilern übersetzt, hat verdient, dass das kracht. Diese ganze Debian-Antiquitätenhändler-Fraktion braucht mal ein paar Warnschüsse vor den Bug. Das ist ja schlimmer als Windows XP!
Wenn ihr überlegt, ob ihr lieber selber Overflow-Checks hackt oder lieber die Builtins nehmen sollt, NEHMT DIE BUILTINS.
Wenn ihr überlegt, ob ihr Overflow-Checks braucht oder nicht, dann NEHMT DIE BUILTINS. Die Checks kosten so gut wie nichts. Nein, wirklich. Da muss man sich anstrengen, um das überhaupt mit geschickten Mikrobenchmarks messen zu können. Und wenn der Check an der Stelle überflüssig wäre, und gcc das erkennen kann, dann fliegen auch die eh schon fast kostenlosen Checks noch raus.
Es gibt keine Ausreden. Nehmt die Builtins. Jedes einzelne Mal. Insbesondere, wenn es um Multiplikation geht. Das verkacken so viele Programmierer da draußen, dass ich das naheliegende Trinkspiel aufgeben musste. Probiert es gar nicht erst. Nehmt die Builtins. Und ich empfehle euch das, obwohl mein Lebensunterhalt davon abhängt, dass es Leute gibt, die nicht die Builtins benutzen.
Update: Wie sich rausstellt, sagt der C-Standard doch was zur Assoziativität von arithmetischen Operatoren. Der Compiler darf sich also nicht aussuchen, in welcher Reihenfolge er was addiert.