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

[Zachary Turner via llvm-commits]

zturner added reviewers: kcc, rnk.
zturner added a comment.

+kcc for general stuff
+rnk for the exception handling stuff.

In the future, when you generate patches, you should pass `-U 999999` to git.  This way it will generate full context for the diff.



================
Comment at: lib/Fuzzer/CMakeLists.txt:12
   add_library(LLVMFuzzerNoMainObjects OBJECT
     FuzzerCrossOver.cpp
     FuzzerDriver.cpp
----------------
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.


================
Comment at: lib/Fuzzer/FuzzerDefs.h:31
+#elif _WIN32
+#define LIBFUZZER_WINDOWS 1
 #else
----------------
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`


================
Comment at: lib/Fuzzer/FuzzerExtFunctionsWindows.cpp:12
+#include "FuzzerDefs.h"
+#if LIBFUZZER_WINDOWS
+#include "FuzzerExtFunctions.h"
----------------
Can you remove this and do it at the CMake level instead?  It seems that the other 2 files were already working this way, but maybe we should clean it all up.  I'll have more specific comments later on in the `CMakeLists.txt` file.


================
Comment at: lib/Fuzzer/FuzzerIOPosix.cpp:13
+#include "FuzzerDefs.h"
+#if LIBFUZZER_POSIX
+#include "FuzzerExtFunctions.h"
----------------
Same thing here, this file should not even be sent to the compiler if it's not a posix system.  More comments later in `CMakeLists.txt`


================
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,
----------------
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.


================
Comment at: lib/Fuzzer/FuzzerIOWindows.cpp:57-64
+  if (Path.size() > 0 &&
+      Path[Path.size() - 1] != '\\' &&
+      Path[Path.size() - 1] != ':') {
+    Path.push_back('\\');
+    Path.push_back('*');
+  } else {
+    Path.push_back('*');
----------------
This looks wrong to me.  We should be expecting a path that ends with either a slash or that doesn't end with a slash.  It looks like you're trying to check here "is this the root directory".  But I'm not sure why that matters.  For example, we should translate all of the following:

```
D:\ -> D:\*
D:\foo -> D:\foo\*
d:\foo\ -> D:\foo\*
```

And I also think we should be asserting that the path is not empty, because that wouldn't make any sense.

So I think the correct check should be something like:

```
assert(!Path.empty());
if (Path.back() != '\\')
  Path.push_back('\\');

Path.push_back('*');
```

Let me know if I'm misunderstood though.


================
Comment at: lib/Fuzzer/FuzzerIOWindows.cpp:68
+  WIN32_FIND_DATAA FirstFind;
+  HANDLE FindHandle(FindFirstFileA(Path.c_str(), &FirstFind));
+  if (!FindHandle)
----------------
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.


================
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)) {
----------------
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()`.


================
Comment at: lib/Fuzzer/FuzzerIOWindows.cpp:113-133
+std::string DirPlusFile(const std::string &DirPath,
+                        const std::string &FileName) {
+  return DirPath + "\\" + FileName;
+}
+
+void DupAndCloseStderr() {
+  int OutputFd = _dup(2);
----------------
It doesn't seem good to me that we're copying all these functions and only making very slight modifications.  I would suggest handling it like this:

1. In `FuzzerIO.h`, make the following declarations:

  - char getSeparator();
  - FILE *OpenFile(int Fd, const char *Mode);
  - int DuplicateFile(int fd);
  - int CloseFile(int fd);

2. Also in `FuzzerIO.h`, copy `DirPlusFile`, `DupAndCloseStderr`, and `CloseStdout` to the common location, and use these functions.

3. Implement the above functions in `FileIOPosix.cpp` and `FileIOWindows.cpp` as one-liners.  For example, on Windows, you have something like this:
```
char getSeparator() { return `\\`; }
FILE *OpenFile(int Fd, const char *Mode) { return _fdopen(Fd, Mode); }
```

You could also do it with `#define`s, but this way seems safer.


================
Comment at: lib/Fuzzer/FuzzerIOWindows.cpp:135
+
+void Printf(const char *Fmt, ...) {
+  va_list ap;
----------------
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.


================
Comment at: lib/Fuzzer/FuzzerUtilWindows.cpp:156-164
+int NumberOfCpuCores() {
+  unsigned N = std::thread::hardware_concurrency();
+  if (!N) {
+    Printf("WARNING: std::thread::hardware_concurrency not well defined for "
+      "your platform. Assuming CPU count of 1.\n");
+    N = 1;
+  }
----------------
Is there any reason we can't use this implementation on all platforms?  The Posix implementation seems unnecessarily complicated, when this will work just fine.


================
Comment at: lib/Fuzzer/FuzzerUtilWindows.cpp:179-187
+bool ExecuteCommandAndReadOutput(const std::string &Command, std::string *Out) {
+  FILE *Pipe = _popen(Command.c_str(), "r");
+  if (!Pipe) return false;
+  char Buff[1024];
+  size_t N;
+  while ((N = fread(Buff, 1, sizeof(Buff), Pipe)) > 0)
+    Out->append(Buff, N);
----------------
In the future we should change this to use `CreateProcessA`, but for now this is probably fine.


Repository:
  rL LLVM

https://reviews.llvm.org/D26956