Greetings,

One of my applications crashed in _ULx86_64_tdep_trace on line 504 here:

// src/x86_64/Gtrace.c

503  cfa = cfa + f->cfa_reg_offset;
504  ACCESS_MEM_FAST(ret, c->validate, d, cfa + f->rbp_cfa_offset + dRIP, rip);

I've finally tracked it down to f->rpb_cfa_offset being incorrect.

The problem is that {rsp,rbp}_cfa_offset only have 15 bits, but for
SIGRETURN frame they are filled with:

// src/x86_64/Gstash_frame.c

    f->cfa_reg_offset = d->cfa - c->sigcontext_addr;
    f->rbp_cfa_offset = DWARF_GET_LOC(d->loc[RBP]) - d->cfa;
    f->rsp_cfa_offset = DWARF_GET_LOC(d->loc[RSP]) - d->cfa;

The problem is that the delta here can be arbitrarily large when
sigaltstack is used, and can easily overflow the 15 and 30-bit fields.

Unfortunately I did not succeed it creating a stand-alone test case showing
this crash.

Several fixes are possible:

1. we can simply turn validation on for SIGRETURN. Something like:

--- src/x86_64/Gtrace.c 2011-11-05 12:08:30.000000000 -0700
+++ src/x86_64/Gtrace.c.new     2011-11-08 09:45:23.000000000 -0800
@@ -501,11 +501,11 @@ tdep_trace (unw_cursor_t *cursor, void *
         is stored at some unknown constant offset off inner frame's
         CFA.  We determine the actual offset from DWARF unwind info. */
       cfa = cfa + f->cfa_reg_offset;
-      ACCESS_MEM_FAST(ret, c->validate, d, cfa + f->rbp_cfa_offset +
dRIP, rip);
+      ret = dwarf_get (d, DWARF_MEM_LOC (d, cfa + f->rbp_cfa_offset +
dRIP), &rip);
       if (likely(ret >= 0))
-        ACCESS_MEM_FAST(ret, c->validate, d, cfa + f->rbp_cfa_offset, rbp);
+       ret = dwarf_get (d, DWARF_MEM_LOC (d, cfa + f->rbp_cfa_offset), &rbp);
       if (likely(ret >= 0))
-        ACCESS_MEM_FAST(ret, c->validate, d, cfa + f->rsp_cfa_offset, rsp);
+       ret = dwarf_get (d, DWARF_MEM_LOC (d, cfa + f->rsp_cfa_offset), &rbp);

       /* Resume stack at signal restoration point. The stack is not
          necessarily continuous here, especially with sigaltstack(). */


2. we can detect overflow in Gstash_frame.c, and mark the frame as impossible
   to handle in fast trace:

--- src/x86_64/Gstash_frame.c   2011-10-05 15:52:33.000000000 -0700
+++ src/x86_64/Gstash_frame.c.new       2011-11-08 09:49:59.000000000 -0800
@@ -85,6 +85,15 @@ tdep_stash_frame (struct dwarf_cursor *d
     f->cfa_reg_offset = d->cfa - c->sigcontext_addr;
     f->rbp_cfa_offset = DWARF_GET_LOC(d->loc[RBP]) - d->cfa;
     f->rsp_cfa_offset = DWARF_GET_LOC(d->loc[RSP]) - d->cfa;
+    if (f->cfa_reg_offset != d->cfa - c->sigcontext_addr
+       || f->rbp_cfa_offset != DWARF_GET_LOC(d->loc[RBP]) - d->cfa
+       || f->rsp_cfa_offset = DWARF_GET_LOC(d->loc[RSP]) - d->cfa)
+      {
+       Debug (4, " sigreturn frame overflow\n");
+       f->frame_type = UNW_X86_64_FRAME_OTHER;
+       return;
+      }
+
     Debug (4, " sigreturn frame rbpoff %d rspoff %d\n",
           f->rbp_cfa_offset, f->rsp_cfa_offset);
   }

3. we can abandon DWARF, record the address of sigcontext in the 60
bits remaining,
   and extract registers directly from there (patch attached).


Fix 1 pessimizes fast trace for every signal -- probably very undesirable
for a profiler, so Lassi would not like it.

Fix 2 pessimizes fast trace only when sigaltstack is far enough away.
Undesirable for profiler with sigaltstack (which covers many Google apps).

I don't see any major disadvantages for fix 3.

Tested on Linux/x86_64 (Ubuntu 10.04). No new failures.

Comments?

Thanks,
-- 
Paul Pluzhnikov
diff --git a/include/tdep-x86_64/libunwind_i.h 
b/include/tdep-x86_64/libunwind_i.h
index c4c6960..46762e4 100644
--- a/include/tdep-x86_64/libunwind_i.h
+++ b/include/tdep-x86_64/libunwind_i.h
@@ -50,12 +50,24 @@ unw_tdep_frame_type_t;
 typedef struct
   {
     uint64_t virtual_address;
-    int64_t frame_type     : 2;  /* unw_tdep_frame_type_t classification */
-    int64_t last_frame     : 1;  /* non-zero if last frame in chain */
-    int64_t cfa_reg_rsp    : 1;  /* cfa dwarf base register is rsp vs. rbp */
-    int64_t cfa_reg_offset : 30; /* cfa is at this offset from base register 
value */
-    int64_t rbp_cfa_offset : 15; /* rbp saved at this offset from cfa (-1 = 
not saved) */
-    int64_t rsp_cfa_offset : 15; /* rsp saved at this offset from cfa (-1 = 
not saved) */
+    union
+      {
+       struct
+         {
+           int64_t frame_type     : 2;  /* unw_tdep_frame_type_t 
classification */
+           int64_t last_frame     : 1;  /* non-zero if last frame in chain */
+           int64_t cfa_reg_rsp    : 1;  /* cfa dwarf base register is rsp vs. 
rbp */
+           int64_t cfa_reg_offset : 30; /* cfa is at this offset from base 
register value */
+           int64_t rbp_cfa_offset : 15; /* rbp saved at this offset from cfa 
(-1 = not saved) */
+           int64_t rsp_cfa_offset : 15; /* rsp saved at this offset from cfa 
(-1 = not saved) */
+         };
+       struct
+         {
+           int64_t x_frame_type   :  2;  /* unw_tdep_frame_type_t 
classification */
+           int64_t sigcontext_addr: 62;  /* addr of sigcontext passed into
+                                            SIGRETURN frame.  */
+         };
+      };
   }
 unw_tdep_frame_t;
 
diff --git a/src/x86_64/Gstash_frame.c b/src/x86_64/Gstash_frame.c
index 2f81cc0..3412369 100644
--- a/src/x86_64/Gstash_frame.c
+++ b/src/x86_64/Gstash_frame.c
@@ -82,11 +82,15 @@ tdep_stash_frame (struct dwarf_cursor *d, struct 
dwarf_reg_state *rs)
   else if (f->frame_type == UNW_X86_64_FRAME_SIGRETURN)
   {
     assert (f->cfa_reg_offset == -1);
-    f->cfa_reg_offset = d->cfa - c->sigcontext_addr;
-    f->rbp_cfa_offset = DWARF_GET_LOC(d->loc[RBP]) - d->cfa;
-    f->rsp_cfa_offset = DWARF_GET_LOC(d->loc[RSP]) - d->cfa;
-    Debug (4, " sigreturn frame rbpoff %d rspoff %d\n",
-          f->rbp_cfa_offset, f->rsp_cfa_offset);
+    f->sigcontext_addr = c->sigcontext_addr;
+    if (unlikely (f->sigcontext_addr != c->sigcontext_addr))
+      {
+       Debug (4, " sigreturn frame offset overflow\n");
+       f->frame_type = UNW_X86_64_FRAME_OTHER;
+       return;
+      }
+    Debug (4, " sigreturn frame sigcontext_addr %zu\n",
+          f->sigcontext_addr);
   }
 
   /* PLT and guessed RBP-walked frames are handled in unw_step(). */
diff --git a/src/x86_64/Gtrace.c b/src/x86_64/Gtrace.c
index f10737f..9e84010 100644
--- a/src/x86_64/Gtrace.c
+++ b/src/x86_64/Gtrace.c
@@ -35,12 +35,6 @@ WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE 
SOFTWARE.  */
 /* Initial hash table size. Table expands by 2 bits (times four). */
 #define HASH_MIN_BITS 14
 
-/* There's not enough space to store RIP's location in a signal
-   frame, but we can calculate it relative to RBP's (or RSP's)
-   position in mcontext structure.  Note we don't want to use
-   the UC_MCONTEXT_GREGS_* directly since we rely on DWARF info. */
-#define dRIP (UC_MCONTEXT_GREGS_RIP - UC_MCONTEXT_GREGS_RBP)
-
 typedef struct
 {
   unw_tdep_frame_t *frames;
@@ -50,7 +44,8 @@ typedef struct
                         been called. */
 } unw_trace_cache_t;
 
-static const unw_tdep_frame_t empty_frame = { 0, UNW_X86_64_FRAME_OTHER, -1, 
-1, 0, -1, -1 };
+static const unw_tdep_frame_t empty_frame = {
+  0, {{UNW_X86_64_FRAME_OTHER, -1, -1, 0, -1, -1 }}};
 static pthread_mutex_t trace_init_lock = PTHREAD_MUTEX_INITIALIZER;
 static pthread_once_t trace_cache_once = PTHREAD_ONCE_INIT;
 static sig_atomic_t trace_cache_once_happen;
@@ -497,15 +492,14 @@ tdep_trace (unw_cursor_t *cursor, void **buffer, int 
*size)
 
     case UNW_X86_64_FRAME_SIGRETURN:
       /* Advance standard signal frame, whose CFA points above saved
-         registers (ucontext) among other things.  We know the info
-        is stored at some unknown constant offset off inner frame's
-        CFA.  We determine the actual offset from DWARF unwind info. */
-      cfa = cfa + f->cfa_reg_offset;
-      ACCESS_MEM_FAST(ret, c->validate, d, cfa + f->rbp_cfa_offset + dRIP, 
rip);
-      if (likely(ret >= 0))
-        ACCESS_MEM_FAST(ret, c->validate, d, cfa + f->rbp_cfa_offset, rbp);
-      if (likely(ret >= 0))
-        ACCESS_MEM_FAST(ret, c->validate, d, cfa + f->rsp_cfa_offset, rsp);
+         registers (ucontext) among other things.  */
+      {
+       unw_word_t sigcontext_addr = f->sigcontext_addr;
+
+       ACCESS_MEM_FAST(ret, 0, d, sigcontext_addr + UC_MCONTEXT_GREGS_RIP, 
rip);
+       ACCESS_MEM_FAST(ret, 0, d, sigcontext_addr + UC_MCONTEXT_GREGS_RBP, 
rbp);
+       ACCESS_MEM_FAST(ret, 0, d, sigcontext_addr + UC_MCONTEXT_GREGS_RSP, 
rsp);
+      }
 
       /* Resume stack at signal restoration point. The stack is not
          necessarily continuous here, especially with sigaltstack(). */
_______________________________________________
Libunwind-devel mailing list
[email protected]
https://lists.nongnu.org/mailman/listinfo/libunwind-devel

Reply via email to