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.

Link: http://www.heise.de/newsticker/meldung/71604

Hhm, zuerst der Delay von Vista selber, nun gilt auch noch die neue Schrifttype "Segoe UI" auch ganz offiziell als nicht so ganz koscher. Mal sehen was nun aus der vollmundigen Ankündigung wird, "Segoe UI" werde es als redistributable Download für ISVs geben, die natürlich jetzt schon anfangen sollen, ihr UI und ihre Applikationen auf "Segoe UI" hinzutrimmen. Vielleicht sollte man dann doch einfach erstmal besser bei "MS Shell Dlg" bleiben bis klar ist ob Segoe UI wirklich einfach so mit Vista mitkommen darf...

Über dieses Stückchen Code mußte ich gestern mit einem Kollegen drüberschauen und ich konnte meinen Augen kaum trauen:

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);
}

Was ist hier alles falsch? Oder anders formuliert: Ist hier eigentlich überhaupt irgendwas richtig an diesem Stück Code?

Link: http://geekswithblogs.net/dirksblog/archive/2006/03/05/71471.aspx

In unserer andauernden Diskussion, wie man selbst mit P/Invoke .NET-Assemblies auf einem x64-Rechner so starten kann, dass sie als x64-Prozesse laufen, war mein Vorschlag, forwarder DLLs zu verwenden, die auf die plattformspezifischen DLLs verweisen, die native für die jeweilige Architektur gebuildet wurden. Das .NET-Assembly wird also nur eine Datei foofwd.dll laden, die ihrerseits je nach Plattform gegen eine foo.dll für x86 oder eine foox64.dll für x64 linkt. Der einzige Knackpunkt (und Dirkies Hauptkritikpunkt) ist dabei, daß man die x64-Variante der foofwd.dll ins Verzeichnis %systemroot%\system32 kopieren muß. Eine Variante, wie man dies umgehen kann und alle DLLs im Installationsverzeichnis halten kann, will ich im Folgenden zeigen:

Ich gehe im Folgenden davon aus, dass unser Installatioonsverzeichnis unter

c:\program files\ourapp

liegt. Ich gehe weiterhin davon aus, daß alle unsere Programme auch dort liegen, inklusive der native DLLs foo.dll und foox64.dll. Für alle Forwarder DLLs legen wir aber nun ein Unterverzeichnis forward.net an und darunter die Verzeichnisse x86 und x64 in welche wir nun alle unsere forwarder DLLs reinkopieren:

c:\-
   |
   -program files-
                 |
                 -ourapp-
                        |
                        -foo.dll (native x86)
                        |
                        -foox64.dll (native x64)  
                        |
                        -forward.net-
                                    |
                                    -x86-
                                    |   |
                                    |   -foofwd.dll (native x86)
                                    |
                                    -x64-
                                        |
                                        -foofwd.dll (native x64)  

Das einzige Problem besteht nun darin, daß ein .NET-basiertes Programm zur Laufzeit ermitteln kann, ob es als 32-bittiger oder als 64-bittiger Prozeß läuft. Und es muß darauf basierend dann in der Lage sein, die forwarder DLLs aus dem richtigen Verzeichnis zu laden. Sehr einfach geht, das, indem der Prozeß gleich zu Beginn seine an ihn vererbte Environmentvariable %PATH% entsprechend ergänzt, dann geht das alles kinderleicht. Folgendes Beispiel zeigt wie's gehen kann:

using System;
using System.Collections.Generic;
using System.Text;
using System.Runtime.InteropServices;
using System.Reflection;

namespace DoIlooklikeIgivaShit
{
  enum PROCESSOR_ARCH
  {
    PROCESSOR_ARCHITECTURE_INTEL = 0,
    PROCESSOR_ARCHITECTURE_MIPS = 1,
    PROCESSOR_ARCHITECTURE_ALPHA = 2,
    PROCESSOR_ARCHITECTURE_PPC = 3,
    PROCESSOR_ARCHITECTURE_SHX = 4,
    PROCESSOR_ARCHITECTURE_ARM = 5,
    PROCESSOR_ARCHITECTURE_IA64 = 6,
    PROCESSOR_ARCHITECTURE_ALPHA64 = 7,
    PROCESSOR_ARCHITECTURE_MSIL = 8,
    PROCESSOR_ARCHITECTURE_AMD64 = 9,
    PROCESSOR_ARCHITECTURE_IA32_ON_WIN64 = 10,
    PROCESSOR_ARCHITECTURE_UNKNOWN = 0xFFFF
  }


  [StructLayout(LayoutKind.Sequential)]
  class SYSTEM_INFO
  {
      public ushort wProcessorArchitecture;
      public ushort wReserved;
      public uint dwPageSize;
      public IntPtr lpMinimumApplicationAddress;
      public IntPtr lpMaximumApplicationAddress;
      public IntPtr dwActiveProcessorMask;
      public uint dwNumberOfProcessors;
      public uint dwProcessorType;
      public uint dwAllocationGranularity;
      public ushort wProcessorLevel;
      public ushort wProcessorRevision;
  }


  class Program
  {
    [DllImport("kernel32.dll", CharSet = CharSet.Unicode,
           CallingConvention = CallingConvention.Winapi,
           EntryPoint = "GetSystemInfo")]
    private static extern void GetSystemInfo([In, 
                MarshalAs(UnmanagedType.LPStruct)]
		SYSTEM_INFO pSysInfo);

    public static ushort GetProcessorArchitecture()
    {
        SYSTEM_INFO si = new SYSTEM_INFO();
        GetSystemInfo(si);
        return si.wProcessorArchitecture;
    }    

    static void Main(string[] args)
    {
      string strPath = Environment.GetEnvironmentVariable("PATH");
      Assembly ass = Assembly.GetExecutingAssembly();
      string str = ass.Location;
      int iLastBSlash = str.LastIndexOf('\\');
      ushort uPlatform = (ushort)GetProcessorArchitecture();
      if (-1 != iLastBSlash && 
        (
        (ushort)PROCESSOR_ARCH.PROCESSOR_ARCHITECTURE_INTEL==
                uPlatform
        ||
        (ushort)PROCESSOR_ARCH.PROCESSOR_ARCHITECTURE_AMD64==
                uPlatform
        ))
      {
        str = str.Substring(0, iLastBSlash) + "\\\\forward.net\\\\";
        switch (uPlatform)
        {
          case (ushort)
               PROCESSOR_ARCH.PROCESSOR_ARCHITECTURE_INTEL:
            str += "x86";
            break;
          case (ushort)
               PROCESSOR_ARCH.PROCESSOR_ARCHITECTURE_AMD64:
            str += "x64";
            break;
        }
        strPath += ";" + str;
       
        Environment.SetEnvironmentVariable("PATH", strPath);
      }

      /// 
      /// und ab jetzt machen wir Aufrufe mit P/Invoke
      /// 
    }
  }
}

Natürlich sollte man das Ganze in ein separates Assembly in Form einer DLL packen, so daß der Code nicht für jede Exe dupliziert werden muß. Das hübsche an dieser Variante ist, daß mit diesem Code nur noch jede neue forwarder-DLL in das richtige Verzeichnis geworfen werden muß und am managed Wrapper-Code nichts angepaßt werden muss. Und das Ganze funktioniert automatisch richtig, wenn ein .NET-Assambly nicht mit "Any CPU" übersetzt ist, sondern für eine spezifische Target-CPU.

In einem seiner letzten Blogs hat Dirkie Code vorgestellt, der zur Laufzeit prüft, ob managed Code unter x64 Windows als 64-bit Prozeß zur Ausführung kommt oder aber als 32-bit Prozeß unter x86 Windows oder x64 Windows läuft. Der einfache Größenvergleich von IntPtr mit 8 hat mich stutzig gemacht und ich habe mir überlegt, ob man das mit P/Invoke irgendwie besser hinkriegen kann, denn der Ansatz liefert dasselbe Ergebnis zurück für alle 64 bit Plattformen, inklusive IA64 oder zukünftigen Plattformen (Can you say 64 bit Pocket PC?). Dabei bin ich auf das traditionelle GetSystemInfo zurückgekommen und so funktioniert's damit dann:

using System;
using System.Collections.Generic;
using System.Text;
using System.Runtime.InteropServices;


namespace platformdetect
{
    enum PROCESSOR_ARCH
    {
        PROCESSOR_ARCHITECTURE_INTEL            = 0,
        PROCESSOR_ARCHITECTURE_MIPS             = 1,
        PROCESSOR_ARCHITECTURE_ALPHA            = 2,
        PROCESSOR_ARCHITECTURE_PPC              = 3,
        PROCESSOR_ARCHITECTURE_SHX              = 4,
        PROCESSOR_ARCHITECTURE_ARM              = 5,
        PROCESSOR_ARCHITECTURE_IA64             = 6,
        PROCESSOR_ARCHITECTURE_ALPHA64          = 7,
        PROCESSOR_ARCHITECTURE_MSIL             = 8,
        PROCESSOR_ARCHITECTURE_AMD64            = 9,
        PROCESSOR_ARCHITECTURE_IA32_ON_WIN64    = 10,
        PROCESSOR_ARCHITECTURE_UNKNOWN          = 0xFFFF
    }


    [StructLayout(LayoutKind.Sequential)]
    class SYSTEM_INFO
    {
        public ushort wProcessorArchitecture;
        public ushort wReserved;
        public uint dwPageSize;
        public IntPtr lpMinimumApplicationAddress;
        public IntPtr lpMaximumApplicationAddress;
        public IntPtr dwActiveProcessorMask;
        public uint dwNumberOfProcessors;
        public uint dwProcessorType;
        public uint dwAllocationGranularity;
        public ushort wProcessorLevel;
        public ushort wProcessorRevision;
    }

    class Program
    {

        [DllImport("kernel32.dll", CharSet = CharSet.Unicode, 
                CallingConvention = CallingConvention.Winapi,
                EntryPoint="GetSystemInfo")]
        private static extern void GetSystemInfo([In, 
                    MarshalAs(UnmanagedType.LPStruct)]
		    SYSTEM_INFO pSysInfo);

        public static ushort GetProcessorArchitecture()
		{
		    SYSTEM_INFO si = new SYSTEM_INFO();
		    GetSystemInfo(si);
		    return si.wProcessorArchitecture;
		}    
        
        static void Main(string[] args)
        {
            ushort uPA = GetProcessorArchitecture();
            string[] platforms = {"x86", "MIPS", "Alpha", 
              "Power PC", "SHX", "ARM", "Itanium", 
              "Alpha 64", "MS intermediate Language", 
              "AMD64", "x86 on Itanium"};
            string strPlatform = "";

            try
            {
                strPlatform = platforms[uPA];
            }
            catch (System.IndexOutOfRangeException e)
            {
                strPlatform = "Unknown";
            }    
            Console.WriteLine("Platform is {0}", strPlatform);
            SYSTEM_INFO si = new SYSTEM_INFO();
            Console.WriteLine("SYSTEM_INFO size is {0} bytes", 
              System.Runtime.InteropServices.Marshal.SizeOf(si));
            Console.ReadKey();
        }
    }
}

Und beim nachschauen, was die Trackback URL von Dirkies Beitrag war, muss ich feststellen, daß Dirkie mit einem Link hierher
das Ganze als "wie es nicht gemacht werden sollte" abtut. Ich finde, damit tut man dieser zugegebenermassen etwas aufwendigeren Variante Unrecht, aber ich denke auch, daß man diese Lösung hier keinesfalls in einer innermost loop verwenden sollte. Aber einmal am Anfang des Programms tut das sicherlich nicht weh...

(Edited 03/11/2006: Oops, Testcode für die IndexOutOfRangeException entfernt)