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

Reading from procfs returns at most a page-sized amount of data. If the process has a larger-than-page-sized /proc/self/maps, we cannot guarantee that the virtual memory table hasn't changed while reading the entire contents from procfs.

In addition, ReadProcMaps() now stops reading as soon as it finds a gate VMA entry to workaround a scenario where the kernel would return duplicate entries (it turns out ThreadSanitizer v2 was very good at triggering said scenario).

BUG=258451

Review URL: https://chromiumcodereview.appspot.com/18661009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@221570 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 105 insertions, 10 deletions

diff --git a/base/debug/proc_maps_linux.cc b/base/debug/proc_maps_linux.cc
index c9afd45..b7a5862 100644
--- a/base/debug/proc_maps_linux.cc
+++ b/base/debug/proc_maps_linux.cc
@@ -4,6 +4,8 @@
 
 #include "base/debug/proc_maps_linux.h"
 
+#include <fcntl.h>
+
 #if defined(OS_LINUX)
 #include <inttypes.h>
 #endif
@@ -22,9 +24,68 @@
 namespace base {
 namespace debug {
 
+// Scans |proc_maps| starting from |pos| returning true if the gate VMA was
+// found, otherwise returns false.
+static bool ContainsGateVMA(std::string* proc_maps, size_t pos) {
+#if defined(ARCH_CPU_ARM_FAMILY)
+  // The gate VMA on ARM kernels is the interrupt vectors page.
+  return proc_maps->find(" [vectors]\n", pos) != std::string::npos;
+#elif defined(ARCH_CPU_X86_64)
+  // The gate VMA on x86 64-bit kernels is the virtual system call page.
+  return proc_maps->find(" [vsyscall]\n", pos) != std::string::npos;
+#else
+  // Otherwise assume there is no gate VMA in which case we shouldn't
+  // get duplicate entires.
+  return false;
+#endif
+}
+
 bool ReadProcMaps(std::string* proc_maps) {
-  FilePath proc_maps_path("/proc/self/maps");
-  return ReadFileToString(proc_maps_path, proc_maps);
+  // seq_file only writes out a page-sized amount on each call. Refer to header
+  // file for details.
+  const long kReadSize = sysconf(_SC_PAGESIZE);
+
+  int fd = HANDLE_EINTR(open("/proc/self/maps", O_RDONLY));
+  if (fd == -1) {
+    DPLOG(ERROR) << "Couldn't open /proc/self/maps";
+    return false;
+  }
+  file_util::ScopedFD fd_closer(&fd);
+  proc_maps->clear();
+
+  while (true) {
+    // To avoid a copy, resize |proc_maps| so read() can write directly into it.
+    // Compute |buffer| afterwards since resize() may reallocate.
+    size_t pos = proc_maps->size();
+    proc_maps->resize(pos + kReadSize);
+    void* buffer = &(*proc_maps)[pos];
+
+    ssize_t bytes_read = HANDLE_EINTR(read(fd, buffer, kReadSize));
+    if (bytes_read < 0) {
+      DPLOG(ERROR) << "Couldn't read /proc/self/maps";
+      proc_maps->clear();
+      return false;
+    }
+
+    // ... and don't forget to trim off excess bytes.
+    proc_maps->resize(pos + bytes_read);
+
+    if (bytes_read == 0)
+      break;
+
+    // The gate VMA is handled as a special case after seq_file has finished
+    // iterating through all entries in the virtual memory table.
+    //
+    // Unfortunately, if additional entries are added at this point in time
+    // seq_file gets confused and the next call to read() will return duplicate
+    // entries including the gate VMA again.
+    //
+    // Avoid this by searching for the gate VMA and breaking early.
+    if (ContainsGateVMA(proc_maps, pos))
+      break;
+  }
+
+  return true;
 }
 
 bool ParseProcMaps(const std::string& input,
diff --git a/base/debug/proc_maps_linux.h b/base/debug/proc_maps_linux.h
index d055712..9fbd478 100644
--- a/base/debug/proc_maps_linux.h
+++ b/base/debug/proc_maps_linux.h
@@ -43,6 +43,40 @@ struct MappedMemoryRegion {
 
 // Reads the data from /proc/self/maps and stores the result in |proc_maps|.
 // Returns true if successful, false otherwise.
+//
+// There is *NO* guarantee that the resulting contents will be free of
+// duplicates or even contain valid entries by time the method returns.
+//
+//
+// THE GORY DETAILS
+//
+// Did you know it's next-to-impossible to atomically read the whole contents
+// of /proc/<pid>/maps? You would think that if we passed in a large-enough
+// buffer to read() that It Should Just Work(tm), but sadly that's not the case.
+//
+// Linux's procfs uses seq_file [1] for handling iteration, text formatting,
+// and dealing with resulting data that is larger than the size of a page. That
+// last bit is especially important because it means that seq_file will never
+// return more than the size of a page in a single call to read().
+//
+// Unfortunately for a program like Chrome the size of /proc/self/maps is
+// larger than the size of page so we're forced to call read() multiple times.
+// If the virtual memory table changed in any way between calls to read() (e.g.,
+// a different thread calling mprotect()), it can make seq_file generate
+// duplicate entries or skip entries.
+//
+// Even if seq_file was changed to keep flushing the contents of its page-sized
+// buffer to the usermode buffer inside a single call to read(), it has to
+// release its lock on the virtual memory table to handle page faults while
+// copying data to usermode. This puts us in the same situation where the table
+// can change while we're copying data.
+//
+// Alternatives such as fork()-and-suspend-the-parent-while-child-reads were
+// attempted, but they present more subtle problems than it's worth. Depending
+// on your use case your best bet may be to read /proc/<pid>/maps prior to
+// starting other threads.
+//
+// [1] http://kernelnewbies.org/Documents/SeqFileHowTo
 BASE_EXPORT bool ReadProcMaps(std::string* proc_maps);
 
 // Parses /proc/<pid>/maps input data and stores in |regions|. Returns true
diff --git a/base/debug/proc_maps_linux_unittest.cc b/base/debug/proc_maps_linux_unittest.cc
index 9b7d7dc..7c2929f 100644
--- a/base/debug/proc_maps_linux_unittest.cc
+++ b/base/debug/proc_maps_linux_unittest.cc
@@ -180,14 +180,7 @@ TEST(ProcMapsTest, Permissions) {
   }
 }
 
-// ProcMapsTest.ReadProcMaps fails under TSan on Linux,
-// see http://crbug.com/258451.
-#if defined(THREAD_SANITIZER)
-#define MAYBE_ReadProcMaps DISABLED_ReadProcMaps
-#else
-#define MAYBE_ReadProcMaps ReadProcMaps
-#endif
-TEST(ProcMapsTest, MAYBE_ReadProcMaps) {
+TEST(ProcMapsTest, ReadProcMaps) {
   std::string proc_maps;
   ASSERT_TRUE(ReadProcMaps(&proc_maps));
 
@@ -238,6 +231,13 @@ TEST(ProcMapsTest, MAYBE_ReadProcMaps) {
   EXPECT_TRUE(found_address);
 }
 
+TEST(ProcMapsTest, ReadProcMapsNonEmptyString) {
+  std::string old_string("I forgot to clear the string");
+  std::string proc_maps(old_string);
+  ASSERT_TRUE(ReadProcMaps(&proc_maps));
+  EXPECT_EQ(std::string::npos, proc_maps.find(old_string));
+}
+
 TEST(ProcMapsTest, MissingFields) {
   static const char* kTestCases[] = {
     "00400000\n",                               // Missing end + beyond.