base: Avoid calling VirtualAlloc with 0 length.

base/memory/discardable_shared_memory.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/memory/discardable_shared_memory.h"
 
 #include <stdint.h>
 
 #include <algorithm>
 
@@ skipping 184 matching lines @@
     if (result.value.u != old_state.value.u) {
       // Update |last_known_usage_| in case the above CAS failed because of
       // an incorrect timestamp.
       last_known_usage_ = result.GetTimestamp();
       return FAILED;
     }
   }
 
   // Zero for length means "everything onward".
   if (!length)
     length = AlignToPageSize(mapped_size_) - offset;

[penny, 2016/02/28 18:44:00]

The only time length can be 0 is if the caller passes in offset == the very end
of the mapping, and length == 0 (i.e. "everything onward").

I think that would be a "bad" function call, and we should perhaps be returning
FAILED (and DCHECKing to try to catch any culprits in the wild?).

[reveman, 2016/02/28 20:36:16]

We've had DCHECKs for this at a higher level but they went unnoticed as DCHECKs
are not used in the wild. These DCHECKs are for that reason temporarily CHECKs
as I'd like to fix any code that allocates 0 bytes of discardable memory.
However, even if we fix the issues we have today I'm worried this might pop up
again in the future as it's not too crazy to expect 0 byte allocations to
succeed as that's valid for malloc.

So I'd like to have DCHECKs for 0 size allocations at a higher level but not
have the discardable code crash in the wild if this happens. Does that make
sense?

 
   size_t start = offset / base::GetPageSize();
   size_t end = start + length / base::GetPageSize();
   DCHECK_LE(start, end);
   DCHECK_LE(end, AlignToPageSize(mapped_size_) / base::GetPageSize());
 
   // Add pages to |locked_page_count_|.
   // Note: Locking a page that is already locked is an error.
   locked_page_count_ += end - start;
 #if DCHECK_IS_ON()
   // Detect incorrect usage by keeping track of exactly what pages are locked.
   for (auto page = start; page < end; ++page) {
     auto result = locked_pages_.insert(page);
     DCHECK(result.second);
   }
   DCHECK_EQ(locked_pages_.size(), locked_page_count_);
 #endif
 
 // Pin pages if supported.
 #if defined(OS_ANDROID)
   SharedMemoryHandle handle = shared_memory_.handle();
   if (SharedMemory::IsHandleValid(handle)) {
     if (ashmem_pin_region(
             handle.fd, AlignToPageSize(sizeof(SharedState)) + offset, length)) {
       return PURGED;
     }
   }
 #elif defined(OS_WIN)
   if (base::win::GetVersion() >= base::win::VERSION_WIN8) {
-    if (!VirtualAlloc(reinterpret_cast<char*>(shared_memory_.memory()) +
+    if (length &&

[penny, 2016/02/28 21:49:10]

if (!length)
  return PURGED;

Unlock is fine as is, but perhaps in Lock we should return PURGED in the rare
case we ever get a length of zero at this point.

This will just prevent the misbehaving caller from thinking the memory contents
of this mapping have been preserved (implied by SUCCESS), when there's a chance
the contents have in fact been lost and need to be recreated.

[reveman, 2016/02/29 16:36:59]

Done.

Note: memory segments are initially locked so you can still have a locked 0-byte
segment.

+        !VirtualAlloc(reinterpret_cast<char*>(shared_memory_.memory()) +
                           AlignToPageSize(sizeof(SharedState)) + offset,
                       length, MEM_RESET_UNDO, PAGE_READWRITE)) {
       return PURGED;
     }
   }
 #endif
 
   return SUCCESS;
 }
 
 void DiscardableSharedMemory::Unlock(size_t offset, size_t length) {
   DCHECK_EQ(AlignToPageSize(offset), offset);
   DCHECK_EQ(AlignToPageSize(length), length);
 
   // Calls to this function must be synchronized properly.
   DFAKE_SCOPED_LOCK(thread_collision_warner_);
 
   // Zero for length means "everything onward".
   if (!length)
     length = AlignToPageSize(mapped_size_) - offset;

[penny, 2016/02/28 18:44:00]

Here too.

 
   DCHECK(shared_memory_.memory());
 
 // Unpin pages if supported.
 #if defined(OS_ANDROID)
   SharedMemoryHandle handle = shared_memory_.handle();
   if (SharedMemory::IsHandleValid(handle)) {
     if (ashmem_unpin_region(
             handle.fd, AlignToPageSize(sizeof(SharedState)) + offset, length)) {
       DPLOG(ERROR) << "ashmem_unpin_region() failed";
     }
   }
 #elif defined(OS_WIN)
   if (base::win::GetVersion() >= base::win::VERSION_WIN8) {
     // Note: MEM_RESET is not technically gated on Win8.  However, this Unlock
     // function needs to match the Lock behaviour (MEM_RESET_UNDO) to properly
     // implement memory pinning.  It needs to bias towards preserving the
     // contents of memory between an Unlock and next Lock.
-    if (!VirtualAlloc(reinterpret_cast<char*>(shared_memory_.memory()) +
+    if (length &&
+        !VirtualAlloc(reinterpret_cast<char*>(shared_memory_.memory()) +
                           AlignToPageSize(sizeof(SharedState)) + offset,
                       length, MEM_RESET, PAGE_READWRITE)) {
       DPLOG(ERROR) << "VirtualAlloc() MEM_RESET failed in Unlock()";
     }
   }
 #endif
 
   size_t start = offset / base::GetPageSize();
   size_t end = start + length / base::GetPageSize();
   DCHECK_LE(start, end);
@@ skipping 125 matching lines @@
 
 void DiscardableSharedMemory::Close() {
   shared_memory_.Close();
 }
 
 Time DiscardableSharedMemory::Now() const {
   return Time::Now();
 }
 
 }  // namespace base