Der Build ist zwar schon etwas älter, aber eben nun der Vollständigkeit halber:

Von 7385 DLL und Exe-Dateien in diesem Build linken 428 gegen die .NET-runtime. Das sind 5,80% und damit 0,06 Prozent mehr als im Build 5308.

Irgendwie sind wir, so scheint's, immer noch nicht im 21. Jahrhundert angekommen. Oder wie erkläre ich mir sonst, daß es immer noch namhafte Software gibt, die nicht mit least Privileges läuft?

Heute habe ich mir Visual Assist installiert. Brav also als lokaler Administrator eingeloggt, das Setup ausgeführt, den Key eingegeben, gefreut. Dann ausgeloggt und als unterprivilegerter User gestartet, msdev abgefeuert - keine Spur von Visual Assist! Prominente Registry Keys in HKCU aus dem Hive des Administratoren in den Userhive gemergt: Aha, Visual Assist zuckt und fragt wieder nach dem Key, aber nach der Keyeingabe fehlt wieder jede Spur von Visual Assist. Schade, daß ein ansonsten so gutes Produkt an dieser Stelle so erbärmlich patzt. Das wäre ja, als ob επτ€σ seine Logfiles ins Program Files Directories schriebe... ähem, OK, hüstel, ...lassen wir das.

Zu guter Letzt die FAQ des Herstellers gesucht und schnell fündig geworden: Irgendwie stellen die Knilche das beinahe auch noch als Feature dar, daß man Visual Assist auch als Nichtadministrator betreiben kann (wenn man die NTFS-Rechte für das Installationsverzeichnis aufbohrt und VA für jeden User dahin oder woandershin neu installiert). So schnell werden aus Tätern Opfer...

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

So jetzt die Antwort. Mein tendenziöses Source-Code-Quoting ließ es dem DEI ja schon wie Schuppen von den Haaren fallen. Hier noch mal der Stein des Anstoßes:

BOOL NetState_ThreadControl( DWORD dwCtrlCode, 
                              PNETSTATE& pNetState )
{
  /// ...
  switch ( dwCtrlCode )
  {
    case  NETSTATE_CTRL_STARTPOLLING:
      pNetState->hPollingThread = _beginthreadex( NULL, 0, 
                                    NetState_PollingThread, ....
      break;
    case NETSTATE_CTRL_SUSPENDPOLLING:
      SuspendThread( pNetState->hPollingThread );
      break;
    case NETSTATE_CTRL_STOPPOLLING:
      TerminateThread( pNetState->hPollingThread, 0);
      CloseHandle( pNetState->hPollingThread );
      pNetState->hPollingThread = NULL;
      break;
  }
  
  return TRUE;
}

Halten wir mal folgende Fakten fest:

• Der Code verwendet die C-tuntime von Microsoft, das erkennt man an der Verwendung von _beginthreadex.
• Derselbe Thread der dergestalt erzeugt wird, wird entweder suspendet oder terminiert in den
  NETSTATE_CTRL_SUSPENDPOLLING oder NETSTATE_CTRL_STOPPOLLING case-branches.

Und damit wäre das Kardinalproblem schon genannt: Einen Thread der mit Methoden der C-runtime erzeugt wurde, darf man niemals mit TerminateThread terminieren, weil dadurch die per-Thread-Datenstrukturen der Runtime nicht freigegeben werden können. Auch Datenstrukturen des Threads, die andere DLls für ihn in DllMain anlegen, werden nicht mehr abgebaut, sein Stack bleibt bestehen (also in der Regel 1MByte im weiteren Leben des Prozesses nicht mehr nutzbarer virtueller Adressraum). Natürlich werden auch die Destruktoren der Objekte auf dem Stack nicht aufgerufen, wär ja noch schöner! Schlimmer noch als diese Speicherlecks, die man beim Shutdown eines Prozesses vielleicht noch verkraften könnte, wiegt aber der Umstand, dass sowohl TerminateThread als auch SuspendThread einen kompletten Prozess mit Leichtigkeit in einen Deadlock bringen können. Denn wenn der zu suspendierende oder terminierende Thread gerade der Owner einer Critical Section ist, wenn auf den Thread diese Aktion ausgelöst wird, dann ist diese Critical Section für andere Threads verloren. Das kann passieren, wenn der fragliche Thread gerade mitten in einer Speicherallokation mit new oder malloc ist und er dann durch diese APIs nicht mehr den lock des Memory Manager freigeben kann. Will nun ein anderer Thread Speicher allozieren, wartet er darauf bis zum Sanktnimmerleinstag. Die Threads, und womöglich der ganze Prozeß, sind dann im Deadlock.

Wann kann man nun die beiden APIs legal aufrufen? Eigentlich nur wenn man gegen keine C-runtime linkt, wenn man in den Threads keine Aufrufe ausser in kernel32.dll und kein LoadLibrary/LoadLibraryEx macht und wenn man keinen State auf seinem Thread hat (Can you say destructors? Can you say __try-__finally?). Warum die Beschränkung auf kernel32.dll? Weil MS seit W2K System-DLLs ausliefert, die per Delayload andere DLLs nachladen. Das bedeutet, daß harmlose API-Aufrufe zum Nachladen einer DLL führen können. Dann hat der fragliche Thread den Loader-Lock, wenn ein anderer ihn nun suspendet oder terminiert. Das hat wiederum zur Folge, daß kein anderer Thread mehr eine DLL laden kann, bzw. ein derartiger Versuch eben deadlockt. Natürlich kann ein anderer Thread auch kein derartiges API mehr aufrufen, das per Delayload andere DLLs nachlädt.

Und ja, warum der zweite formale Parameter ausgerechnet eine PNETSTATE-Referenz sein muß, ist mir auch nicht wirklich klar...

Auch warum der TeminateThread dem Thread einen im allgemeinen als legal angesehenen Wert wie 0 unterschiebt, damit ein anderer ja nicht erkennt, daß der Thread eines gewaltsamen Todes gestorben ist, entzieht sich meinem an Defekten so reichen Intellekt dann vollends. Wie wär's mit 0xdeadbeef?

So, heute war's mir langweilig und ich habe mal den unlängst geschriebenen Code von anderen Leuten in meinem Dunstkreis inspiziert. Denn man gibt sich bei επτ€σ zwar gerne weltmännisch offen und trägt die eine oder andere Tugend wie eine Fahne vor sich her, aber die Realität sieht dann in aller Regel a bissal anders aus. Wie früher beim Unternehmensmotto ("Where people make..."), so heute auch bei Code Reviews. Und deswegen muß die Lücke eben am frühen Samstagabend notdürftig geschlossen werden. Denn nach nicht mal 5 Minuten bin ich über ein ziemlich grauenvolles Artefakt gestossen:

BOOL NetState_ThreadControl( DWORD dwCtrlCode, 
                              PNETSTATE& pNetState )
{
  /// ...
  switch ( dwCtrlCode )
  {
    case  NETSTATE_CTRL_STARTPOLLING:
      pNetState->hPollingThread = _beginthreadex( NULL, 0, 
                                    NetState_PollingThread, ....
      break;
    case NETSTATE_CTRL_SUSPENDPOLLING:
      SuspendThread( pNetState->hPollingThread );
      break;
    case NETSTATE_CTRL_STOPPOLLING:
      TerminateThread( pNetState->hPollingThread, 0);
      CloseHandle( pNetState->hPollingThread );
      pNetState->hPollingThread = NULL;
      break;
  }
  
  return TRUE;
}

Wo ist hier das große Problem, das den Einsatz dieses Stückchen Code zum Glücksspiel werden läßt? Kleiner Hinweis: Es geht nicht um fehlende Prüfungen von Rückgabewerten, error propagation im Allgemeinen. Los DEI, KIH, Schmitti und OTE und wer noch, was stinkt hier unübersehbar zum Himmel?

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.