Update ReadProcMaps() to reflect lack of atomicity when reading /proc/self/maps.

base/debug/proc_maps_linux.cc

 // Copyright (c) 2013 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/debug/proc_maps_linux.h"
 
 #if defined(OS_LINUX)
 #include <inttypes.h>
 #endif
 
 #include "base/file_util.h"
 #include "base/strings/string_split.h"
 
 #if defined(OS_ANDROID)
 // Bionic's inttypes.h defines PRI/SCNxPTR as an unsigned long int, which
 // is incompatible with Bionic's stdint.h defining uintptr_t as a unsigned int:
 // https://code.google.com/p/android/issues/detail?id=57218
 #undef SCNxPTR
 #define SCNxPTR "x"
 #endif
 
 namespace base {
 namespace debug {
 
+// Local testing revealed that the size of /proc/<pid>/maps for an official
+// release build of chrome hovered around the ~45 KB mark with some processes
+// hitting 80-90 KB. We'll play it safe and use 256 KB.
+enum { kLargeProcMapsSize = 256 * 1024 };

[Mark Mentovai, 2013/07/11 03:47:50]

Why an enum? Why not a const?

And why is it out here? You can put it in ReadProcMaps, since that’s the only
place it’s used.

+
 bool ReadProcMaps(std::string* proc_maps) {
+  // It's possible for /proc/self/maps to change during reading (e.g., when
+  // running under ThreadSanitizer). Resize |proc_maps| to be large enough
+  // to read without causing a reallocation.
+  //
+  // Refer to http://crbug.com/258451 for details.
+  proc_maps->clear();
+  proc_maps->reserve(kLargeProcMapsSize);

[Mark Mentovai, 2013/07/11 03:47:50]

How is this supposed to help…?

+
   FilePath proc_maps_path("/proc/self/maps");
   return file_util::ReadFileToString(proc_maps_path, proc_maps);

[Mark Mentovai, 2013/07/11 03:47:50]

…file_util::ReadFileToString still reads chunks of at most 64kB at a time. It
doesn’t care about the size of the “contents” buffer. All the “reserve” does is
make sure that the std::string::append calls don’t result in internal
reallocations until at least 256kB has been read.

It’s not enough to avoid reallocations, you really need to gobble up the entire
contents of the file in a single read operation.

[Alexander Potapenko, 2013/07/11 10:28:54]

Maybe base::MemoryMappedFile could be used instead?
This is still non-atomic, because the size of /proc/self/maps may change between
the fstat() and mmap() calls, so we may need to pass some default size to
MapFileToMemoryInternal.

[Mark Mentovai, 2013/07/11 14:20:40]

Alexander Potapenko wrote:
/proc is not a regular filesystem. There are two problems with this approach.

1. You can’t use stat (or fstat) to figure out how big the file is, because
there is no file. The kernel won’t assemble any data until you try to read the
file, so it doesn’t even know the answer. st_size will always be 0 for this
file, and for most or all other files in /proc.

2. You can’t mmap the file, because it doesn’t implement mmap.
(fs/proc/task_mmu.c proc_tid_maps_operations). You’ll get ENODEV. I don’t know
what semantics you would expect of mmap, anyway, since this isn’t a real file.

 }
 
 bool ParseProcMaps(const std::string& input,
                    std::vector<MappedMemoryRegion>* regions_out) {
   std::vector<MappedMemoryRegion> regions;
 
   // This isn't async safe nor terribly efficient, but it doesn't need to be at
   // this point in time.
   std::vector<std::string> lines;
   SplitString(input, '\n', &lines);
@@ skipping 54 matching lines @@
     regions.push_back(region);
     regions.back().path.assign(line + path_index);
   }
 
   regions_out->swap(regions);
   return true;
 }
 
 }  // namespace debug
 }  // namespace base