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

[Pavel Labath via Phabricator via llvm-commits]

labath marked an inline comment as done.
labath 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:
> aganea wrote:
> > 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}
> > 
> Sorry for the spam. On a afterthought I think we should leave `ERROR_OPERATION_ABORTED` as an error. Otherwise the input stream will be broken and we might see an (unrelated) error later down the line. It maybe makes sense for LLDB.
> 
> As for my last post, `ReadFile` can be used in a different ways for files and pipes if we want to be optimal; but at this point we don't know so we should leave it the way it is.
No worries. Thanks for investigating this and for the pointers. I'm not very familiar with windows, so this was actually a very interesting and enlightening read for me.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66344