[PATCH] D26956: Initial changes for porting libFuzzer to Windows.
Marcos Pividori via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 22 12:44:26 PST 2016
mpividori added inline comments.
================
Comment at: lib/Fuzzer/CMakeLists.txt:12
add_library(LLVMFuzzerNoMainObjects OBJECT
FuzzerCrossOver.cpp
FuzzerDriver.cpp
----------------
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".
================
Comment at: lib/Fuzzer/FuzzerDefs.h:31
+#elif _WIN32
+#define LIBFUZZER_WINDOWS 1
#else
----------------
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.
================
Comment at: lib/Fuzzer/FuzzerExtFunctionsWindows.cpp:12
+#include "FuzzerDefs.h"
+#if LIBFUZZER_WINDOWS
+#include "FuzzerExtFunctions.h"
----------------
zturner wrote:
> 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.
Yes, I completely agree.
================
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,
----------------
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()`.
================
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('*');
----------------
zturner wrote:
> 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.
Hi,
I took that code from `directory_iterator_construct(..)` implementation in `llvm/lib/Support/Windows/Path.inc`. I don't know why it check for `:` character, I thought this was a special case to be considered...
Ok, I agree with the code you proposed. It makes more sense for me too.
================
Comment at: lib/Fuzzer/FuzzerIOWindows.cpp:68
+ WIN32_FIND_DATAA FirstFind;
+ HANDLE FindHandle(FindFirstFileA(Path.c_str(), &FirstFind));
+ if (!FindHandle)
----------------
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 :).
================
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)) {
----------------
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.
================
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);
----------------
zturner wrote:
> 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.
Thanks, I completely agree.
================
Comment at: lib/Fuzzer/FuzzerIOWindows.cpp:135
+
+void Printf(const char *Fmt, ...) {
+ va_list ap;
----------------
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`.
================
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;
+ }
----------------
zturner wrote:
> 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.
Yes, I think we can. I will update the code.
I could add this as the first option. If it fails (returns 0), then consider the commands `npos`/`sysctl`. Or directly simplify the code to only use: `std::thread::hardware_concurrency()`.
Repository:
rL LLVM
https://reviews.llvm.org/D26956
More information about the llvm-commits
mailing list