`ReadFile`: semantics of `bytesRead`

[lostmsu]

Just spent a couple of hours trying to figure out an issue with my FS.

Turned out that Dokan expects the implementation of ReadFile to always fill the entire buffer when offset + buffer.Length are within the file length, and read exactly remaining bytes when offset + buffer.Length are more than file length.

I was calling .NET Stream.Read with the buffer, and my filesystem would return garbage to some programs. The reason is Stream.Read by its contract does not have to fill your entire buffer. For example, when reading from a TCP connection if you specify 1MiB buffer and 1KiB packet arrives, NetworkStream will fill 1KiB and return 1024 even though the connection is still open and it will be possible to read more data in the future.

Now I don't know what the kernel APIs expect in this case, but IMHO one of the following should be done:

• documenting this behavior
• if this behavior is the result of a bug (perhaps the value returned in bytesRead is not used everywhere it should be), fixing that bug
• considering changing .NET wrapper code to repeat ReadFile calls until either buffer is filled or end of file reached (this would remove the need for documenting the discrepancy)

[Liryna]

Hi @lostmsu ,

Turned out that Dokan expects the implementation of ReadFile to always fill the entire buffer when offset + buffer.Length are within the file length, and read exactly remaining bytes when offset + buffer.Length are more than file length.

Dokan does use bytesRead to inform the system how much was actually read. It does not expect that the buffer is always fill.
That said, the calling application must handle the bytesRead it will receive and reissue a read to get the next data.

[lostmsu]

The scenario I worked with is `dumpbin` from C++ build tools, and it failed with my FS until I did fill the buffer (but didn't fail on a raw file). There might be a bug in .NET wrapper. In the decompilation it looked like it copies the whole buffer regardless of `bytesRead` for some reason, which is suspect. Perhaps some programs rely on only `bytesRead` to be overwritten. E.g. if they read into a circular buffer.

[LTRData]

I assume that you have seen this line:
https://github.com/dokan-dev/dokan-dotnet/blob/master/DokanNet%2FDokanOperationProxy.cs#L411

And yes, it looks like it copies the whole buffer. But the value of rawReadLegth is reported back to the OS because it is a ref parameter, so this should never cause any issues. Have you checked and made absolutely sure that you return the correct read length in the bytesRead output parameter?

[lostmsu]

Reasonably sure. This is the end of the method before the fix

bytesRead = stream.Read(buffer, 0, buffer.Length);
return DokanResult.Success;

[LTRData]

Okay. I think it would be interesting in your case to just try and change the Marshal.Copy code in the library to use the reported length instead of initial length and see if that makes any change in this case.

[lostmsu]

So I tried the suggested change, and dumpbin is still misbehaving when I read less than requested. Could still be bug elsewhere in dokan, but maybe it is a bug in dumpbin, where they unreasonably expect to read the exact amount requested.

This is what I put into DokanOperationProxy:

if (rawReadLength < 0)
    throw new ArgumentOutOfRangeException("bytesRead", rawReadLength, "bytesRead must be greater than or equal to zero.");
if (rawReadLength > rawBufferLength)
    throw new ArgumentOutOfRangeException("bytesRead", rawReadLength, "bytesRead must be less than or equal to the length of the buffer.");
Marshal.Copy(buffer, 0, rawBuffer, rawReadLength);

This is what I put into Mirror.ReadFile (+ further down replaced all buffer.Length with read):

int read = Math.Min(buffer.Length, 128);

I copied apphost.exe to the mirrored folder and tried to dumpbin /dependents mirror\apphost.exe which gave

LINK : warning LNK4094: 'apphost.exe' is an MS-DOS executable; use EXEHDR to dump it

When I replace with read = buffer.Length it works correctly.

The sad part is that if Microsoft tool is misbehaving like that, how many other apps silently rely on a similar behavior? I guess I will have to ensure full reads now in all file system implementations for compatibility purposes. 😭

[LTRData]

Thanks for testing! Based on your results, it seems like something we probably need to address somehow. I'll take a closer look at this next week when I am back in office.

[lostmsu]

BTW, if you are considering forcing the compatibility thing at Dokan level, I think simply repeating reads until buffer is filled is going to break potential scenarios where FS is backed by something like sockets.

[Liryna]

I have confirmed that bytesRead behave as expected. The caller will receive the value set by the FS implementation.
The only thing I saw is that the library did not use this value to copy the data to the destination buffer 2f15c8a Not sure if this solves dumpbin case but worth a try.

Procmon might also be useful to see what is going on with dumpbin.

@lostmsu Are you able to reproduce the issue with the C# Mirror ? C Mirror / Memfs ? It would help find out where the issue is

[lostmsu]

@Liryna please see above. You can repro the issue with Mirror if you limit the amount of bytes read to 128 in ReadFile implementation.

[lostmsu]

@Liryna also, see my suggested change instead of your fix, that guards against bad number returned by bytesRead. It is nice to have foot guards 😁

[LTRData]

@Liryna please see above. You can repro the issue with Mirror if you limit the amount of bytes read to 128 in ReadFile implementation.

But that is not a correct implementation. If fewer than requested bytes are returned for a file by the file system, it means that the read reached end of file. It is not like reading from devices such as communication channels. So, effectively, dumpbin will obviously treat the file as just 128 bytes long if you do this (which means it will only get the MZ header and not the PE header and assume it is a DOS exe etc etc).

[LTRData]

@Liryna also, see my suggested change instead of your fix, that guards against bad number returned by bytesRead. It is nice to have foot guards 😁

It is a bit of an issue when an implementation actually wants to return fewer than requested bytes. That should be perfectly fine to do, to indicate that only that much data was available towards the end of the file.

[lostmsu]

@LTRData the guards are against negative values and values that are higher than requested.