Link: http://mcblogs.craalse.de/sku?title=title&more=1&c=1&tb=1&pb=1

Vor einer Woche habe ich ein Stück Code vorgestellt mit der Frage was daran falsch ist. Entweder hat sich niemand getraut dieses Stück Code eines Fehlers zu bezichtigen, oder es war zu einfach. Wie auch immer, das hier war nochmal der Stein des Anstoßes:

BOOL FileCreateEmptyFile( LPCTSTR lpszFilename )
{
  HANDLE hEmptyFile = 
    CreateFile(
      lpszFilename,
      GENERIC_WRITE,
      FILE_SHARE_READ | FILE_SHARE_WRITE,
      NULL,
      CREATE_ALWAYS,
      FILE_ATTRIBUTE_HIDDEN,
      NULL
    );

  ASSERT( hEmptyFile );
  CloseHandle( hEmptyFile );
  return (hEmptyFile != 0);
}

Folgendes habe ich an dem Code auszusetzen:

1. Der Code testet sowohl in dem ASSERT als auch bei der Rückgabe des Boolean auf NULL als Rückgabewert. Dummerweise liefert CreateFile aber nicht NULL im Fehlerfall, sondern INVALID_HANDLE_VALUE, also (DWORD_PTR)-1, also einen Wert ungleich NULL. Das bedeutet, daß es mit dieser Funktion (FileCreateEmptyFile) unmöglich ist, einen Fehler zu erkennen, sie klappt scheinbar einfach immer und der ASSERT triggert auch quasi nie. Nur in dem höchst unwahrscheinlichen Fall, daß sie als legitimen Wert im Erfolgsfall NULL zurückliefert, meldet diese Funktion auch noch irrtümlicherweise einen Fehler zurück.

2. Die Funktion ruft einen ASSERT auf. Das legt nahe, daß ASSERTs naher Verwandter VERIFY auch irgendwie nicht weit sein kann. Warum also in drei Teufels Namen hat der Autor nicht den Aufruf von CloseHandle(hEmptyFile); durch VERIFY(CloseHandle(hEmptyFile)); ersetzt? Man mag sich nun fragen, ob das nicht etwas übertrieben ist, wenn wir korrekt prüfen würden ob hEmptyFile ungleich INVALID_HANDLE_VALUE ist. Meine Meinung ist, dass ein VERIFY(CloseHandle(hEmptyFile)); nicht nur nicht schadet, sondern auch ganz üble Bugs in Multithreaded Applikationen nachweisen und aufspüren helfen kann. Betrachten wir dazu folgendes Szenario:

         Thread 1                 |   Thread 2
|                                 |
t1       h1 = CreateEvent(...)    |
|                                 |
t2       CloseHandle(h1)          |
|                                 |
t3                                |   h2 = CreateFile(...
|                                 |
t4       CloseHandle(h1)          |
|                                 |
t5                                |   CloseHandle(h2)
|                                 |
|                                 |
| Zeitachse                 
V

Die Zeitachse gehe hier in vertikaler Richtung von oben nach unten und links ist Thread 1 dargestellt, der einen Bug eingebaut hat. Er erzeugt nämlich einen Event zum Zeitpunkt t1 und schließt ihn zum Zeitpunkt t2 wieder. Er schließt ihn aber aufgrund eines Programmierfehlers (sowas kommt in den besten Familien vor) ein zweites Mal. Wenn nun nach dem ersten Schließen dieses Events Thread 2 ein beliebiges anderes Kernelobjekt erzeugt, im obigen Beispiel etwa eine Datei mit CreateFile und wenn diese zufällig denselben Wert erhält wie das just zuvor in Thread 1 geschlossene Handle auf den Ex-Event, dann schließt Thread 1 ein Handle, das in Thread 2 verwendet wird. So etwas führt zu ganz ganz komischen Fehlern und außerdem zu einem unschuldigen Verdächtigten: Es ist Thread 2, der sich ab sofort überhaupt nicht wie erwartet benimmt und ganz komische Resultate liefert. Kein Wunder, schließlich operiert Thread 2 ja auch mit einem Ex-File-Handle herum. Hat man nun einen VERIFY um den CloseHandle in Thread 2, kann man hieb- und stichfest nachweisen, daß hier etwas nicht stimmt. Und wenn aus der Programmlogik hervorgeht, daß Thread 2 dieses Handle eigentlich gar nicht schließt vor dem Zeitpunkt t5, bedeutet das, daß ein anderer Thread marodiert und das Handle aus Thread 2 sabotiert. Ohne einen VERIFY um das CloseHandle in Thread 2 ist so ein Fehler verdammt schwierig zu finden. Darum gibt es bei mir keinen Aufruf vonn CloseHandle ohne einen VERIFY drumrum. Das hat mir schon einige Male den Tag gerettet und mich vor einen schlaflosen Nacht bewahrt. Been there, done that.

3. Selbst im Happy-Day-Szenario, wenn alles in dem Code gutgeht, muss man sich fragen: Was passiert dann? Ich meine: Die Funktion soll eine leere Datei anlegen, aber wer hindert denn irgendeinen anderen Prozess oder Thread daran, etwas in die Datei reinzuschreiben? Can you say "Race Condition"? Schlimmer noch: Da die Datei mit FILE_SHARE_WRITE erzeugt wird, kann es sein daß der aufrufende Thread nach dem Erzeugen der Datei, aber noch vor dem Schließen des Handles preempted wird und jemand anderes etwas in die Datei reinschreibt. Wenn nun nachfolgender Code davon ausgehen muß, daß diese Datei wirklich eine Größe von Null Bytes hat, ist er der Gelackmeierte. Das einzige was dieses Problem lösen kann, ist, die Datei ohne Sharing zu erzeugen und das Handle so lange offen zu halten, wie man garantieren muß, daß die Datei wirklich leer ist. Aber wozu braucht man in diesem Fall dann überhaupt noch eine separate Funktion, schließlich hat man dann einen Wrapper um CreateFile der nichts tut außer CreateFile aufzurufen....
Man kann sich jetzt natürlich auf den Standpunkt stellen: "Unsere Applikation tut das nicht, sie schreibt erst dann was in die Datei, wenn wir wissen daß sie vorher erzeugt wurde und außerdem passiert das nicht in nebenläufigen Threads sondern im selben Thread hinterher." Tja, das ist dann eben halt auch die Einstellung, mit der Sicherheitslücken in sicherheitsrelevanten Applikationen entstehen. Ist also die obige Funktion in irgendeiner Form Common Code, der auch in kritischerem Code verwendet wird, der sich 100%ig darauf verlassen muß, daß die Datei wirklich leer ist, hat man hier ein Problem. Das Beispiel mag übertrieben erscheinen, aber ich sage mir: Wer sowas im Kleinen, bei unkritischen Problemen schon nicht hinkriegt, wird im Großen, bei den kritischen und sicherheitsrelevanten Problemen erst recht versagen. Und wer den potentiellen Angriffsvektor im Kleinen nicht erkennt, wird auch bei den sicherheitsrelevanten Dingen nicht wie ein Angreifer denken.

4. Wenn die Funktion eigentlich scheitern sollte (also CreateFile INVALID_HANDLE_VALUE zurückliefert), dann wird der last error von CreateFile durch den Aufruf des ebenfalls zum Scheitern verurteilten CloseHandle zerstört. Alles was als last error zum Aufrufer zurückkommt ist ein last error von ERROR_INVALID_HANDLE anstatt beispielweise einem ERROR_ACCESS_DENIED, wie er von CreateFile erzeugt worden sein könnte. Der Aufrufer hat, selbst wenn er den Verdacht hat, daß die Funktion trotz gegenteiliger Behauptungen gescheitert ist, nicht die Chance einer Möglichkeit, herauszubekommen, warum das der Fall war.

Alles in allem sind also nicht nur 4 kapitale Fehler in dieser harmlos ausschauenden Funktion von gerade 'mal 4 Zeilen versteckt, sie ist auch noch komplett sinnlos, weil eine korrekte Implementation zu einem Aufruf von CreateFile selbst degeneriert. Sinnlose Wrapper um APIs, die den Aufruf lediglich um zwei drei Parameter vereinfachen, die ansonsten NULL oder 0 wären, sehe ich ziemlich häufig, und sowas dann als Verinfachung oder gar als Common Code verkaufen zu wollen, erachte ich in den meisten Fällen als äußerst grenzwertig. Das ist dann sinnvolle Energie für's falsche Ziel vergeudet.