Prefetch the native library from native code by parsing /proc/pid/maps.

base/android/library_loader/library_prefetcher.cc

+// Copyright 2015 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/android/library_loader/library_prefetcher.h"
+
+#include <sys/resource.h>
+#include <sys/wait.h>
+#include <unistd.h>
+#include <utility>
+#include <vector>
+
+#include "base/logging.h"
+#include "base/posix/eintr_wrapper.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_util.h"
+
+namespace base {
+namespace android {
+
+namespace {
+
+// Android defines the background priority to this value since at least 2009

[pasko, 2015/03/27 13:29:28]

If THREAD_PRIORITY_BACKGROUND can change, let's just pass it from Java. Probably
nothing would break in this century, but less things to think about when
reading.

[Benoit L, 2015/03/27 15:44:52]

Well, this adds clutter, and as you said, is highly likely not to change. And it
is a lower priority than the default one anyway. Unless Android decides to have
foreground applications with a lower priority than the default one, we are fine.

+// (see Process.java).
+const int kBackgroundPriority = 10;
+// Valid for all the Android architectures.
+const size_t kPageSize = 4096;
+// We may load directly from the APK.
+const int kSuffixesToMatchCount = 2;
+const std::string kSuffixesToMatch[] = {"libchrome.so", "base.apk"};

[pasko, 2015/03/27 13:29:28]

<quote>
... we only allow static variables to contain POD data. This rule completely
disallows vector (use C arrays instead), or string (use const char []).
</quote>

https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Static_and_G...

[Benoit L, 2015/03/27 15:44:52]

Done.

+
+bool IsReadableAndPrivate(const std::string& flags) {
+  return flags.size() == 4 && flags[0] == 'r' && flags[3] == 'p';
+}
+
+bool FilenameMatchesSuffix(const std::string& filename) {
+  for (int i = 0; i < kSuffixesToMatchCount; i++) {
+    if (EndsWith(filename, kSuffixesToMatch[i], true)) {
+      return true;
+    }
+  }
+  return false;
+}
+
+// For each range, reads a byte per page to force it into the page cache.

[pasko, 2015/03/27 17:51:54]

can you add a comment saying that heap allocations are not allowed in this
function?

+void Prefetch(const std::vector<std::pair<uint64_t, uint64_t>>& ranges) {
+  for (const auto& range : ranges) {
+    const uint64_t page_mask = kPageSize - 1;
+    CHECK(!(range.first & page_mask) && !(range.second & page_mask));
+    unsigned char* start_address = (unsigned char*)range.first;
+    unsigned char* end_address = (unsigned char*)range.second;
+    unsigned char dummy;
+    for (unsigned char* addr = start_address; addr < end_address;
+         addr += kPageSize) {
+      // Volatile is required to prevent the compiler from eliminating this
+      // loop.
+      dummy ^= *((volatile unsigned char*)addr);
+    }
+  }
+}
+
+}  // namespace
+
+bool NativeLibraryPrefetcher::ParseMapping(const std::string& line,

[pasko, 2015/03/27 13:29:28]

// static

(same for the 2 other functions)

[Benoit L, 2015/03/27 15:44:51]

Done.

+                                           Mapping* mapping) {
+  // Example:
+  // b6e62000-b6e64000 r-xp 00000000 b3:15 1204 /system/lib/libstdc++.so
+  // start-end flags offset device inode [filename]
+  std::vector<std::string> pieces;
+  base::SplitStringAlongWhitespace(line, &pieces);
+  size_t pieces_count = pieces.size();
+  // Filename is optional, so at least 5 fields.
+  if (pieces_count < 5) {
+    return false;
+  }
+
+  std::vector<std::string> start_end;
+  base::SplitString(pieces[0], '-', &start_end);
+  if (start_end.size() != 2) {
+    return false;
+  }
+  bool ok = base::HexStringToUInt64(start_end[0], &mapping->start);
+  ok &= base::HexStringToUInt64(start_end[1], &mapping->end);
+  mapping->flags = pieces[1];
+  ok &= base::HexStringToUInt64(pieces[2], &mapping->offset);
+  mapping->device = pieces[3];
+  ok &= base::StringToInt64(pieces[4], &mapping->inode);
+  if (!ok) {
+    return false;
+  }
+  if (pieces_count == 5) {
+    mapping->filename = std::string();
+  } else {
+    // Truncates filenames containing spaces, but these are not the droids we're
+    // looking for.
+    mapping->filename = pieces[5];
+  }
+  return true;
+}
+
+bool NativeLibraryPrefetcher::MappingMatches(const Mapping& mapping) {
+  return FilenameMatchesSuffix(mapping.filename) &&
+      IsReadableAndPrivate(mapping.flags);  // Code mappings are private.
+}
+
+bool NativeLibraryPrefetcher::FindRanges(
+    std::vector<std::pair<uint64_t, uint64_t>>* ranges) {
+  base::ScopedFILE file(base::OpenFile(base::FilePath("/proc/self/maps"), "r"));

[pasko, 2015/03/27 13:29:27]

OpenFile is the implementation detail of base::File. Any reason not to use
base::File? there are tons of uses of it across the codebase.

[Benoit L, 2015/03/27 15:44:52]

We need a FILE*, and not a File. And File::PlatformFile is an int on Unix.

[pasko, 2015/03/27 17:51:54]

ah, you are right, fgets, and there is no line by line reading in base::File

+  if (!file.get()) {
+    return false;
+  }
+  std::vector<Mapping> mappings;
+  const size_t kMaxLineLength = 4096;
+  char line_buffer[kMaxLineLength];
+  while (char* line_str = fgets(line_buffer, kMaxLineLength - 1, file.get())) {
+    std::string line(line_str);
+    Mapping mapping;
+    if (!ParseMapping(line, &mapping)) {
+      continue;
+    }
+    if (MappingMatches(mapping)) {
+      mappings.push_back(mapping);
+    }
+  }
+  const std::string& libchrome_suffix = kSuffixesToMatch[0];

[pasko, 2015/03/27 13:29:27]

nit: it's probably nice to separate this part with one empty line and putting a
comment like:
// Add ranges. If chrome DSO ranges are found avoid taking other ranges.

[Benoit L, 2015/03/27 15:44:52]

Done.

+  bool has_libchrome_mapping = false;
+  for (const Mapping& mapping : mappings) {
+    if (EndsWith(mapping.filename, libchrome_suffix, true)) {
+      has_libchrome_mapping = true;
+      break;
+    }
+  }
+  for (const Mapping& mapping : mappings) {
+    if (has_libchrome_mapping
+        && !EndsWith(mapping.filename, libchrome_suffix, true)) {
+      continue;
+    }
+    ranges->push_back(std::make_pair(mapping.start, mapping.end));
+  }
+  return true;
+}
+
+// Forks and waits for a process prefetching the native library. This is done in
+// a forked process for the following reasons:
+// - Isolating the main process from mistakes in the parsing. If the parsing

[pasko, 2015/03/27 11:03:37]

Forking does not fully 'isolate' because the process was granted all the same
capabilities. A buffer overrun may harm the main process by writing something to
one of the FDs being open. We are hoping that doing a ROP with /proc/pid/maps is
nontrivial, but it's certainly a new attack vector.

+//   returns an incorrect address, only the forked process will crash.
+// - Not inflating the memory used by the main process uselessly, which could
+//   increase its likelihood to be killed.
+// The forked process has background priority and, since it is not declared to
+// the android runtime, can be killed at any time, which is not an issue here.
+bool NativeLibraryPrefetcher::ForkAndPrefetchNativeLibrary() {
+  // Looking for ranges is done before the fork, to avoid syscalls and/or
+  // memory allocations in the forked process, that can be problematic.

[pasko, 2015/03/27 17:51:54]

s/ that can be problematic//

does not add any useful information. You could explain, of course that memory
allocator lock can be acquired in the child process.

+  std::vector<std::pair<uint64_t, uint64_t>> ranges;
+  if (!FindRanges(&ranges)) {
+    return false;
+  }
+  pid_t pid = fork();

[pasko, 2015/03/27 11:03:37]

what if the child process deadlocks? It might happen to want to grab a lock in
malloc(), and that might be acquired by the parent process just before the fork
syscall.

yey complexity, shall we spawn a timer to kill the child process upon timeout?

[Benoit L, 2015/03/27 15:44:51]

This process doesn't do any heap allocation, and doesn't call any syscall aside
from _exit(). This is why the parsing is done in the parent process.
In the worst case, this will leave an AsyncTask behind, sitting idle. I would
like to avoid complexity if possible.

+  if (pid == 0) {
+    setpriority(PRIO_PROCESS, 0, kBackgroundPriority);
+    Prefetch(ranges);
+    exit(0);

[pasko, 2015/03/27 11:03:37]

this would invoke destructors which we don't do normally for non-browser
processes, I can expect all kinds of disasters caused by this up to database
backends competing to write data to disk.

Should just die here.

[Benoit L, 2015/03/27 15:44:51]

_exit(2) doesn't call the atexit() handlers.
Done.

+  } else {
+    if (pid < 0) {
+      return false;
+    }
+    int status;
+    const pid_t result = HANDLE_EINTR(waitpid(pid, &status, 0));
+    if (result == pid) {
+      if (WIFEXITED(status)) {
+        return WEXITSTATUS(status) == 0;
+      }
+      if (WIFSIGNALED(status)) {
+        switch (WTERMSIG(status)) {
+          case SIGABRT:
+          case SIGBUS:
+          case SIGFPE:
+          case SIGILL:
+          case SIGSEGV:
+            LOG(WARNING) << "The native library prefetching process crashed ("
+                         << pid << ")";

[pasko, 2015/03/27 11:03:37]

break;

[Benoit L, 2015/03/27 15:44:52]

Done.

[pasko, 2015/03/27 17:51:54]

I like how this got done :)

+          case SIGINT:
+          case SIGKILL:
+          case SIGTERM:
+            LOG(WARNING) << "The native library prefetching process was killed "

[pasko, 2015/03/27 11:03:37]

is there a reason to distinguish these cases? If you plan to add a histogram to
cover this, please add a TODO here

[Benoit L, 2015/03/27 15:44:52]

Done.

+                         << "(" << pid << ")";
+          default:
+            break;
+        }
+      }
+    }
+    return false;
+  }
+}
+
+}  // namespace android
+}  // namespace base