Fix File.ReadAllBytes{Async} for virtual file system files #28388

stephentoub · 2018-03-22T23:11:54Z

Some file systems, like procfs, can return 0 for a file's length, even though it actually contains on-demand computed contents. This breaks File.ReadAllBytes{Async}, which currently assumes that a length of 0 means the file is empty. This commit fixes it by changing the assumption to mean that a length of 0 means we can't depend on the Length and instead need to read until EOF.

This brings File.ReadAllBytes inline with the behavior of File.ReadAllText. Note that this does mean that it'll also start OOM'ing in similar situations, e.g. if you try to do File.ReadAllBytes("/dev/urandom"), it'll eventually OOM whereas today it returns an empty array.

Fixes https://github.com/dotnet/corefx/issues/28343
cc: @JeremyKuhne, @tmds, @VasiliyNovikov

danmoseley · 2018-03-23T00:10:02Z

src/System.IO.FileSystem/tests/File/ReadWriteAllBytes.cs

+        }
+
+        [Theory]
+        [PlatformSpecific(TestPlatforms.Linux)]


Similar test for OSX maybe?

Do you know of any file paths that present this behavior on macOS?

Some file systems, like procfs, can return 0 for a file's length, even though it actually contains on-demand computed contents. This breaks File.ReadAllBytes{Async}, which currently assumes that a length of 0 means the file is empty. This commit fixes it by changing the assumption to mean that a length of 0 means we can't depend on the Length and instead need to read until EOF.

tmds · 2018-03-23T07:37:54Z

Thanks @stephentoub!

VasiliyNovikov · 2018-03-23T07:58:01Z

src/System.IO.FileSystem/src/System/IO/File.cs

@@ -341,6 +350,50 @@ public static byte[] ReadAllBytes(string path)
            }
        }

+        private static byte[] ReadAllBytesUnknownLength(FileStream fs)


Maybe I missed something but it doesn't seem to throw new IOException(SR.IO_FileTooLong2GB) in case of newLength > int.MaxValue like it is done in ReadAllBytes for the case where length is known. And it seems like DefaultArrayPool.Rent doesn't throw either

I think this stops when Rent throws out-of-memory which will happen when you ask it to allocate more than MaxByteArrayLength (or when there is no more memory).

I can't see such limit in DefaultArrayPool:

corefx/src/System.Buffers/src/System/Buffers/DefaultArrayPool.cs

Line 108 in bffef76

buffer = new T[minimumLength];

So it will throw only on some runtime limit or out of memory which seem to be inconsistent with known length case

MaxByteArrayLength is the max array length supported by .NET.

I see. Thanks! That's what I actually didn't realize

VasiliyNovikov · 2018-03-23T08:07:33Z

src/System.IO.FileSystem/src/System/IO/File.cs

+                        uint newLength = (uint)buffer.Length * 2;
+                        if (newLength > MaxByteArrayLength)
+                        {
+                            newLength = (uint)Math.Max(MaxByteArrayLength, buffer.Length + 1);


Can't there be signed integer overflow in buffer.Length + 1 at some point? We don't seem to have any hard limit for this whole loop. Maybe you actually meant Math.Min here?

For buffer.Length + 1 > MaxByteArrayLength case, Rent will end up throwing (I think).

Do I understand it correctly - when buffer.Length becomes MaxByteArrayLength on all further iterations it grows by one byte?

@tmds That's the thing - DefaultArrayPool.Rent implementation doesn't seem have any limits (or maybe I missed one)

From what I see this is how it behaves:
On each iteration size doubles;
Then it tries MaxByteArrayLength;
And on the next iteration (buffer.Length + 1), Rent throws.

So I'm looking into DefaultArrayPool.Rent implementation and in case requested length is greater than supported by pool it just creates an array of requested length:

corefx/src/System.Buffers/src/System/Buffers/DefaultArrayPool.cs

Line 108 in bffef76

buffer = new T[minimumLength];

So newermind. I didn't realize that MaxArrayLength is the actual maximum supported by .NET

VasiliyNovikov · 2018-03-23T08:45:42Z

Q. Didn't we suppose to limit this fix only for Unixes?

stephentoub · 2018-03-23T12:42:22Z

Didn't we suppose to limit this fix only for Unixes?

I considered that but decided not to special case because a) this only applies to the minor case where the file length is 0, b) it adds minimal cost to that case (one read operation), c) it actually saves some cost in that case as well by using Array.Empty<byte>() rather than allocating a new byte[0], and d) with the advent of the Windows Subsystem for Linux it seems feasible that at some point we could find ourselves in a similar situation on Windows, or potentially with other Windows file system drivers in general, though I don't have a concrete example.

If others disagree, though, it's trivial to change it to just be for non-Windows. @JeremyKuhne?

VasiliyNovikov · 2018-03-23T13:56:55Z

d) with the advent of the Windows Subsystem for Linux it seems feasible that at some point we could find ourselves in a similar situation on Windows

Strictly speaking when it runs under WSL it recognizes itself running on Linux so Windows-only code is not executed there

stephentoub · 2018-03-23T13:58:16Z

When it runs under WSL it recognizes itself running on Linux so Windows-only code is not executed there

I understand that. I'm saying that this forward-proofs it against a potential future where you can access those paths from outside of the subsystem. Or against other arbitrary file system drivers that behave similarly.

VasiliyNovikov · 2018-03-23T14:01:13Z

I see. Thanks

JeremyKuhne · 2018-03-23T18:44:08Z

it's trivial to change it to just be for non-Windows. @JeremyKuhne?

I'm fine with it being included on Windows for the purposes you mentioned.

…refx#28388) Some file systems, like procfs, can return 0 for a file's length, even though it actually contains on-demand computed contents. This breaks File.ReadAllBytes{Async}, which currently assumes that a length of 0 means the file is empty. This commit fixes it by changing the assumption to mean that a length of 0 means we can't depend on the Length and instead need to read until EOF. Commit migrated from dotnet/corefx@77bf407

JeremyKuhne approved these changes Mar 22, 2018

View reviewed changes

stephentoub force-pushed the readallbytes branch from 1a6538c to 8a5bfc2 Compare March 22, 2018 23:35

danmoseley reviewed Mar 23, 2018

View reviewed changes

stephentoub force-pushed the readallbytes branch from 8a5bfc2 to c40fa44 Compare March 23, 2018 02:09

stephentoub merged commit 77bf407 into dotnet:master Mar 23, 2018

stephentoub deleted the readallbytes branch March 23, 2018 03:11

VasiliyNovikov reviewed Mar 23, 2018

View reviewed changes

karelz added this to the 2.1.0 milestone Mar 27, 2018

adamsitnik mentioned this pull request Nov 15, 2021

Minor File.ReadAllBytes* improvements dotnet/runtime#61519

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix File.ReadAllBytes{Async} for virtual file system files #28388

Fix File.ReadAllBytes{Async} for virtual file system files #28388

stephentoub commented Mar 22, 2018

danmoseley Mar 23, 2018

stephentoub Mar 23, 2018

tmds commented Mar 23, 2018

VasiliyNovikov Mar 23, 2018

tmds Mar 23, 2018

VasiliyNovikov Mar 23, 2018

tmds Mar 23, 2018

VasiliyNovikov Mar 23, 2018

VasiliyNovikov Mar 23, 2018

tmds Mar 23, 2018

VasiliyNovikov Mar 23, 2018

VasiliyNovikov Mar 23, 2018

tmds Mar 23, 2018

VasiliyNovikov Mar 23, 2018

VasiliyNovikov Mar 23, 2018

VasiliyNovikov commented Mar 23, 2018

stephentoub commented Mar 23, 2018

VasiliyNovikov commented Mar 23, 2018 •

edited

Loading

stephentoub commented Mar 23, 2018

VasiliyNovikov commented Mar 23, 2018

JeremyKuhne commented Mar 23, 2018

Fix File.ReadAllBytes{Async} for virtual file system files #28388

Fix File.ReadAllBytes{Async} for virtual file system files #28388

Conversation

stephentoub commented Mar 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmds commented Mar 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VasiliyNovikov commented Mar 23, 2018

stephentoub commented Mar 23, 2018

VasiliyNovikov commented Mar 23, 2018 • edited Loading

stephentoub commented Mar 23, 2018

VasiliyNovikov commented Mar 23, 2018

JeremyKuhne commented Mar 23, 2018

VasiliyNovikov commented Mar 23, 2018 •

edited

Loading