Bug 1455662 - Guard against mprotect() failure when manipulating link map r=jchen draft
authorJames Willcox <snorp@snorp.net>
Wed, 11 Apr 2018 16:49:11 -0500
changeset 787922 7ed361bd3253645a82a12c8218c77784d54fa9de
parent 787921 5ca9ef085fe6b5352261a866641000dbff2573ec
child 788070 c435f8b4bd3765238230514d9bb354940c7f7db8
push id107841
push userbmo:snorp@snorp.net
push dateWed, 25 Apr 2018 16:42:35 +0000
reviewersjchen
bugs1455662
milestone61.0a1
Bug 1455662 - Guard against mprotect() failure when manipulating link map r=jchen MozReview-Commit-ID: 7orhBmf4j5j
mozglue/linker/ElfLoader.cpp
--- a/mozglue/linker/ElfLoader.cpp
+++ b/mozglue/linker/ElfLoader.cpp
@@ -899,26 +899,38 @@ public:
     length = lastPageEnd - firstPage;
     uintptr_t start = reinterpret_cast<uintptr_t>(firstPage);
     uintptr_t end;
 
     prot = getProt(start, &end);
     if (prot == -1 || (start + length) > end)
       MOZ_CRASH();
 
-    if (prot & PROT_WRITE)
+    if (prot & PROT_WRITE) {
+      success = true;
       return;
+    }
 
     page = firstPage;
-    mprotect(page, length, prot | PROT_WRITE);
+    int ret = mprotect(page, length, prot | PROT_WRITE);
+    success = ret == 0;
+    if (!success) {
+      ERROR("mprotect(%p, %zu, %d) = %d (errno=%d; %s)",
+            page, length, prot | PROT_WRITE, ret,
+            errno, strerror(errno));
+    }
+  }
+
+  bool IsWritable() const {
+    return success;
   }
 
   ~EnsureWritable()
   {
-    if (page != MAP_FAILED) {
+    if (success && page != MAP_FAILED) {
       mprotect(page, length, prot);
 }
   }
 
 private:
   int getProt(uintptr_t addr, uintptr_t *end)
   {
     /* The interesting part of the /proc/self/maps format looks like:
@@ -948,16 +960,17 @@ private:
       return result;
     }
     return -1;
   }
 
   int prot;
   void *page;
   size_t length;
+  bool success;
 };
 
 /**
  * The system linker maintains a doubly linked list of library it loads
  * for use by the debugger. Unfortunately, it also uses the list pointers
  * in a lot of operations and adding our data in the list is likely to
  * trigger crashes when the linker tries to use data we don't provide or
  * that fall off the amount data we allocated. Fortunately, the linker only
@@ -971,54 +984,73 @@ private:
  * program executable, so the system linker should not be changing
  * r_debug::r_map.
  */
 void
 ElfLoader::DebuggerHelper::Add(ElfLoader::link_map *map)
 {
   if (!dbg->r_brk)
     return;
+
   dbg->r_state = r_debug::RT_ADD;
   dbg->r_brk();
-  map->l_prev = nullptr;
-  map->l_next = dbg->r_map;
+
   if (!firstAdded) {
-    firstAdded = map;
     /* When adding a library for the first time, r_map points to data
      * handled by the system linker, and that data may be read-only */
     EnsureWritable w(&dbg->r_map->l_prev);
+    if (!w.IsWritable()) {
+      dbg->r_state = r_debug::RT_CONSISTENT;
+      dbg->r_brk();
+      return;
+    }
+
+    firstAdded = map;
     dbg->r_map->l_prev = map;
   } else
     dbg->r_map->l_prev = map;
+
+  map->l_prev = nullptr;
+  map->l_next = dbg->r_map;
+
   dbg->r_map = map;
   dbg->r_state = r_debug::RT_CONSISTENT;
   dbg->r_brk();
 }
 
 void
 ElfLoader::DebuggerHelper::Remove(ElfLoader::link_map *map)
 {
   if (!dbg->r_brk)
     return;
+
   dbg->r_state = r_debug::RT_DELETE;
   dbg->r_brk();
+
+  if (map == firstAdded) {
+    /* When removing the first added library, its l_next is going to be
+     * data handled by the system linker, and that data may be read-only */
+    EnsureWritable w(&map->l_next->l_prev);
+    if (!w.IsWritable()) {
+      dbg->r_state = r_debug::RT_CONSISTENT;
+      dbg->r_brk();
+      return;
+    }
+
+    firstAdded = map->l_prev;
+    map->l_next->l_prev = map->l_prev;
+  } else if (map->l_next) {
+    map->l_next->l_prev = map->l_prev;
+  }
+
   if (dbg->r_map == map)
     dbg->r_map = map->l_next;
   else if (map->l_prev) {
     map->l_prev->l_next = map->l_next;
   }
-  if (map == firstAdded) {
-    firstAdded = map->l_prev;
-    /* When removing the first added library, its l_next is going to be
-     * data handled by the system linker, and that data may be read-only */
-    EnsureWritable w(&map->l_next->l_prev);
-    map->l_next->l_prev = map->l_prev;
-  } else if (map->l_next) {
-    map->l_next->l_prev = map->l_prev;
-  }
   dbg->r_state = r_debug::RT_CONSISTENT;
   dbg->r_brk();
 }
 
 #if defined(ANDROID) && defined(__NR_sigaction)
 /* As some system libraries may be calling signal() or sigaction() to
  * set a SIGSEGV handler, effectively breaking MappableSeekableZStream,
  * or worse, restore our SIGSEGV handler with wrong flags (which using