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

[Kostya Serebryany via Phabricator via llvm-commits]

kcc 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);
----------------
zturner wrote:
> zturner wrote:
> > 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.
> Also, the [[ https://msdn.microsoft.com/en-us/library/kt0etdcs.aspx | 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`.
Ok.


Repository:
  rL LLVM

https://reviews.llvm.org/D27234