Prevent unsafe narrowing: base/files

Bug: 1292951
Change-Id: Ie7489503b41003ec3f4ec97991b39e4d82009c58
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3691384
Reviewed-by: danakj <danakj@chromium.org>
Owners-Override: danakj <danakj@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1011590}

base/files/file_proxy.cc

@@ -95,7 +95,7 @@ class CreateOrOpenHelper : public FileHelper {
   CreateOrOpenHelper(const CreateOrOpenHelper&) = delete;
   CreateOrOpenHelper& operator=(const CreateOrOpenHelper&) = delete;
 
-  void RunWork(const FilePath& file_path, int file_flags) {
+  void RunWork(const FilePath& file_path, uint32_t file_flags) {
     file_.Initialize(file_path, file_flags);
     error_ = file_.IsValid() ? File::FILE_OK : file_.error_details();
   }
@@ -175,7 +175,7 @@ class ReadHelper : public FileHelper {
  public:
   ReadHelper(FileProxy* proxy, File file, int bytes_to_read)
       : FileHelper(proxy, std::move(file)),
-        buffer_(new char[bytes_to_read]),
+        buffer_(new char[static_cast<size_t>(bytes_to_read)]),
         bytes_to_read_(bytes_to_read) {}
   ReadHelper(const ReadHelper&) = delete;
   ReadHelper& operator=(const ReadHelper&) = delete;
@@ -204,9 +204,9 @@ class WriteHelper : public FileHelper {
               const char* buffer,
               int bytes_to_write)
       : FileHelper(proxy, std::move(file)),
-        buffer_(new char[bytes_to_write]),
+        buffer_(new char[static_cast<size_t>(bytes_to_write)]),
         bytes_to_write_(bytes_to_write) {
-    memcpy(buffer_.get(), buffer, bytes_to_write);
+    memcpy(buffer_.get(), buffer, static_cast<size_t>(bytes_to_write));
   }
   WriteHelper(const WriteHelper&) = delete;
   WriteHelper& operator=(const WriteHelper&) = delete;
@@ -239,7 +239,7 @@ FileProxy::~FileProxy() {
 }
 
 bool FileProxy::CreateOrOpen(const FilePath& file_path,
-                             int file_flags,
+                             uint32_t file_flags,
                              StatusCallback callback) {
   DCHECK(!file_.IsValid());
   CreateOrOpenHelper* helper = new CreateOrOpenHelper(this, File());

base/files/file_proxy.h

@@ -59,7 +59,7 @@ class BASE_EXPORT FileProxy : public SupportsWeakPtr<FileProxy> {
   //
   // This returns false if task posting to |task_runner| has failed.
   bool CreateOrOpen(const FilePath& file_path,
-                    int file_flags,
+                    uint32_t file_flags,
                     StatusCallback callback);
 
   // Creates a temporary file for writing. The path and an open file are

base/files/file_util.cc

@@ -101,7 +101,8 @@ bool CopyFileContents(File& infile, File& outfile) {
   std::vector<char> buffer(kBufferSize);
 
   for (;;) {
-    int bytes_read = infile.ReadAtCurrentPos(buffer.data(), buffer.size());
+    int bytes_read =
+        infile.ReadAtCurrentPos(buffer.data(), static_cast<int>(buffer.size()));
     if (bytes_read < 0) {
       return false;
     }
@@ -112,7 +113,8 @@ bool CopyFileContents(File& infile, File& outfile) {
     int bytes_written_per_read = 0;
     do {
       int bytes_written_partial = outfile.WriteAtCurrentPos(
-          &buffer[bytes_written_per_read], bytes_read - bytes_written_per_read);
+          &buffer[static_cast<size_t>(bytes_written_per_read)],
+          bytes_read - bytes_written_per_read);
       if (bytes_written_partial < 0) {
         return false;
       }
@@ -228,34 +230,34 @@ bool ReadStreamToStringWithMaxSize(FILE* stream,
   // Many files have incorrect size (proc files etc). Hence, the file is read
   // sequentially as opposed to a one-shot read, using file size as a hint for
   // chunk size if available.
-  constexpr int64_t kDefaultChunkSize = 1 << 16;
-  int64_t chunk_size = kDefaultChunkSize - 1;
+  constexpr size_t kDefaultChunkSize = 1 << 16;
+  size_t chunk_size = kDefaultChunkSize - 1;
   ScopedBlockingCall scoped_blocking_call(FROM_HERE, BlockingType::MAY_BLOCK);
 #if BUILDFLAG(IS_WIN)
   BY_HANDLE_FILE_INFORMATION file_info = {};
   if (::GetFileInformationByHandle(
           reinterpret_cast<HANDLE>(_get_osfhandle(_fileno(stream))),
           &file_info)) {
     LARGE_INTEGER size;
-    size.HighPart = file_info.nFileSizeHigh;
+    size.HighPart = static_cast<LONG>(file_info.nFileSizeHigh);
     size.LowPart = file_info.nFileSizeLow;
     if (size.QuadPart > 0)
-      chunk_size = size.QuadPart;
+      chunk_size = static_cast<size_t>(size.QuadPart);
   }
 #else   // BUILDFLAG(IS_WIN)
   // In cases where the reported file size is 0, use a smaller chunk size to
   // minimize memory allocated and cost of string::resize() in case the read
   // size is small (i.e. proc files). If the file is larger than this, the read
   // loop will reset |chunk_size| to kDefaultChunkSize.
-  constexpr int64_t kSmallChunkSize = 4096;
+  constexpr size_t kSmallChunkSize = 4096;
   chunk_size = kSmallChunkSize - 1;
   stat_wrapper_t file_info = {};
   if (!File::Fstat(fileno(stream), &file_info) && file_info.st_size > 0)
-    chunk_size = file_info.st_size;
+    chunk_size = static_cast<size_t>(file_info.st_size);
 #endif  // BUILDFLAG(IS_WIN)
   // We need to attempt to read at EOF for feof flag to be set so here we
   // use |chunk_size| + 1.
-  chunk_size = std::min<uint64_t>(chunk_size, max_size) + 1;
+  chunk_size = std::min(chunk_size, max_size) + 1;
   size_t bytes_read_this_pass;
   size_t bytes_read_so_far = 0;
   bool read_status = true;
@@ -345,7 +347,7 @@ bool GetFileSize(const FilePath& file_path, int64_t* file_size) {
 bool TouchFile(const FilePath& path,
                const Time& last_accessed,
                const Time& last_modified) {
-  int flags = File::FLAG_OPEN | File::FLAG_WRITE_ATTRIBUTES;
+  uint32_t flags = File::FLAG_OPEN | File::FLAG_WRITE_ATTRIBUTES;
 
 #if BUILDFLAG(IS_WIN)
   // On Windows, FILE_FLAG_BACKUP_SEMANTICS is needed to open a directory.

base/files/file_util_win.cc

@@ -88,7 +88,7 @@ DWORD DeleteFileRecursive(const FilePath& path,
         (recursive || !info.IsDirectory())) {
       ::SetFileAttributes(
           current.value().c_str(),
-          info.find_data().dwFileAttributes & ~FILE_ATTRIBUTE_READONLY);
+          info.find_data().dwFileAttributes & ~DWORD{FILE_ATTRIBUTE_READONLY});
     }
 
     DWORD this_result = ERROR_SUCCESS;
@@ -151,7 +151,7 @@ bool DoCopyFile(const FilePath& from_path,
     return false;
   }
   if (attrs & FILE_ATTRIBUTE_READONLY) {
-    SetFileAttributes(dest, attrs & ~FILE_ATTRIBUTE_READONLY);
+    SetFileAttributes(dest, attrs & ~DWORD{FILE_ATTRIBUTE_READONLY});
   }
   return true;
 }
@@ -274,7 +274,7 @@ DWORD DoDeleteFile(const FilePath& path, bool recursive) {
   // Clear the read-only bit if it is set.
   if ((attr & FILE_ATTRIBUTE_READONLY) &&
       !::SetFileAttributes(path.value().c_str(),
-                           attr & ~FILE_ATTRIBUTE_READONLY)) {
+                           attr & ~DWORD{FILE_ATTRIBUTE_READONLY})) {
     // It's possible for |path| to be gone now under a race with other deleters.
     return ReturnLastErrorOrSuccessOnNotFound();
   }
@@ -812,7 +812,9 @@ bool GetFileInfo(const FilePath& file_path, File::Info* results) {
   ULARGE_INTEGER size;
   size.HighPart = attr.nFileSizeHigh;
   size.LowPart = attr.nFileSizeLow;
-  results->size = size.QuadPart;
+  // TODO(crbug.com/1333521): Change Info::size to uint64_t and eliminate this
+  // cast.
+  results->size = checked_cast<int64_t>(size.QuadPart);
 
   results->is_directory =
       (attr.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0;
@@ -879,12 +881,15 @@ int ReadFile(const FilePath& filename, char* data, int max_size) {
                                     FILE_SHARE_READ | FILE_SHARE_WRITE, NULL,
                                     OPEN_EXISTING, FILE_FLAG_SEQUENTIAL_SCAN,
                                     NULL));
-  if (!file.is_valid())
+  if (!file.is_valid() || max_size < 0)
     return -1;
 
   DWORD read;
-  if (::ReadFile(file.get(), data, max_size, &read, NULL))
-    return read;
+  if (::ReadFile(file.get(), data, static_cast<DWORD>(max_size), &read, NULL)) {
+    // TODO(crbug.com/1333521): Change to return some type with a uint64_t size
+    // and eliminate this cast.
+    return checked_cast<int>(read);
+  }
 
   return -1;
 }
@@ -894,15 +899,16 @@ int WriteFile(const FilePath& filename, const char* data, int size) {
   win::ScopedHandle file(CreateFile(filename.value().c_str(), GENERIC_WRITE, 0,
                                     NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL,
                                     NULL));
-  if (!file.is_valid()) {
-    DPLOG(WARNING) << "CreateFile failed for path " << filename.value();
+  if (!file.is_valid() || size < 0) {
+    DPLOG(WARNING) << "WriteFile failed for path " << filename.value();
     return -1;
   }
 
   DWORD written;
-  BOOL result = ::WriteFile(file.get(), data, size, &written, NULL);
+  BOOL result =
+      ::WriteFile(file.get(), data, static_cast<DWORD>(size), &written, NULL);
   if (result && static_cast<int>(written) == size)
-    return written;
+    return static_cast<int>(written);
 
   if (!result) {
     // WriteFile failed.
@@ -995,7 +1001,8 @@ bool CopyFile(const FilePath& from_path, const FilePath& to_path) {
 
 bool SetNonBlocking(int fd) {
   unsigned long nonblocking = 1;
-  if (ioctlsocket(fd, FIONBIO, &nonblocking) == 0)
+  if (ioctlsocket(static_cast<SOCKET>(fd), static_cast<long>(FIONBIO),
+                  &nonblocking) == 0)
     return true;
   return false;
 }

base/files/file_win.cc

@@ -62,21 +62,25 @@ int File::Read(int64_t offset, char* data, int size) {
   ScopedBlockingCall scoped_blocking_call(FROM_HERE, BlockingType::MAY_BLOCK);
   DCHECK(IsValid());
   DCHECK(!async_);
-  if (size < 0)
+  if (size < 0 || offset < 0)
     return -1;
 
   SCOPED_FILE_TRACE_WITH_SIZE("Read", size);
 
-  LARGE_INTEGER offset_li;
-  offset_li.QuadPart = offset;
+  ULARGE_INTEGER offset_li;
+  offset_li.QuadPart = static_cast<uint64_t>(offset);
 
   OVERLAPPED overlapped = {};
   overlapped.Offset = offset_li.LowPart;
   overlapped.OffsetHigh = offset_li.HighPart;
 
   DWORD bytes_read;
-  if (::ReadFile(file_.get(), data, size, &bytes_read, &overlapped))
-    return bytes_read;
+  if (::ReadFile(file_.get(), data, static_cast<DWORD>(size), &bytes_read,
+                 &overlapped)) {
+    // TODO(crbug.com/1333521): Change to return some type with a uint64_t size
+    // and eliminate this cast.
+    return checked_cast<int>(bytes_read);
+  }
   if (ERROR_HANDLE_EOF == GetLastError())
     return 0;
 
@@ -93,8 +97,12 @@ int File::ReadAtCurrentPos(char* data, int size) {
   SCOPED_FILE_TRACE_WITH_SIZE("ReadAtCurrentPos", size);
 
   DWORD bytes_read;
-  if (::ReadFile(file_.get(), data, size, &bytes_read, NULL))
-    return bytes_read;
+  if (::ReadFile(file_.get(), data, static_cast<DWORD>(size), &bytes_read,
+                 NULL)) {
+    // TODO(crbug.com/1333521): Change to return some type with a uint64_t size
+    // and eliminate this cast.
+    return checked_cast<int>(bytes_read);
+  }
   if (ERROR_HANDLE_EOF == GetLastError())
     return 0;
 
@@ -115,19 +123,22 @@ int File::Write(int64_t offset, const char* data, int size) {
   ScopedBlockingCall scoped_blocking_call(FROM_HERE, BlockingType::MAY_BLOCK);
   DCHECK(IsValid());
   DCHECK(!async_);
+  if (size < 0 || offset < 0)
+    return -1;
 
   SCOPED_FILE_TRACE_WITH_SIZE("Write", size);
 
-  LARGE_INTEGER offset_li;
-  offset_li.QuadPart = offset;
+  ULARGE_INTEGER offset_li;
+  offset_li.QuadPart = static_cast<uint64_t>(offset);
 
   OVERLAPPED overlapped = {};
   overlapped.Offset = offset_li.LowPart;
   overlapped.OffsetHigh = offset_li.HighPart;
 
   DWORD bytes_written;
-  if (::WriteFile(file_.get(), data, size, &bytes_written, &overlapped))
-    return bytes_written;
+  if (::WriteFile(file_.get(), data, static_cast<DWORD>(size), &bytes_written,
+                  &overlapped))
+    return static_cast<int>(bytes_written);
 
   return -1;
 }
@@ -142,8 +153,9 @@ int File::WriteAtCurrentPos(const char* data, int size) {
   SCOPED_FILE_TRACE_WITH_SIZE("WriteAtCurrentPos", size);
 
   DWORD bytes_written;
-  if (::WriteFile(file_.get(), data, size, &bytes_written, NULL))
-    return bytes_written;
+  if (::WriteFile(file_.get(), data, static_cast<DWORD>(size), &bytes_written,
+                  NULL))
+    return static_cast<int>(bytes_written);
 
   return -1;
 }
@@ -218,10 +230,12 @@ bool File::GetInfo(Info* info) {
   if (!GetFileInformationByHandle(file_.get(), &file_info))
     return false;
 
-  LARGE_INTEGER size;
+  ULARGE_INTEGER size;
   size.HighPart = file_info.nFileSizeHigh;
   size.LowPart = file_info.nFileSizeLow;
-  info->size = size.QuadPart;
+  // TODO(crbug.com/1333521): Change Info::size to uint64_t and eliminate this
+  // cast.
+  info->size = checked_cast<int64_t>(size.QuadPart);
   info->is_directory =
       (file_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0;
   info->is_symbolic_link = false;  // Windows doesn't have symbolic links.
@@ -341,7 +355,8 @@ File::Error File::OSErrorToFileError(DWORD last_error) {
     case ERROR_DISK_CORRUPT:  // The disk structure is corrupted and unreadable.
       return FILE_ERROR_IO;
     default:
-      UmaHistogramSparse("PlatformFile.UnknownErrors.Windows", last_error);
+      UmaHistogramSparse("PlatformFile.UnknownErrors.Windows",
+                         static_cast<int>(last_error));
       // This function should only be called for errors.
       DCHECK_NE(static_cast<DWORD>(ERROR_SUCCESS), last_error);
       return FILE_ERROR_FAILED;

base/files/important_file_writer.cc

@@ -193,7 +193,8 @@ bool ImportantFileWriter::WriteFileAtomicallyImpl(const FilePath& path,
   int bytes_written = 0;
   for (const char *scan = data.data(), *const end = scan + data.length();
        scan < end; scan += bytes_written) {
-    const int write_amount = std::min(kMaxWriteAmount, end - scan);
+    const int write_amount =
+        static_cast<int>(std::min(kMaxWriteAmount, end - scan));
     bytes_written = tmp_file.WriteAtCurrentPos(scan, write_amount);
     if (bytes_written != write_amount) {
       DPLOG(WARNING) << "Failed to write " << write_amount << " bytes to temp "

base/files/memory_mapped_file.cc

@@ -134,11 +134,13 @@ void MemoryMappedFile::CalculateVMAlignedBoundaries(int64_t start,
                                                     size_t* aligned_size,
                                                     int32_t* offset) {
   // Sadly, on Windows, the mmap alignment is not just equal to the page size.
-  auto mask = SysInfo::VMAllocationGranularity() - 1;
+  uint64_t mask = SysInfo::VMAllocationGranularity() - 1;
   DCHECK(IsValueInRangeForNumericType<int32_t>(mask));
-  *offset = start & mask;
-  *aligned_start = start & ~mask;
-  *aligned_size = (size + *offset + mask) & ~mask;
+  *offset = static_cast<int32_t>(static_cast<uint64_t>(start) & mask);
+  *aligned_start = static_cast<int64_t>(static_cast<uint64_t>(start) & ~mask);
+  // The DCHECK above means bit 31 is not set in `mask`, which in turn means
+  // *offset is positive.  Therefore this cast is safe.
+  *aligned_size = (size + static_cast<size_t>(*offset) + mask) & ~mask;
 }
 #endif  // !BUILDFLAG(IS_NACL)
 

base/files/memory_mapped_file_win.cc

@@ -12,6 +12,7 @@
 
 #include "base/files/file_path.h"
 #include "base/logging.h"
+#include "base/numerics/checked_math.h"
 #include "base/threading/scoped_blocking_call.h"
 #include "base/win/pe_image.h"
 
@@ -61,7 +62,7 @@ bool MemoryMappedFile::MapFileRegionToMemory(
   if (!file_.IsValid())
     return false;
 
-  int flags = 0;
+  DWORD flags = 0;
   ULARGE_INTEGER size = {};
   switch (access) {
     case READ_ONLY:
@@ -83,7 +84,7 @@ bool MemoryMappedFile::MapFileRegionToMemory(
   if (!file_mapping_.is_valid())
     return false;
 
-  LARGE_INTEGER map_start = {};
+  ULARGE_INTEGER map_start = {};
   SIZE_T map_size = 0;
   int32_t data_offset = 0;
 
@@ -105,17 +106,16 @@ bool MemoryMappedFile::MapFileRegionToMemory(
     size_t ignored = 0U;
     CalculateVMAlignedBoundaries(region.offset, region.size, &aligned_start,
                                  &ignored, &data_offset);
-    int64_t full_map_size = region.size + data_offset;
+    base::CheckedNumeric<SIZE_T> full_map_size = region.size;
+    full_map_size += data_offset;
 
     // Ensure that the casts below in the MapViewOfFile call are sane.
-    if (aligned_start < 0 || full_map_size < 0 ||
-        !IsValueInRangeForNumericType<SIZE_T>(
-            static_cast<uint64_t>(full_map_size))) {
+    if (aligned_start < 0 || !full_map_size.IsValid()) {
       DLOG(ERROR) << "Region bounds are not valid for MapViewOfFile";
       return false;
     }
-    map_start.QuadPart = aligned_start;
-    map_size = static_cast<SIZE_T>(full_map_size);
+    map_start.QuadPart = static_cast<uint64_t>(aligned_start);
+    map_size = full_map_size.ValueOrDie();
     length_ = region.size;
   }