[Libunwind-devel] [PATCH][x86_64] Make address validation a per thread setting

[Arun Sharma]

For local unwinding, we have a defence mechanism against bad/missing
unwind information, which could result in libunwind dereferencing
bad pointers. This mechanism is based on msync(2) system call and
significantly reduces the chances of a bad pointer dereference in
libunwind.

The original idea was to turn this mechanism on only when necessary
i.e. libunwind didn't find proper unwind information for a IP.

There are a couple of problems in the current implementation.

* The flag is global and is modified without locking
* The flag isn't reset when starting a new unwind

The attached patch makes ->validate a per-thread setting by moving
it into struct cursor from unw_local_addr_space and resets it to
false when starting a new unwind. As a result, cursor->as_arg points
to the cursor itself instead of the ucontext (for the local case).

This was found to reduce the number of msync() system calls from an
application using libunwind significantly.

Signed-off-by: Paul Pluzhnikov <address@hidden>
Signed-off-by: Arun Sharma <address@hidden>

--- a/include/tdep-x86_64/libunwind_i.h
+++ b/include/tdep-x86_64/libunwind_i.h
@@ -51,7 +51,6 @@ struct unw_addr_space
     unw_word_t dyn_info_list_addr;     /* (cached) dyn_info_list_addr */
     struct dwarf_rs_cache global_cache;
     struct unw_debug_frame_list *debug_frames;
-    int validate;
    };
 
 struct cursor
@@ -67,8 +66,17 @@ struct cursor
       }
     sigcontext_format;
     unw_word_t sigcontext_addr;
+    int validate;
+    ucontext_t *uc;
   };
 
+static inline ucontext_t *
+dwarf_get_uc(const struct dwarf_cursor *cursor)
+{
+  const struct cursor *c = (struct cursor *) cursor->as_arg;
+  return c->uc;
+}
+
 #define DWARF_GET_LOC(l)       ((l).val)
 
 #ifdef UNW_LOCAL_ONLY
@@ -77,10 +85,10 @@ struct cursor
 # define DWARF_LOC(r, t)       ((dwarf_loc_t) { .val = (r) })
 # define DWARF_IS_REG_LOC(l)   0
 # define DWARF_REG_LOC(c,r)    (DWARF_LOC((unw_word_t)                      \
-                                tdep_uc_addr((c)->as_arg, (r)), 0))
+                                tdep_uc_addr(dwarf_get_uc(c), (r)), 0))
 # define DWARF_MEM_LOC(c,m)    DWARF_LOC ((m), 0)
 # define DWARF_FPREG_LOC(c,r)  (DWARF_LOC((unw_word_t)                      \
-                                tdep_uc_addr((c)->as_arg, (r)), 0))
+                                tdep_uc_addr(dwarf_get_uc(c), (r)), 0))
 #else /* !UNW_LOCAL_ONLY */
 
 # define DWARF_LOC_TYPE_FP     (1 << 0)
diff --git a/src/x86_64/Ginit.c b/src/x86_64/Ginit.c
index 026e8d2..031deaa 100644
--- a/src/x86_64/Ginit.c
+++ b/src/x86_64/Ginit.c
@@ -158,7 +158,8 @@ access_mem (unw_addr_space_t as, unw_word_t addr, 
unw_word_t *val, int write,
   else
     {
       /* validate address */
-      if (as->validate && validate_mem(addr))
+      const struct cursor *c = (const struct cursor *)arg;
+      if (c && c->validate && validate_mem(addr))
         return -1;
       *val = *(unw_word_t *) addr;
       Debug (16, "mem[%016lx] -> %lx\n", addr, *val);
@@ -171,7 +172,7 @@ access_reg (unw_addr_space_t as, unw_regnum_t reg, 
unw_word_t *val, int write,
            void *arg)
 {
   unw_word_t *addr;
-  ucontext_t *uc = arg;
+  ucontext_t *uc = ((struct cursor *)arg)->uc;
 
   if (unw_is_fpreg (reg))
     goto badreg;
@@ -200,7 +201,7 @@ static int
 access_fpreg (unw_addr_space_t as, unw_regnum_t reg, unw_fpreg_t *val,
              int write, void *arg)
 {
-  ucontext_t *uc = arg;
+  ucontext_t *uc = ((struct cursor *)arg)->uc;
   unw_fpreg_t *addr;
 
   if (!unw_is_fpreg (reg))
@@ -252,7 +253,6 @@ x86_64_local_addr_space_init (void)
   local_addr_space.acc.get_proc_name = get_static_proc_name;
   unw_flush_cache (&local_addr_space, 0, 0);
 
-  local_addr_space.validate = 0;
   memset (last_good_addr, 0, sizeof (unw_word_t) * NLGA);
   lga_victim = 0;
 }
diff --git a/src/x86_64/Ginit_local.c b/src/x86_64/Ginit_local.c
index 9bb2513..1e80cb8 100644
--- a/src/x86_64/Ginit_local.c
+++ b/src/x86_64/Ginit_local.c
@@ -49,7 +49,9 @@ unw_init_local (unw_cursor_t *cursor, ucontext_t *uc)
   Debug (1, "(cursor=%p)\n", c);
 
   c->dwarf.as = unw_local_addr_space;
-  c->dwarf.as_arg = uc;
+  c->dwarf.as_arg = c;
+  c->uc = uc;
+  c->validate = 0;
   return common_init (c);
 }
 
diff --git a/src/x86_64/Ginit_remote.c b/src/x86_64/Ginit_remote.c
index 9b2fd23..8aa644d 100644
--- a/src/x86_64/Ginit_remote.c
+++ b/src/x86_64/Ginit_remote.c
@@ -42,7 +42,16 @@ unw_init_remote (unw_cursor_t *cursor, unw_addr_space_t as, 
void *as_arg)
   Debug (1, "(cursor=%p)\n", c);
 
   c->dwarf.as = as;
-  c->dwarf.as_arg = as_arg;
+  if (as == unw_local_addr_space)
+    {
+      c->dwarf.as_arg = c;
+      c->uc = as_arg;
+    }
+  else
+    {
+      c->dwarf.as_arg = as_arg;
+      c->uc = 0;
+    }
   return common_init (c);
 #endif /* !UNW_LOCAL_ONLY */
 }
diff --git a/src/x86_64/Gis_signal_frame.c b/src/x86_64/Gis_signal_frame.c
index b795827..72edf46 100644
--- a/src/x86_64/Gis_signal_frame.c
+++ b/src/x86_64/Gis_signal_frame.c
@@ -40,7 +40,6 @@ unw_is_signal_frame (unw_cursor_t *cursor)
 
   as = c->dwarf.as;
   a = unw_get_accessors (as);
-  as->validate = 1;    /* Don't trust the ip */
   arg = c->dwarf.as_arg;
 
   /* Check if RIP points at sigreturn sequence.
diff --git a/src/x86_64/Gresume.c b/src/x86_64/Gresume.c
index 4edc4da..b400040 100644
--- a/src/x86_64/Gresume.c
+++ b/src/x86_64/Gresume.c
@@ -51,7 +51,7 @@ x86_64_local_resume (unw_addr_space_t as, unw_cursor_t 
*cursor, void *arg)
 {
 #if defined(__linux)
   struct cursor *c = (struct cursor *) cursor;
-  ucontext_t *uc = c->dwarf.as_arg;
+  ucontext_t *uc = c->uc;
 
   /* Ensure c->pi is up-to-date.  On x86-64, it's relatively common to
      be missing DWARF unwind info.  We don't want to fail in that
diff --git a/src/x86_64/Gstep.c b/src/x86_64/Gstep.c
index 75f796f..2da1c25 100644
--- a/src/x86_64/Gstep.c
+++ b/src/x86_64/Gstep.c
@@ -71,6 +71,10 @@ unw_step (unw_cursor_t *cursor)
       unw_word_t prev_ip = c->dwarf.ip, prev_cfa = c->dwarf.cfa;
       struct dwarf_loc rbp_loc, rsp_loc, rip_loc;
 
+      /* We could get here because of missing/bad unwind information.
+         Validate all addresses before dereferencing. */
+      c->validate = 1;
+
       Debug (13, "dwarf_step() failed (ret=%d), trying frame-chain\n", ret);
 
       if (unw_is_signal_frame (cursor))