[PATCH] D66344: Filesystem/Windows: fix inconsistency in readNativeFileSlice API

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added inline comments.


================
Comment at: lib/Support/Windows/Path.inc:1233
+    // EOF is not an error.
+    if (Err == ERROR_BROKEN_PIPE || Err == ERROR_HANDLE_EOF)
       return std::error_code();
----------------
aganea wrote:
> rnk wrote:
> > aganea wrote:
> > > Zachary also handles `ERROR_OPERATION_ABORTED` in `lldb/source/Host/windows/ConnectionGenericFileWindows.cpp` which corresponds to the user hitting Ctrl-C.
> > > 
> > > Also small nit not related to your change: we can't discriminate between a "full success" and a "eof success" from outside of this API, which could cause an extra loop to execute below? (and possibly trigger a different return code in `readNativeFileSlice`)
> > Handling ctrl C seems reasonable to me, but it's not unit testable, so if you want to do it separately and manually test it, that's fine with me too.
> > 
> > I guess the concern with the loop for slice reading is that successive calls to ReadFile may not return the same error code. Right now, the loop is basically dead code since no test reads a slice of more than 4GB of data so far as I know, so it's hard to know what happens. Prior to my changes, I think the loop structure was exactly the same, though: it called `::read` until it got back EOF or zero bytes read. We could check what `read` does in the CRT to see what we should be doing.
> CRT `::read` is actually a wrapper around `ReadFile`. It returns 0 if it's at `EOF` or if `GetLastError() == ERROR_BROKEN_PIPE`; -1 if there's any other error; or the actual bytes read. Apparently, the [[ https://docs.microsoft.com/en-us/windows/win32/fileio/testing-for-the-end-of-a-file | doc  ]] states that `ReadFile` should return `TRUE` and `bytesRead == 0` if reaching the end of file, and `ERROR_HANDLE_EOF` is reserved only for async/overlapped IO which we don't use here (setting `Overlapped.Offset` and `Overlapped.OffsetHigh` is not considered as overlapped IO), so I'm a bit surprised by its usage in this patch, but it's not the first time the behavior is different from what the doc states.
> @labath Pavel, on which version/build of Windows are you testing? I'll give a try on mine to see if the return value from `ReadFile` is the same.
Ah I see, the first read past the end of the file returns "Success" but with less `bytesRead` than asked for. And then any subsequent call with an offset larger that the length of file would return `ERROR_HANDLE_EOF`. At the expense of an extra trip to kernel space, which could be slow, considering recent mitigations and [[ https://en.wikipedia.org/wiki/Control-flow_integrity | CFG ]] (unrelated to this patch, but it'd worth the mention):
{F9813806}



Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66344/new/

https://reviews.llvm.org/D66344