Implement utils functions for Windows, using vectored exception handlers as equivalent of POSIX signals. Implementation of timers is incomplete.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
My comments are mostly just things to be aware of. No changes required.
lib/Fuzzer/FuzzerUtil.cpp | ||
---|---|---|
205 | Implicit conversion from unsigned to signed. Probably not an issue until processors get a lot bigger. Relying on implicit conversions makes it hard to turn on conversion warnings, which might help detect real bugs in other parts of the code. | |
lib/Fuzzer/FuzzerUtilPosix.cpp | ||
56 | Be aware that this name clashes with the Windows function SetTimer that's in scope here. You're OK, since this is a C++ file and so it'll be name-mangled and thus distinct. If this had been a C source file instead, though, this would be a problem. | |
lib/Fuzzer/FuzzerUtilWindows.cpp | ||
159 | Be aware that this converts from an unsigned to a signed type. This is not a big deal with modern Windows, since it's extremely unlikely you'll ever see a PID approaching 2^31, but it's at least theoretically possible. If some logging somewhere prints this as a signed int, it may not look like the value you'd see in other Window stools. Raymond Chen mentions seeing PIDs approaching 4 billion, but I suspect that's when they were actually pointers in disguise. http://stackoverflow.com/a/17868515/1386054 | |
186 | I'm not sure if libfuzzer has its own coding style guidelines, but I'd probably use nullptr rather than NULL. |
lib/Fuzzer/FuzzerUtil.cpp | ||
---|---|---|
199 | I noticed that in the SleepSeconds function, there is a comment that they couldn't use stl because they wanted to avoid coverage of instrumented libc++. You might wait for comments from kcc@ about whether this is relevant here as well. If so, we might need to go back to pure C implementations for each platform. | |
lib/Fuzzer/FuzzerUtilWindows.cpp | ||
170 | This function appears almost entirely the same except _popen vs popen. Perhaps this could be hidden behind a function called OpenProcessPipe and then ExecuteCommandAndReadOutput could be shared. | |
174–175 | This looks wrong to me. fread does not indicate an error by returning a value less than 0, it indicates an error by returning a value different from the requested number of bytes. Then, you check feof and ferr to determine whether it was an error or an end of file. This code doesn't seem to handle the case where there was an error reading the process output. |
lib/Fuzzer/FuzzerUtil.h | ||
---|---|---|
64 ↗ | (On Diff #79692) | No such ifdefs, please. |
lib/Fuzzer/FuzzerUtil.h | ||
---|---|---|
64 ↗ | (On Diff #79692) | It is called directly from non platform specific code. The alternative would be to add a function like SearchMemory(), and in Posix file it just calls memmem, and in Win32 it does its own algorithm. Then update callsites to use SearchMemory() instead. |
lib/Fuzzer/FuzzerUtil.h | ||
---|---|---|
64 ↗ | (On Diff #79692) | Yes, that would be fine. |
lib/Fuzzer/FuzzerUtil.cpp | ||
---|---|---|
197–198 | That change was my suggestion. The code is incorrect as written because fread doesn't use <= 0 to indicate an error, it uses != request_size, then you check ferror. In any case, your point about mixing functionality and refactoring changes is still valid, so it might be worth doing this in a followup as you suggest. |
lib/Fuzzer/FuzzerUtil.cpp | ||
---|---|---|
197–198 | hmmm. am I reading "man fread" wrong? the file is reached, the return value is a short item count (or zero). |
lib/Fuzzer/FuzzerUtil.cpp | ||
---|---|---|
197–198 | It's a little bit ambiguous, but it doesn't explicitly state that 0 is for errors and the less specific condition of < request_size is eof. Indeed, the very next line in the man page is "fread() does not distinguish between end-of-file and error, and callers must use feof(3) and ferror(3) to determine which occurred." So it seems like the modified version is slightly more correct. |
lib/Fuzzer/FuzzerUtil.cpp | ||
---|---|---|
197–198 | Also, the Microsoft documentation says similarly "fread returns the number of full items actually read, which may be less than count if an error occurs or if the end of the file is encountered before reaching count. Use the feof or ferror function to distinguish a read error from an end-of-file condition." So again, it seems you can't use the return value to distinguish feof and ferr, and it seems theoretically possible that a value greater than 0 but less than the number of bytes requested could still be an error, which would only be determinable through ferror. |
lib/Fuzzer/FuzzerUtil.cpp | ||
---|---|---|
197–198 | Ok. |
+ Remove memmem() and add SearchMemory() as suggested.
+ Remove changes to fread. I will add that changes in a new diff as suggested.
lib/Fuzzer/FuzzerUtil.cpp | ||
---|---|---|
199 | Kostya, can you confirm that the call to std::thread::hardware_concurrency() here is ok? The call to sleep earlier has a comment explicitly stating that stl can't be used. Is the same thing going to be true here? |
LGTM (I did not read the widows code)
As for every other patch, please ensure check-fuzzer still work on Linux.
lib/Fuzzer/FuzzerUtil.cpp | ||
---|---|---|
199 | Yep, should be fine. |
why this change from >0 to == sizeof(Buff)
In general, please avoid mixing refactoring and functionality changes.