[PATCH] D27234: [libFuzzer] Diff 4 - Split FuzzerUtil implementation for Posix and Windows.

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: lib/Fuzzer/FuzzerUtil.cpp:217
   size_t N;
-  while ((N = fread(Buff, 1, sizeof(Buff), Pipe)) > 0)
+  while ((N = fread(Buff, 1, sizeof(Buff), Pipe)) == sizeof(Buff))
     Out->append(Buff, N);
----------------
kcc wrote:
> zturner wrote:
> > kcc wrote:
> > > why this change from >0 to == sizeof(Buff)
> > > 
> > > In general, please avoid mixing refactoring and functionality changes. 
> > 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.
> hmmm.  am I reading "man fread" wrong? 
> On success, fread() and fwrite() return the number of items read or written.  This number equals the number of bytes transferred only when size is 1.  If an error occurs, or the end of
>        the file is reached, the return value is a short item count (or zero).
> 
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D27234