Introduce safe forms of File::*NoBestEffort()

Have them merely route into the unsafe forms for the moment.

-- disentangle declarations of read vs. write methods.
-- Avoid temporary string creation in one place.

Change-Id: I0fefab2b25da4bd50603a6e6172419e24842b5fa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5813540
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Bo Liu <boliu@chromium.org>
Reviewed-by: Joe Downing <joedow@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Momoko Hattori <momohatt@chromium.org>
Reviewed-by: Francois Pierre Doray <fdoray@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Reviewed-by: Ben Reich <benreich@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1354829}

base/files/file.cc

@@ -163,6 +163,43 @@ bool File::WriteAtCurrentPosAndCheck(span<const uint8_t> data) {
   return WriteAtCurrentPos(data) == static_cast<int>(data.size());
 }
 
+std::optional<size_t> File::ReadNoBestEffort(int64_t offset,
+                                             base::span<uint8_t> data) {
+  span<char> chars = base::as_writable_chars(data);
+  int size = checked_cast<int>(chars.size());
+  // SAFETY: `chars.size()` describes valid portion of `chars.data()`.
+  int result = UNSAFE_BUFFERS(ReadNoBestEffort(offset, chars.data(), size));
+  if (result < 0) {
+    return std::nullopt;
+  }
+  return checked_cast<size_t>(result);
+}
+
+std::optional<size_t> File::ReadAtCurrentPosNoBestEffort(
+    base::span<uint8_t> data) {
+  span<char> chars = base::as_writable_chars(data);
+  int size = checked_cast<int>(chars.size());
+  // SAFETY: `chars.size()` describes valid portion of `chars.data()`.
+  int result = UNSAFE_BUFFERS(ReadAtCurrentPosNoBestEffort(chars.data(), size));
+  if (result < 0) {
+    return std::nullopt;
+  }
+  return checked_cast<size_t>(result);
+}
+
+std::optional<size_t> File::WriteAtCurrentPosNoBestEffort(
+    base::span<const uint8_t> data) {
+  span<const char> chars = base::as_chars(data);
+  int size = checked_cast<int>(chars.size());
+  // SAFETY: `chars.size()` describes valid portion of `chars.data()`.
+  int result =
+      UNSAFE_BUFFERS(WriteAtCurrentPosNoBestEffort(chars.data(), size));
+  if (result < 0) {
+    return std::nullopt;
+  }
+  return checked_cast<size_t>(result);
+}
+
 // static
 std::string File::ErrorToString(Error error) {
   switch (error) {

base/files/file.h

@@ -207,13 +207,11 @@ class BASE_EXPORT File {
   // (relative to the start) or -1 in case of error.
   int64_t Seek(Whence whence, int64_t offset);
 
-  // Simplified versions of Read() and friends (see below) that check the int
+  // Simplified versions of Read() and friends (see below) that check the
   // return value and just return a boolean. They return true if and only if
-  // the function read in / wrote out exactly |data.size()| bytes of data.
+  // the function read in exactly |data.size()| bytes of data.
   bool ReadAndCheck(int64_t offset, span<uint8_t> data);
   bool ReadAtCurrentPosAndCheck(span<uint8_t> data);
-  bool WriteAndCheck(int64_t offset, span<const uint8_t> data);
-  bool WriteAtCurrentPosAndCheck(span<const uint8_t> data);
 
   // Reads the given number of bytes (or until EOF is reached) starting with the
   // given offset. Returns the number of bytes read, or -1 on error. Note that
@@ -230,13 +228,22 @@ class BASE_EXPORT File {
 
   // Reads the given number of bytes (or until EOF is reached) starting with the
   // given offset, but does not make any effort to read all data on all
-  // platforms. Returns the number of bytes read, or -1 on error.
+  // platforms. Returns the number of bytes read, or -1/std::nullopt on error.
   UNSAFE_BUFFER_USAGE int ReadNoBestEffort(int64_t offset,
                                            char* data,
                                            int size);
+  std::optional<size_t> ReadNoBestEffort(int64_t offset,
+                                         base::span<uint8_t> data);
 
   // Same as above but without seek.
   UNSAFE_BUFFER_USAGE int ReadAtCurrentPosNoBestEffort(char* data, int size);
+  std::optional<size_t> ReadAtCurrentPosNoBestEffort(base::span<uint8_t> data);
+
+  // Simplified versions of Write() and friends (see below) that check the
+  // return value and just return a boolean. They return true if and only if
+  // the function wrote out exactly |data.size()| bytes of data.
+  bool WriteAndCheck(int64_t offset, span<const uint8_t> data);
+  bool WriteAtCurrentPosAndCheck(span<const uint8_t> data);
 
   // Writes the given buffer into the file at the given offset, overwritting any
   // data that was previously there. Returns the number of bytes written, or -1
@@ -252,9 +259,12 @@ class BASE_EXPORT File {
   std::optional<size_t> WriteAtCurrentPos(base::span<const uint8_t> data);
 
   // Save as above but does not make any effort to write all data on all
-  // platforms. Returns the number of bytes written, or -1 on error.
+  // platforms. Returns the number of bytes written, or -1/std::nullopt
+  // on error.
   UNSAFE_BUFFER_USAGE int WriteAtCurrentPosNoBestEffort(const char* data,
                                                         int size);
+  std::optional<size_t> WriteAtCurrentPosNoBestEffort(
+      base::span<const uint8_t> data);
 
   // Returns the current size of this file, or a negative number on failure.
   int64_t GetLength() const;

base/files/file_unittest.cc

@@ -227,10 +227,14 @@ TEST(FileTest, ReadWrite) {
     EXPECT_EQ(data_to_write[i], data_read_1[i]);
 
   // Read again, but using the trivial native wrapper.
-  bytes_read = file.ReadNoBestEffort(0, data_read_1, kTestDataSize);
-  EXPECT_LE(bytes_read, kTestDataSize);
-  for (int i = 0; i < bytes_read; i++)
+  std::optional<size_t> maybe_bytes_read =
+      file.ReadNoBestEffort(0, base::as_writable_byte_span(data_read_1)
+                                   .first(static_cast<size_t>(kTestDataSize)));
+  ASSERT_TRUE(maybe_bytes_read.has_value());
+  EXPECT_LE(maybe_bytes_read.value(), static_cast<size_t>(kTestDataSize));
+  for (size_t i = 0; i < maybe_bytes_read.value(); i++) {
     EXPECT_EQ(data_to_write[i], data_read_1[i]);
+  }
 
   // Write past the end of the file.
   const int kOffsetBeyondEndOfFile = 10;

base/mac/launch_application_unittest.mm

@@ -9,7 +9,7 @@
 #include "base/apple/bridging.h"
 #include "base/apple/foundation_util.h"
 #include "base/base_paths.h"
-#include "base/compiler_specific.h"
+#include "base/containers/span.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
@@ -45,10 +45,11 @@ void ReadLaunchEventsFromFifo(
   std::string data;
   while (true) {
     char buf[4096];
-    int read_count =
-        UNSAFE_TODO(f.ReadAtCurrentPosNoBestEffort(buf, sizeof buf));
-    if (read_count) {
-      data += std::string(buf, read_count);
+    std::optional<size_t> read_count =
+        f.ReadAtCurrentPosNoBestEffort(base::as_writable_byte_span(buf));
+    ASSERT_TRUE(read_count.has_value());
+    if (read_count.value()) {
+      data += std::string_view(buf, read_count.value());
       // Assume that at any point the beginning of the data buffer is the start
       // of a plist. Search for the first end, and parse that substring.
       size_t end_of_plist;

chrome/browser/ash/arc/fileapi/arc_content_file_system_file_stream_reader.cc

@@ -17,6 +17,7 @@
 #include "base/files/file.h"
 #include "base/functional/bind.h"
 #include "base/functional/callback_helpers.h"
+#include "base/numerics/safe_conversions.h"
 #include "base/task/thread_pool.h"
 #include "base/threading/scoped_blocking_call.h"
 #include "chrome/browser/ash/arc/fileapi/arc_content_file_system_size_util.h"
@@ -31,10 +32,12 @@ namespace arc {
 namespace {
 
 // Calls base::File::ReadAtCurrentPosNoBestEffort with the given buffer.
-int ReadFile(base::File* file,
-             scoped_refptr<net::IOBuffer> buffer,
-             int buffer_length) {
-  return file->ReadAtCurrentPosNoBestEffort(buffer->data(), buffer_length);
+// Return the number of bytes read, or std::nullopt on error.
+std::optional<size_t> ReadFile(base::File* file,
+                               scoped_refptr<net::IOBuffer> buffer,
+                               int buffer_length) {
+  return file->ReadAtCurrentPosNoBestEffort(
+      buffer->span().first(base::checked_cast<size_t>(buffer_length)));
 }
 
 // Seeks the file, returns 0 on success, or errno on an error.
@@ -130,11 +133,14 @@ void ArcContentFileSystemFileStreamReader::ReadInternal(
 
 void ArcContentFileSystemFileStreamReader::OnRead(
     net::CompletionOnceCallback callback,
-    int result) {
+    std::optional<size_t> result) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-  if (result < 0)
+  if (!result.has_value()) {
     CloseInternal(CloseStatus::kStatusError);
-  std::move(callback).Run(result < 0 ? net::ERR_FAILED : result);
+    std::move(callback).Run(net::ERR_FAILED);
+    return;
+  }
+  std::move(callback).Run(result.value());
 }
 
 void ArcContentFileSystemFileStreamReader::OnGetFileSize(
@@ -257,16 +263,16 @@ void ArcContentFileSystemFileStreamReader::OnConsumeFileContents(
     net::CompletionOnceCallback callback,
     scoped_refptr<net::IOBufferWithSize> temporary_buffer,
     int64_t num_bytes_to_consume,
-    int read_result) {
+    std::optional<size_t> read_result) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-  if (read_result < 0) {
+  if (!read_result.has_value()) {
     LOG(ERROR) << "Failed to consume the file stream.";
     CloseInternal(CloseStatus::kStatusError);
     std::move(callback).Run(net::ERR_FAILED);
     return;
   }
-  DCHECK_GE(num_bytes_to_consume, read_result);
-  num_bytes_to_consume -= read_result;
+  DCHECK_GE(num_bytes_to_consume, static_cast<int64_t>(read_result.value()));
+  num_bytes_to_consume -= read_result.value();
   ConsumeFileContents(buf, buffer_length, std::move(callback), temporary_buffer,
                       num_bytes_to_consume);
 }

chrome/browser/ash/arc/fileapi/arc_content_file_system_file_stream_reader.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_ASH_ARC_FILEAPI_ARC_CONTENT_FILE_SYSTEM_FILE_STREAM_READER_H_
 
 #include <memory>
+#include <optional>
 #include <string>
 
 #include "ash/components/arc/mojom/file_system.mojom-forward.h"
@@ -58,7 +59,8 @@ class ArcContentFileSystemFileStreamReader : public storage::FileStreamReader {
                     net::CompletionOnceCallback callback);
 
   // Called when read completes.
-  void OnRead(net::CompletionOnceCallback callback, int result);
+  void OnRead(net::CompletionOnceCallback callback,
+              std::optional<size_t> result);
 
   // Called when GetFileSize() completes.
   void OnGetFileSize(net::Int64CompletionOnceCallback callback, int64_t size);
@@ -90,7 +92,7 @@ class ArcContentFileSystemFileStreamReader : public storage::FileStreamReader {
       net::CompletionOnceCallback callback,
       scoped_refptr<net::IOBufferWithSize> temporary_buffer,
       int64_t num_bytes_to_consume,
-      int read_result);
+      std::optional<size_t> read_result);
 
   GURL arc_url_;
   int64_t offset_;

chrome/browser/ash/arc/fileapi/arc_content_file_system_file_stream_writer.cc

@@ -17,6 +17,7 @@
 #include "base/files/file.h"
 #include "base/functional/callback_helpers.h"
 #include "base/logging.h"
+#include "base/numerics/safe_conversions.h"
 #include "base/task/thread_pool.h"
 #include "base/threading/scoped_blocking_call.h"
 #include "content/public/browser/browser_thread.h"
@@ -30,11 +31,12 @@ namespace arc {
 namespace {
 
 // Calls base::File::WriteAtCurrentPosNoBestEffort with the given buffer.
-// Returns the number of bytes written, or -1 on error.
-int WriteFile(base::File* file,
-              scoped_refptr<net::IOBuffer> buffer,
-              int buffer_length) {
-  return file->WriteAtCurrentPosNoBestEffort(buffer->data(), buffer_length);
+// Returns the number of bytes written, or std::nullopt on error.
+std::optional<size_t> WriteFile(base::File* file,
+                                scoped_refptr<net::IOBuffer> buffer,
+                                int buffer_length) {
+  return file->WriteAtCurrentPosNoBestEffort(
+      buffer->span().first(base::checked_cast<size_t>(buffer_length)));
 }
 
 // Seeks the file, returns 0 on success, or errno on an error.
@@ -156,18 +158,20 @@ void ArcContentFileSystemFileStreamWriter::WriteInternal(
 
 void ArcContentFileSystemFileStreamWriter::OnWrite(
     net::CompletionOnceCallback callback,
-    int result) {
+    std::optional<size_t> result) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   DCHECK(has_pending_operation_);
 
-  if (CancelIfRequested())
+  if (CancelIfRequested()) {
     return;
-
-  has_pending_operation_ = false;
-  if (result < 0) {
-    CloseInternal(CloseStatus::kStatusError);
   }
-  std::move(callback).Run(result < 0 ? net::ERR_FAILED : result);
+  has_pending_operation_ = false;
+  if (!result.has_value()) {
+    CloseInternal(CloseStatus::kStatusError);
+    std::move(callback).Run(net::ERR_FAILED);
+    return;
+  }
+  std::move(callback).Run(result.value());
 }
 
 void ArcContentFileSystemFileStreamWriter::OnOpenFileSession(

chrome/browser/ash/arc/fileapi/arc_content_file_system_file_stream_writer.h

@@ -8,6 +8,7 @@
 #include <stdint.h>
 
 #include <memory>
+#include <optional>
 #include <string>
 
 #include "ash/components/arc/mojom/file_system.mojom-forward.h"
@@ -61,7 +62,8 @@ class ArcContentFileSystemFileStreamWriter : public storage::FileStreamWriter {
                      net::CompletionOnceCallback callback);
 
   // Called when write completes.
-  void OnWrite(net::CompletionOnceCallback callback, int result);
+  void OnWrite(net::CompletionOnceCallback callback,
+               std::optional<size_t> result);
 
   // Called when opening file session completes.
   void OnOpenFileSession(scoped_refptr<net::IOBuffer> buf,

chrome/browser/ash/fileapi/fallback_copy_in_foreign_file.cc

@@ -14,6 +14,7 @@
 #include "base/files/file_error_or.h"
 #include "base/files/safe_base_name.h"
 #include "base/memory/raw_ref.h"
+#include "base/numerics/safe_conversions.h"
 #include "base/strings/strcat.h"
 #include "base/unguessable_token.h"
 #include "net/base/io_buffer.h"
@@ -227,12 +228,13 @@ void Copier::CallRead() {
   static constexpr auto read_file_off_the_io_thread =
       [](scoped_refptr<net::IOBuffer> buffer,
          base::File file) -> base::FileErrorOr<FileAndInt> {
-    int num_bytes_read =
-        file.ReadAtCurrentPosNoBestEffort(buffer->data(), kBufferSize);
-    if (num_bytes_read >= 0) {
-      return FileAndInt(std::move(file), num_bytes_read);
+    std::optional<size_t> num_bytes_read =
+        file.ReadAtCurrentPosNoBestEffort(buffer->span().first(kBufferSize));
+    if (!num_bytes_read.has_value()) {
+      return base::unexpected(base::File::GetLastFileError());
     }
-    return base::unexpected(base::File::GetLastFileError());
+    return FileAndInt(std::move(file),
+                      base::checked_cast<int>(num_bytes_read.value()));
   };
 
   scoped_refptr<net::IOBuffer> buffer = io_buffer_;

components/subresource_filter/core/browser/copying_file_stream.cc

@@ -5,6 +5,8 @@
 #include "components/subresource_filter/core/browser/copying_file_stream.h"
 
 #include "base/compiler_specific.h"
+#include "base/containers/span.h"
+#include "base/numerics/safe_conversions.h"
 
 namespace subresource_filter {
 
@@ -16,8 +18,11 @@ CopyingFileInputStream::CopyingFileInputStream(base::File file)
     : file_(std::move(file)) {}
 
 int CopyingFileInputStream::Read(void* buffer, int size) {
-  return UNSAFE_TODO(
-      file_.ReadAtCurrentPosNoBestEffort(static_cast<char*>(buffer), size));
+  std::optional<size_t> result =
+      file_.ReadAtCurrentPosNoBestEffort(UNSAFE_TODO(base::span(
+          static_cast<uint8_t*>(buffer), base::checked_cast<size_t>(size))));
+
+  return result.has_value() ? base::checked_cast<int>(result.value()) : -1;
 }
 
 // CopyingFileOutputStream -----------------------------------------------------

content/browser/devtools/devtools_stream_file.cc

@@ -120,19 +120,20 @@ DevToolsIOContext::Stream::Status DevToolsStreamFile::InnerReadOnFileSequence(
   max_size =
       std::min(max_size, static_cast<size_t>(last_written_pos_ - position));
   buffer.resize(max_size);
-  int size_got =
-      UNSAFE_TODO(file_.ReadNoBestEffort(position, &*buffer.begin(), max_size));
 
-  if (size_got < 0) {
+  std::optional<size_t> size_got =
+      file_.ReadNoBestEffort(position, base::as_writable_byte_span(buffer));
+
+  if (!size_got.has_value()) {
     LOG(ERROR) << "Failed to read temporary file";
     return StatusFailure;
   }
 
   // Provided client has requested sufficient large block, make their
   // life easier by not truncating in the middle of a UTF-8 character.
-  if (size_got > 6 && !CBU8_IS_SINGLE(buffer[size_got - 1])) {
+  if (size_got.value() > 6 && !CBU8_IS_SINGLE(buffer[size_got.value() - 1])) {
     std::string truncated;
-    base::TruncateUTF8ToByteSize(buffer, size_got, &truncated);
+    base::TruncateUTF8ToByteSize(buffer, size_got.value(), &truncated);
     // If the above failed, we're dealing with binary files, so
     // don't mess with them.
     if (truncated.size()) {
@@ -140,8 +141,8 @@ DevToolsIOContext::Stream::Status DevToolsStreamFile::InnerReadOnFileSequence(
       size_got = buffer.size();
     }
   }
-  buffer.resize(size_got);
-  last_read_pos_ = position + size_got;
+  buffer.resize(size_got.value());
+  last_read_pos_ = position + size_got.value();
   if (binary_) {
     buffer = base::Base64Encode(buffer);
   }

net/base/file_stream_context_posix.cc

@@ -5,6 +5,8 @@
 #include "net/base/file_stream_context.h"
 
 #include <errno.h>
+
+#include <optional>
 #include <utility>
 
 #include "base/check.h"
@@ -13,6 +15,7 @@
 #include "base/functional/callback.h"
 #include "base/functional/callback_helpers.h"
 #include "base/location.h"
+#include "base/numerics/safe_conversions.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/task/task_runner.h"
 #include "net/base/io_buffer.h"
@@ -96,23 +99,23 @@ void FileStream::Context::OnFileOpened() {
 FileStream::Context::IOResult FileStream::Context::ReadFileImpl(
     scoped_refptr<IOBuffer> buf,
     int buf_len) {
-  int res =
-      UNSAFE_TODO(file_.ReadAtCurrentPosNoBestEffort(buf->data(), buf_len));
-  if (res == -1)
+  std::optional<size_t> res = file_.ReadAtCurrentPosNoBestEffort(
+      buf->span().first(base::checked_cast<size_t>(buf_len)));
+  if (!res.has_value()) {
     return IOResult::FromOSError(errno);
-
-  return IOResult(res, 0);
+  }
+  return IOResult(res.value(), 0);
 }
 
 FileStream::Context::IOResult FileStream::Context::WriteFileImpl(
     scoped_refptr<IOBuffer> buf,
     int buf_len) {
-  int res =
-      UNSAFE_TODO(file_.WriteAtCurrentPosNoBestEffort(buf->data(), buf_len));
-  if (res == -1)
+  std::optional<size_t> res = file_.WriteAtCurrentPosNoBestEffort(
+      buf->span().first(base::checked_cast<size_t>(buf_len)));
+  if (!res.has_value()) {
     return IOResult::FromOSError(errno);
-
-  return IOResult(res, 0);
+  }
+  return IOResult(res.value(), 0);
 }
 
 }  // namespace net

remoting/host/security_key/security_key_message_reader_impl.cc

@@ -13,6 +13,7 @@
 #include <string>
 #include <utility>
 
+#include "base/compiler_specific.h"
 #include "base/files/file.h"
 #include "base/functional/bind.h"
 #include "base/logging.h"
@@ -104,23 +105,21 @@ bool SecurityKeyMessageReaderImpl::ReadFromStream(char* buffer,
                                                   size_t bytes_to_read) {
   DCHECK(buffer);
   DCHECK_GT(bytes_to_read, 0u);
-
-  size_t bytes_read = 0;
+  base::span<uint8_t> buffer_span =
+      base::as_writable_bytes(UNSAFE_TODO(base::span(buffer, bytes_to_read)));
   do {
-    int read_result = read_stream_.ReadAtCurrentPosNoBestEffort(
-        buffer + bytes_read, bytes_to_read - bytes_read);
-    if (read_result < 1) {
-      // 0 means EOF which is normal and should not be logged as an error.
-      if (read_result != 0) {
-        LOG(ERROR) << "Failed to read from stream, ReadAtCurrentPos returned "
-                   << read_result;
-      }
+    std::optional<size_t> read_result =
+        read_stream_.ReadAtCurrentPosNoBestEffort(buffer_span);
+    if (!read_result.has_value()) {
+      LOG(ERROR) << "Failed to read from stream, ReadAtCurrentPos failed";
       return false;
     }
-    bytes_read += read_result;
-  } while (bytes_read < bytes_to_read);
-  DCHECK_EQ(bytes_read, bytes_to_read);
-
+    if (*read_result == 0) {
+      // 0 means EOF which is normal and should not be logged as an error.
+      return false;
+    }
+    buffer_span = buffer_span.subspan(*read_result);
+  } while (!buffer_span.empty());
   return true;
 }
 

third_party/leveldatabase/env_chromium.cc

@@ -12,6 +12,8 @@
 #include <utility>
 
 #include "base/check_op.h"
+#include "base/compiler_specific.h"
+#include "base/containers/span.h"
 #include "base/files/file_error_or.h"
 #include "base/files/file_util.h"
 #include "base/format_macros.h"
@@ -136,13 +138,14 @@ class ChromiumSequentialFile : public leveldb::SequentialFile {
   // Note: This method is relatively hot during leveldb database
   // compaction. Please avoid making them slower.
   Status Read(size_t n, Slice* result, char* scratch) override {
-    int bytes_read = file_.ReadAtCurrentPosNoBestEffort(scratch, n);
-    if (bytes_read == -1) {
+    std::optional<size_t> bytes_read = file_.ReadAtCurrentPosNoBestEffort(
+        base::as_writable_bytes(UNSAFE_TODO(base::span(scratch, n))));
+    if (!bytes_read.has_value()) {
       base::File::Error error = base::File::GetLastFileError();
       return MakeIOError(filename_, base::File::ErrorToString(error),
                          kSequentialFileRead, error);
     }
-    *result = Slice(scratch, bytes_read);
+    *result = Slice(scratch, *bytes_read);
     return Status::OK();
   }