Support read/write memory-mapped files.

base/files/memory_mapped_file_win.cc

Index: base/files/memory_mapped_file_win.cc
diff --git a/base/files/memory_mapped_file_win.cc b/base/files/memory_mapped_file_win.cc
index 5b397eed093e960827c4a4f1993e29b4001c6c91..84e09de20ba490051889c5533d16a07558d5cd2f 100644
--- a/base/files/memory_mapped_file_win.cc
+++ b/base/files/memory_mapped_file_win.cc
@@ -24,16 +24,32 @@ bool MemoryMappedFile::InitializeAsImageSection(const FilePath& file_name) {
 }
 
 bool MemoryMappedFile::MapFileRegionToMemory(
-    const MemoryMappedFile::Region& region) {
+    const MemoryMappedFile::Region& region,
+    Access access) {
   ThreadRestrictions::AssertIOAllowed();
 
   if (!file_.IsValid())
     return false;
 
-  int flags = image_ ? SEC_IMAGE | PAGE_READONLY : PAGE_READONLY;
+  int flags = image_ ? SEC_IMAGE : 0;

[forshaw, 2016/05/09 21:23:19]

While we're changing this I'd highly recommend we remove support for image
sections. As far as I can tell it's not used at present, is a Windows specific
addition and leads to security issues like
https://bugs.chromium.org/p/chromium/issues/detail?id=564238. 

[bcwhite, 2016/05/10 00:44:41]

Hmmm....  How about I work towards that in a different CL so as to not hold up
this one unnecessarily?

[forshaw, 2016/05/10 07:04:30]

Well we could do it in another CL, sure. I'd argue that especially in light of
the changes being made the support for image sections doesn't make any sense.
You can't have a "writable" image section, at the least passing PAGE_READONLY or
PAGE_READWRITE with SEC_IMAGE makes no difference to what actually gets mapped.
If I was being pedantic you could argue that the code should have some sort of
check (even a DCHECK) that ensures image section and writable are not being used
at the same time, but if we do that change it might just be simpler and reduce
code to just remove support (assuming no one else is using it, after all there's
not even a unit test for the image section code).

[bcwhite, 2016/05/10 21:00:54]

Since it's not actually in-use anywhere, I guess there's no delay by removing it
now.  Done.

+  uint32_t size_low = 0;
+  uint32_t size_high = 0;
+  switch (access) {
+    case READ_ONLY:
+      flags |= PAGE_READONLY;
+      break;
+    case READ_WRITE:
+      flags |= PAGE_READWRITE;
+      break;
+    case READ_WRITE_EXTEND:
+      flags |= PAGE_READWRITE;
+      size_high = static_cast<uint32_t>(region.size >> 32);
+      size_low = static_cast<uint32_t>(region.size & 0xFFFFFFFF);
+      break;
+  }
 
-  file_mapping_.Set(::CreateFileMapping(file_.GetPlatformFile(), NULL,
-                                        flags, 0, 0, NULL));
+  file_mapping_.Set(::CreateFileMapping(file_.GetPlatformFile(), NULL, flags,
+                                        size_high, size_low, NULL));

[erikchen, 2016/05/10 02:21:51]

A question, mostly for forshaw:

Should we be removing the access control permissions, and also giving the
FileMapping a name, as in https://codereview.chromium.org/1677163003

[forshaw, 2016/05/10 07:04:30]

I don't believe so, at least as far as I can tell there's no way of sharing a
memory mapped file, it's only for the purposes of mapping a file into the
current process. Certainly you could share the File object to another process
but the transport of PlatformFile instances should already be handled. Once at
the other side the section object would be created anew. We do hold a reference
to the file_mapping handle, but there's no method to extract it for transport.
This code also doesn't need to maintain that reference, it would be possible to
close the handle after calling MapViewOfFile and nothing would go wrong.

   if (!file_mapping_.IsValid())
     return false;
 
@@ -42,6 +58,7 @@ bool MemoryMappedFile::MapFileRegionToMemory(
   int32_t data_offset = 0;
 
   if (region == MemoryMappedFile::Region::kWholeFile) {
+    DCHECK_NE(READ_WRITE_EXTEND, access);
     int64_t file_len = file_.GetLength();
     if (file_len <= 0 || file_len > std::numeric_limits<int32_t>::max())
       return false;
@@ -72,8 +89,9 @@ bool MemoryMappedFile::MapFileRegionToMemory(
   }
 
   data_ = static_cast<uint8_t*>(
-      ::MapViewOfFile(file_mapping_.Get(), FILE_MAP_READ, map_start.HighPart,
-                      map_start.LowPart, map_size));
+      ::MapViewOfFile(file_mapping_.Get(),
+                      (flags & PAGE_READONLY) ? FILE_MAP_READ : FILE_MAP_WRITE,
+                      map_start.HighPart, map_start.LowPart, map_size));
   if (data_ == NULL)
     return false;
   data_ += data_offset;