...someplace, where there isn't any trouble? Do you suppose there is such a place, Toto?

What's wrong with this code? Antwort

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?

Trackback address for this post

This is a captcha-picture. It is used to prevent mass-access by robots.
Please enter the characters from the image above. (case insensitive)

No feedback yet

Comments are closed for this post.