Letzte Woche habe ich in einer Email aufgefordert, die Fehler in folgendem Codeabschnitt zu finden:

CString GetComputerName ()
{
  static CString szComputerName;
  if (szComputerName.IsEmpty())
  {
    DWORD dwDataSize = MAX_PATH;

    ::GetComputerName( szComputerName.GetBuffer( MAX_PATH ), 
      &dwDataSize );
    szComputerName.ReleaseBuffer();
  }
  return szComputerName;
}

Der absolute Gewinner war Olli, denn er hat kurz und knapp die drei wichtigsten Probleme gefunden:

"- Anstatt MAX_PATH muss es "MAX_COMPUTERNAME_LENGTH + 1" heissen
- GetBuffer kann eine Exception werfen.
- Nicht threadsafe"

Warum ist das Ganze nicht threadsafe? Weil ein nichttrivialer Typ als globale Variable in szComputerName verwendet wird. Vom Crash bis zum Speicherleck kann alles mögliche passieren wenn zwei threads dumm preempted werden, wenn sie parallel durch diesen Code laufen. Und tatsächlich kann CString::GetBuffer eine CMemoryException werfen. Ob wohl jeder Aufrufer auf sowas vorbereitet ist? Besser wäre es, die Exception in der Funktion aufzufangen und den Fehler mit einem return value anzuzeigen oder mit Setzen eines last error für den aufrufenden Thread, so daß man die Funktion nach "Fire and Forget"-Manier aufrufen kann und sich um Exceptions nicht kümmern muß. Und last but not least ist die Größe des Puffers mit 260 Zeichen für MAX_PATH auch etwas übertrieben, gemessen an den schlappen 15 für MAX_COMPUTERNAME_LENGTH. Was Olli aber nicht gefunden hat, sind vier weitere Probleme:

• <Korinthenkacker>Wenn schon ungarische Notation (App's Hungarian, um genau zu sein), dann richtig. Es darf dann nicht szComputerName heißen, sondern muß strComputerName heißen.</Korinthenkacker>
• Selbst wenn die Funktion intern alle Exceptions abfängt, so kann immer noch eine Exception auftreten, wenn der Rückgabewert der Funktion einer CString-Variablen im Scope des Aufrufers zugewiesen wird. Der Aufrufer kommt also in keinem Fall um einen try...catch herum, es sei denn man gibt den Computer-Namen nicht als Rückgabewert zurück sondern hat eine Referenz auf einen CString als formalen [out]-Parameter. Wow, und wenn man dann das tut, hat man plötzlich wieder unverhofft einen Rückgabewert frei, wo man FALSE im Fehlerfall zurückgeben kann. Und weil dann auch kein lokales Objekt mehr da ist, ist sogar ein SetLastError im Fehlerfall bombensicher.
• Anstatt dem GetBuffer die Konstante MAX_PATH zu übergeben, hätte man durchaus die Variable dwDataSize übergeben können, dann hätte man eine einzige Stelle im Code, wo man den MAX_PATH auf (MAX_COMPUTERNAME_LENGTH+1) hätte abändern müssen.
• Statt CString::GetBuffer wäre es angebrachter, CString::GetBufferSetLength einzusetzen, da diese Methode den String garantiert erstmal nullterminiert. CString::GetBufferSetLength einzusetzen ist eine gute Angewohnheit, da diese Methode auch in denjenigen Fällen den String nullterminiert, wo die Funktion, die den String dann beschreibt, dies nicht tut.

Ich persönlich finde die fehlende Exception-Awareness nicht so suupertragisch, weil MFC-Code ist normalerweise UI code und was macht es schon, wenn solcher Code dann mal die Grätsche macht? Viel tragischer fände ich es, wenn solcher Code in Services eingesetzt wird, wo eigentlich 24/7 gefragt sein sollte. Das Bestürzende ist allerdings meiner Meinung nach, daß dieser Code in jedem Thread aufgerufen wird, der Logfiles schreibt und zwar ziemlich genau am Anfang, wenn die Logfile-Präambel geschrieben wird. Wenn zwei Threads also ziemlich zeitgleich starten, ist also die Gefahr einer Race-Condition in dieser Funktion durchaus gegeben.