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

[David Mosberger-Tang]

Applied, thanks.

  --david

On 6/9/08, Arun Sharma <address@hidden> wrote:
> 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))
>
>
>  _______________________________________________
>  Libunwind-devel mailing list
>  address@hidden
>  http://lists.nongnu.org/mailman/listinfo/libunwind-devel
>


-- 
Mosberger Consulting LLC, http://www.mosberger-consulting.com/