[PATCH] D26956: Initial changes for porting libFuzzer to Windows.

[Zachary Turner via llvm-commits]

zturner added inline comments.


================
Comment at: lib/Fuzzer/CMakeLists.txt:12
   add_library(LLVMFuzzerNoMainObjects OBJECT
     FuzzerCrossOver.cpp
     FuzzerDriver.cpp
----------------
rnk wrote:
> mpividori wrote:
> > zturner wrote:
> > > I mentioned below that we should not have the preprocessor definitions use to conditionally compile code or not, but instead use CMake.  Here is how you would do it.  You might want to skip thsi comment for now and come back to it after you read the rest of the comments.
> > > 
> > > ```
> > > set(FUZZER_SOURCES
> > >     FuzzerCrossOver.cpp
> > >     FuzzerDriver.cpp
> > >     FuzzerIO.cpp
> > >     FuzzerLoop.cpp
> > >     FuzzerMutate.cpp
> > >     FuzzerSHA1.cpp
> > >     FuzzerTracePC.cpp
> > >     FuzzerTraceState.cpp
> > >     FuzzerUtil.cpp
> > > )
> > > 
> > > if (CMAKE_SYSTEM_NAME MATCHES "Windows")
> > >     list(APPEND FUZZER_SOURCES
> > >         FuzzerUtilWindows.cpp
> > >         FuzzerIOWindows.cpp
> > >         FuzzerExtFunctionsWindows.cpp
> > >     )    
> > > else()
> > >     list(APPEND FUZZER_SOURCES
> > >         FuzzerUtilPosix.cpp
> > >         FuzzerIOPosix.cpp
> > >     )
> > > 
> > >     if (CMAKE_SYSTEM_NAME MATCHES "Linux")
> > >         FuzzerUtilLinux.cpp
> > >         FuzzerExtFunctionsLinux.cpp
> > >     elseif(CMAKE_SYSTEM_NAME MATCHES "Darwin")
> > >         FuzzerUtilDarwin.cpp
> > >         FuzzerExtFunctionsApple.cpp
> > >     endif()
> > > endif()
> > > 
> > > add_library(LLVMFuzzerNoMainObjects OBJECT ${FUZZER_SOURCES})
> > > ```
> > > 
> > > This way you can remove all the `#if <foo>` from the individual source files.
> > Thanks! Yes, I completely agree.
> > I continued with the current approach in the implementation and left it as a future modification, because I wanted to know your opinion. I see that there is a bash script: "build.sh" that simply compiles all .cpp files and generates the LLVM library. If we remove the macros guards, then that script will not work.
> > I guess the purpose of that script is to make libFuzzer independent of the rest of the LLVM repository.
> > Would you agree if I simply remove that script? Also, I can modify it to ignore files ending in "Windows".
> FWIW, the sanitizer code takes the opposite approach, specifically to simplify build logic.
Let's wait and see what kcc has to say on this.


================
Comment at: lib/Fuzzer/FuzzerDefs.h:31
+#elif _WIN32
+#define LIBFUZZER_WINDOWS 1
 #else
----------------
mpividori wrote:
> zturner wrote:
> > I think you will need to add
> > 
> > ```
> > #define LIBFUZZER_POSIX 0
> > #define LIBFUZZER_LINUX 0
> > #define LIBFUZZER_APPLE 0
> > ```
> > 
> > similarly, in the previous 2 branches of the `#if`, you will need to `#define LIBFUZZER_WINDOWS 0`
> Hi,
> I omitted them to simplify the code. (when not defined, the macros assume value 0).
> If you think it is clearer, I can include them.
As long as we're consistent it's fine.  If we're going to remove the 0 values, then remove them from the other cases too.


================
Comment at: lib/Fuzzer/FuzzerIOWindows.cpp:29
+bool IsFile(const std::string &Path) {
+  HANDLE FileHandle(
+      CreateFileA(Path.c_str(), 0, FILE_SHARE_READ, NULL, OPEN_EXISTING,
----------------
mpividori wrote:
> zturner wrote:
> > Check the code in `llvm/lib/Support/Windows/Path.inc` for the `status()` function.  There are some subtleties that you should be aware of that this code doesn't handle.  You can probably just rip the code straight from there, changing `W` to `A` so we don't have to worry about widening the paths.
> Hi,
> Thanks. Yes, I based my implementation in the implementation of `status()` / `getStatus()` from the Support Library.
> What I haven't included is the function `isReservedName`, because I assumed that reserved names would return `GetFileType()` different to `FILE_TYPE_DISK`  (probably `FILE_TYPE_CHAR` ?). Do you think I should include check for reserved names? The rest of the code is similar to the implementation of `status()`.
Hmm.  The function I'm looking at first calls `GetFileAttributes()`, and then only if it is a reparse point (kinda like a symlink) does it open the handle.  This allows it to "follow the symlink" to get info about the target.  But the advantage of that approach is that `GetFileAttributes()` doesn't actually open the handle -- which can fail in certain scenarios -- unless it has to.

Are we looking at the same function?  The one I'm looking at is `lib/Support/Windows/Path.inc` line 464.


================
Comment at: lib/Fuzzer/FuzzerIOWindows.cpp:68
+  WIN32_FIND_DATAA FirstFind;
+  HANDLE FindHandle(FindFirstFileA(Path.c_str(), &FirstFind));
+  if (!FindHandle)
----------------
mpividori wrote:
> zturner wrote:
> > Some Windows api functions return `nullptr` if it fails, and some return `INVALID_HANDLE_VALUE`.  You need to always check which one is returned to indicate an error.  This one returns `INVALID_HANDLE_VALUE`, so the check here is incorrect.
> Thanks,
> Ok. I took that code from `directory_iterator_construct(..)` implementation in `llvm/lib/Support/Windows/Path.inc`, so I think I should update that implementation too :).
> 
Interesting, yea that looks like a bug in the original code.  Please submit that in a separate patch though since it's unrelated to libfuzzer.


================
Comment at: lib/Fuzzer/FuzzerIOWindows.cpp:84-94
+    HANDLE FileHandle(
+        CreateFileA(FileName.c_str(), 0, FILE_SHARE_READ, NULL, OPEN_EXISTING,
+                    FILE_FLAG_BACKUP_SEMANTICS, 0));
+
+    BY_HANDLE_FILE_INFORMATION Info;
+
+    if (!GetFileInformationByHandle(FileHandle, &Info)) {
----------------
mpividori wrote:
> zturner wrote:
> > The same comment here applies as above.  We shouldn't be opening a handle to the file unless we really need to, because that can lead to sharing violations if someone else is using the file.  If you move the code for `IsFile()` (which I commented on above) into a separate function called `Stat()` or something that returns the file type, then you could call that function both here and in `IsFile()`.
> Hi. I am not sure I understand properly. The `status()` implementation in `llvm/lib/Support/Windows/Path.inc`, opens a handle to get the file type, so I think it would be the same that current implementation. As far as I understand, it is not possible to know the file type without opening a handle. Am I missing something?
> Thanks.
You are right, I looked over that function again.  But I still think we can do better.  I think the status function there is intended to be more general.  It retrieves more information than just whether or not it is a regular file or directory.  For example, if you call `GetFileAttributesA()` and the output parameter is equal to `FILE_ATTRIBUTE_NORMAL`, you can return true immediately.  Only if it contains `FILE_ATTRIBUTE_REPARSE_POINT` do you need to open the handle and do the more complicated aproach.

This will allow it to work in more scenarios, as opening a handle can fail in strange situations.


================
Comment at: lib/Fuzzer/FuzzerIOWindows.cpp:135
+
+void Printf(const char *Fmt, ...) {
+  va_list ap;
----------------
mpividori wrote:
> zturner wrote:
> > Printf code is exactly the same in both places, so this function should just be declared and implemented in `FuzzerIO.h` and `FuzzerIO.cpp`, there is no reason to copy it.
> Yes, it is repeated because I included all functions working with the global variable `OutputFile` in the same .cpp file where it is defined.
> I can declare `OutputFile` as extern variable in `FuzzerDefs.h` and move Printf code to `FuzzerIO.cpp`.
Maybe just have Printf take the file as first argument?  So that instead of `printf` it's more like `fprintf`?


Repository:
  rL LLVM

https://reviews.llvm.org/D26956