[io] Fix a bug where large reads would return partial data.

Bug: #51071
Change-Id: Ia64d803c9709b106e52a1c671c1c3288c051bd85
Tested: ci + new test
CoreLibraryReviewExempt: bug fix only for vm
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/279204
Reviewed-by: Alexander Aprelev <[email protected]>
Reviewed-by: Martin Kustermann <[email protected]>
Commit-Queue: Brian Quinlan <[email protected]>

Author: brianquinlan

runtime/bin/file.h

@@ -143,9 +143,15 @@ class File : public ReferenceCounted<File> {
                     int64_t length,
                     void* start = nullptr);
 
-  // Read/Write attempt to transfer num_bytes to/from buffer. It returns
-  // the number of bytes read/written.
+  // Read at most 'num_bytes' from the file. It may read less than 'num_bytes'
+  // even when EOF is not encountered. If no data is available then `Read`
+  // will block waiting for input (e.g. if the file represents a pipe that
+  // contains no unread data). A positive return value indicates the number of
+  // bytes read. Zero indicates an attempt to read past the end-of-file. -1
+  // indicates an error.
   int64_t Read(void* buffer, int64_t num_bytes);
+  // Attempt to write 'num_bytes' bytes from 'buffer'. It returns the number
+  // of bytes written.
   int64_t Write(const void* buffer, int64_t num_bytes);
 
   // ReadFully and WriteFully do attempt to transfer num_bytes to/from

sdk/lib/io/file_impl.dart

@@ -7,6 +7,23 @@ part of dart.io;
 // Read the file in blocks of size 64k.
 const int _blockSize = 64 * 1024;
 
+// The maximum number of bytes to read in a single call to `read`.
+//
+// On Windows and macOS, it is an error to call
+// `read/_read(fildes, buf, nbyte)` with `nbyte >= INT_MAX`.
+//
+// The POSIX specification states that the behavior of `read` is
+// implementation-defined if `nbyte > SSIZE_MAX`. On Linux, the `read` will
+// transfer at most 0x7ffff000 bytes and return the number of bytes actually.
+// transfered.
+//
+// A smaller value has the advantage of:
+// 1. making vm-service clients (e.g. debugger) more responsive
+// 2. reducing memory overhead (since `readInto` copies memory)
+//
+// A bigger value reduces the number of system calls.
+const int _maxReadSize = 16 * 1024 * 1024; // 16MB.
+
 class _FileStream extends Stream<List<int>> {
   // Stream controller.
   late StreamController<Uint8List> _controller;
@@ -500,7 +517,7 @@ class _File extends FileSystemEntity implements File {
   }
 
   Future<Uint8List> readAsBytes() {
-    Future<Uint8List> readDataChunked(RandomAccessFile file) {
+    Future<Uint8List> readUnsized(RandomAccessFile file) {
       var builder = new BytesBuilder(copy: false);
       var completer = new Completer<Uint8List>();
       void read() {
@@ -518,13 +535,37 @@ class _File extends FileSystemEntity implements File {
       return completer.future;
     }
 
+    Future<Uint8List> readSized(RandomAccessFile file, int length) {
+      var data = Uint8List(length);
+      var offset = 0;
+      var completer = new Completer<Uint8List>();
+      void read() {
+        file.readInto(data, offset, min(offset + _maxReadSize, length)).then(
+            (readSize) {
+          if (readSize > 0) {
+            offset += readSize;
+            read();
+          } else {
+            assert(readSize == 0);
+            if (offset < length) {
+              data = Uint8List.sublistView(data, 0, offset);
+            }
+            completer.complete(data);
+          }
+        }, onError: completer.completeError);
+      }
+
+      read();
+      return completer.future;
+    }
+
     return open().then((file) {
       return file.length().then((length) {
         if (length == 0) {
           // May be character device, try to read it in chunks.
-          return readDataChunked(file);
+          return readUnsized(file);
         }
-        return file.read(length);
+        return readSized(file, length);
       }).whenComplete(file.close);
     });
   }
@@ -539,11 +580,27 @@ class _File extends FileSystemEntity implements File {
         var builder = new BytesBuilder(copy: false);
         do {
           data = opened.readSync(_blockSize);
-          if (data.length > 0) builder.add(data);
+          if (data.length > 0) {
+            builder.add(data);
+          }
         } while (data.length > 0);
         data = builder.takeBytes();
       } else {
-        data = opened.readSync(length);
+        data = Uint8List(length);
+        var offset = 0;
+
+        while (offset < length) {
+          final readSize = opened.readIntoSync(
+              data, offset, min(offset + _maxReadSize, length));
+          if (readSize == 0) {
+            break;
+          }
+          offset += readSize;
+        }
+
+        if (offset < length) {
+          data = Uint8List.view(data.buffer, 0, offset);
+        }
       }
       return data;
     } finally {

tests/standalone/io/file_test.dart

@@ -14,6 +14,7 @@ import 'dart:async';
 import 'dart:convert';
 import 'dart:collection';
 import 'dart:io';
+import 'dart:typed_data';
 
 import "package:async_helper/async_helper.dart";
 import "package:expect/expect.dart";
@@ -41,6 +42,10 @@ class MyListOfOneElement extends Object
 
 class FileTest {
   static late Directory tempDirectory;
+  static late File largeFile;
+  static final largeFileLastBytes = [104, 101, 108, 108, 111];
+  static final largeFileSize = (1 << 31) + largeFileLastBytes.length;
+
   static int numLiveAsyncTests = 0;
 
   static void asyncTestStarted() {
@@ -63,6 +68,26 @@ class FileTest {
     });
   }
 
+  static void createLargeFile(Function doNext) {
+    // Increase the test count to prevent the temp directory from being
+    // deleted.
+    ++numLiveAsyncTests;
+    final buffer = Uint8List(1 << 24);
+    largeFile = new File('${tempDirectory.path}/test_large_file');
+    IOSink output = largeFile.openWrite();
+    for (var i = 0;
+        i < (largeFileSize - largeFileLastBytes.length) / buffer.length;
+        ++i) {
+      output.add(buffer);
+    }
+    output.add(largeFileLastBytes);
+    output.flush().then((_) => output.close());
+    output.done.then((_) {
+      --numLiveAsyncTests;
+      doNext();
+    });
+  }
+
   static void deleteTempDirectory() {
     tempDirectory.deleteSync(recursive: true);
   }
@@ -1132,6 +1157,27 @@ class FileTest {
     });
   }
 
+  static void testReadAsBytesLargeFile() {
+    asyncTestStarted();
+    // On Windows and macOS, it is an error to call
+    // `read/_read(fildes, buf, nbyte)` with `nbyte >= INT_MAX` and, on Linux,
+    // it returns partial data.
+    largeFile.readAsBytes().then((bytes) {
+      Expect.equals(largeFileSize, bytes.length);
+      Expect.listEquals(
+          largeFileLastBytes,
+          Uint8List.sublistView(
+              bytes, bytes.length - largeFileLastBytes.length));
+      asyncTestDone("testReadAsBytesLargeFile");
+    }, onError: (e) {
+      // The test fails on 32-bit platforms or when using compressed
+      // pointers. It is not possible to identify when running with
+      // compressed pointers.
+      Expect.type<OutOfMemoryError>(e);
+      asyncTestDone("testReadAsBytesLargeFile");
+    });
+  }
+
   static void testReadAsBytesSync() {
     var name = getFilename("fixed_length_file");
     var bytes = new File(name).readAsBytesSync();
@@ -1145,6 +1191,27 @@ class FileTest {
     Expect.equals(bytes.length, 0);
   }
 
+  static testReadAsBytesSyncLargeFile() {
+    asyncTestStarted();
+    // On Windows and macOS, it is an error to call
+    // `read/_read(fildes, buf, nbyte)` with `nbyte >= INT_MAX` and, on Linux,
+    // it returns partial data.
+    Uint8List? bytes;
+    try {
+      bytes = largeFile.readAsBytesSync();
+    } on OutOfMemoryError catch (e) {
+      // The test fails on 32-bit platforms or when using compressed
+      // pointers. It is not possible to identify when running with
+      // compressed pointers.
+      asyncTestDone("testReadAsBytesSyncLargeFile");
+      return;
+    }
+    Expect.equals(largeFileSize, bytes.length);
+    Expect.listEquals(largeFileLastBytes,
+        Uint8List.sublistView(bytes, bytes.length - largeFileLastBytes.length));
+    asyncTestDone("testReadAsBytesSyncLargeFile");
+  }
+
   static void testReadAsText() {
     asyncTestStarted();
     var name = getFilename("fixed_length_file");
@@ -1612,7 +1679,8 @@ class FileTest {
 
     var absFile = file.absolute;
     Expect.isTrue(absFile.isAbsolute);
-    Expect.isTrue(absFile.path.startsWith(tempDirectory.path));
+    Expect.isTrue(absFile.path.startsWith(tempDirectory.path),
+        '${absFile.path} not in ${tempDirectory.path}');
 
     Expect.equals("content", absFile.readAsStringSync());
 
@@ -1729,7 +1797,11 @@ class FileTest {
       testSetLastAccessedSyncDirectory();
       testDoubleAsyncOperation();
       testAbsolute();
-      asyncEnd();
+      createLargeFile(() {
+        testReadAsBytesLargeFile();
+        testReadAsBytesSyncLargeFile();
+        asyncEnd();
+      });
     });
   }
 }

tests/standalone_2/io/file_test.dart

@@ -16,6 +16,7 @@ import 'dart:async';
 import 'dart:convert';
 import 'dart:collection';
 import 'dart:io';
+import 'dart:typed_data';
 
 import "package:async_helper/async_helper.dart";
 import "package:expect/expect.dart";
@@ -43,6 +44,10 @@ class MyListOfOneElement extends Object
 
 class FileTest {
   static Directory tempDirectory;
+  static File largeFile;
+  static final largeFileLastBytes = [104, 101, 108, 108, 111];
+  static final largeFileSize = (1 << 31) + largeFileLastBytes.length;
+
   static int numLiveAsyncTests = 0;
 
   static void asyncTestStarted() {
@@ -65,6 +70,26 @@ class FileTest {
     });
   }
 
+  static void createLargeFile(Function doNext) {
+    // Increase the test count to prevent the temp directory from being
+    // deleted.
+    ++numLiveAsyncTests;
+    final buffer = Uint8List(1 << 24);
+    largeFile = new File('${tempDirectory.path}/test_large_file');
+    IOSink output = largeFile.openWrite();
+    for (var i = 0;
+        i < (largeFileSize - largeFileLastBytes.length) / buffer.length;
+        ++i) {
+      output.add(buffer);
+    }
+    output.add(largeFileLastBytes);
+    output.flush().then((_) => output.close());
+    output.done.then((_) {
+      --numLiveAsyncTests;
+      doNext();
+    });
+  }
+
   static void deleteTempDirectory() {
     tempDirectory.deleteSync(recursive: true);
   }
@@ -1136,6 +1161,27 @@ class FileTest {
     });
   }
 
+  static void testReadAsBytesLargeFile() {
+    asyncTestStarted();
+    // On Windows and macOS, it is an error to call
+    // `read/_read(fildes, buf, nbyte)` with `nbyte >= INT_MAX` and, on Linux,
+    // it returns partial data.
+    largeFile.readAsBytes().then((bytes) {
+      Expect.equals(largeFileSize, bytes.length);
+      Expect.listEquals(
+          largeFileLastBytes,
+          Uint8List.sublistView(
+              bytes, bytes.length - largeFileLastBytes.length));
+      asyncTestDone("testReadAsBytesLargeFile");
+    }, onError: (e) {
+      // The test fails on 32-bit platforms or when using compressed
+      // pointers. It is not possible to identify when running with
+      // compressed pointers.
+      Expect.type<OutOfMemoryError>(e);
+      asyncTestDone("testReadAsBytesLargeFile");
+    });
+  }
+
   static void testReadAsBytesSync() {
     var name = getFilename("fixed_length_file");
     var bytes = new File(name).readAsBytesSync();
@@ -1149,6 +1195,27 @@ class FileTest {
     Expect.equals(bytes.length, 0);
   }
 
+  static testReadAsBytesSyncLargeFile() {
+    asyncTestStarted();
+    // On Windows and macOS, it is an error to call
+    // `read/_read(fildes, buf, nbyte)` with `nbyte >= INT_MAX` and, on Linux,
+    // it returns partial data.
+    Uint8List bytes;
+    try {
+      bytes = largeFile.readAsBytesSync();
+    } on OutOfMemoryError catch (e) {
+      // The test fails on 32-bit platforms or when using compressed
+      // pointers. It is not possible to identify when running with
+      // compressed pointers.
+      asyncTestDone("testReadAsBytesSyncLargeFile");
+      return;
+    }
+    Expect.equals(largeFileSize, bytes.length);
+    Expect.listEquals(largeFileLastBytes,
+        Uint8List.sublistView(bytes, bytes.length - largeFileLastBytes.length));
+    asyncTestDone("testReadAsBytesSyncLargeFile");
+  }
+
   static void testReadAsText() {
     asyncTestStarted();
     var name = getFilename("fixed_length_file");
@@ -1613,7 +1680,8 @@ class FileTest {
 
     var absFile = file.absolute;
     Expect.isTrue(absFile.isAbsolute);
-    Expect.isTrue(absFile.path.startsWith(tempDirectory.path));
+    Expect.isTrue(absFile.path.startsWith(tempDirectory.path),
+        '${absFile.path} not in ${tempDirectory.path}');
 
     Expect.equals("content", absFile.readAsStringSync());
 
@@ -1730,7 +1798,11 @@ class FileTest {
       testSetLastAccessedSyncDirectory();
       testDoubleAsyncOperation();
       testAbsolute();
-      asyncEnd();
+      createLargeFile(() {
+        testReadAsBytesLargeFile();
+        testReadAsBytesSyncLargeFile();
+        asyncEnd();
+      });
     });
   }
 }