Hi,
>> Force address validation on x86-64 to avoid frequent crashes, caused
>> both by .eh_frame info inaccuracies and current libunwind limitations.
>> It is not sufficient to validate only in or after signal frames.
>>>
>>
>
> Could you make this a compile time option?
Like this? Updated patch + knock-on update in another patch in the series.
Subject: [PATCH 1/2] Always validate all addresses.
From: Lassi Tuura <[email protected]>
Date: Thu, 22 Apr 2010 16:38:13 +0200
Force address validation on x86-64 to avoid frequent crashes, caused
both by .eh_frame info inaccuracies and current libunwind limitations.
It is not sufficient to validate only in or after signal frames.
---
configure.in | 6 ++++++
src/x86_64/Ginit_local.c | 4 ++++
2 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/configure.in b/configure.in
index fcc8813..a116104 100644
--- a/configure.in
+++ b/configure.in
@@ -169,6 +169,12 @@ if test x$enable_block_signals = xyes; then
AC_DEFINE([CONFIG_BLOCK_SIGNALS], [], [Block signals before mutex
operations])
fi
+AC_ARG_ENABLE(conservative_checks,
+[ --enable-conservative-checks Validate all memory addresses before use],
+[enable_conservative_checks=$enableval], [enable_conservative_checks=no])
+if test x$enable_conservative_checks = xyes; then
+ CPPFLAGS="${CPPFLAGS} -DCONSERVATIVE_CHECKS"
+fi
LIBUNWIND___THREAD
diff --git a/src/x86_64/Ginit_local.c b/src/x86_64/Ginit_local.c
index 1e80cb8..37fe0bd 100644
--- a/src/x86_64/Ginit_local.c
+++ b/src/x86_64/Ginit_local.c
@@ -51,7 +51,11 @@ unw_init_local (unw_cursor_t *cursor, ucontext_t *uc)
c->dwarf.as = unw_local_addr_space;
c->dwarf.as_arg = c;
c->uc = uc;
+#if CONSERVATIVE_CHECKS
+ c->validate = 1;
+#else
c->validate = 0;
+#endif
return common_init (c);
}
Subject: [PATCH 2/2] Correct choice of next or previous instruction.
From: Lassi Tuura <[email protected]>
Date: Thu, 22 Apr 2010 16:38:18 +0200
---
include/dwarf.h | 1 +
src/arm/Ginit_local.c | 2 +-
src/arm/Ginit_remote.c | 2 +-
src/arm/init.h | 3 ++-
src/dwarf/Gparser.c | 36 +++++++++++++++++++++++++++++++++---
src/hppa/Ginit_local.c | 2 +-
src/hppa/Ginit_remote.c | 2 +-
src/hppa/init.h | 4 +++-
src/mips/Ginit_local.c | 2 +-
src/mips/Ginit_remote.c | 2 +-
src/mips/init.h | 3 ++-
src/ppc/Ginit_local.c | 4 ++--
src/ppc/Ginit_remote.c | 4 ++--
src/ppc32/init.h | 3 ++-
src/ppc64/init.h | 3 ++-
src/x86/Ginit_local.c | 2 +-
src/x86/Ginit_remote.c | 2 +-
src/x86/init.h | 3 ++-
src/x86_64/Ginit_local.c | 2 +-
src/x86_64/Ginit_remote.c | 2 +-
src/x86_64/init.h | 3 ++-
21 files changed, 63 insertions(+), 24 deletions(-)
diff --git a/include/dwarf.h b/include/dwarf.h
index 53c803b..5dcc6ec 100644
--- a/include/dwarf.h
+++ b/include/dwarf.h
@@ -295,6 +295,7 @@ typedef struct dwarf_cursor
dwarf_loc_t loc[DWARF_NUM_PRESERVED_REGS];
+ unsigned int use_prev_instr :1; /* use previous (= call) or current (=
signal) instruction? */
unsigned int pi_valid :1; /* is proc_info valid? */
unsigned int pi_is_dynamic :1; /* proc_info found via dynamic proc info? */
unw_proc_info_t pi; /* info about current procedure */
diff --git a/src/arm/Ginit_local.c b/src/arm/Ginit_local.c
index 7b2881e..debf5bb 100644
--- a/src/arm/Ginit_local.c
+++ b/src/arm/Ginit_local.c
@@ -47,7 +47,7 @@ unw_init_local (unw_cursor_t *cursor, ucontext_t *uc)
c->dwarf.as = unw_local_addr_space;
c->dwarf.as_arg = uc;
- return common_init (c);
+ return common_init (c, 1);
}
#endif /* !UNW_REMOTE_ONLY */
diff --git a/src/arm/Ginit_remote.c b/src/arm/Ginit_remote.c
index 3baf3f6..854f3b6 100644
--- a/src/arm/Ginit_remote.c
+++ b/src/arm/Ginit_remote.c
@@ -40,6 +40,6 @@ unw_init_remote (unw_cursor_t *cursor, unw_addr_space_t as,
void *as_arg)
c->dwarf.as = as;
c->dwarf.as_arg = as_arg;
- return common_init (c);
+ return common_init (c, 0);
#endif /* !UNW_LOCAL_ONLY */
}
diff --git a/src/arm/init.h b/src/arm/init.h
index 5b3f927..a302569 100644
--- a/src/arm/init.h
+++ b/src/arm/init.h
@@ -25,7 +25,7 @@ WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE. */
#include "unwind_i.h"
static inline int
-common_init (struct cursor *c)
+common_init (struct cursor *c, unsigned use_prev_instr)
{
int ret, i;
@@ -62,6 +62,7 @@ common_init (struct cursor *c)
c->dwarf.args_size = 0;
c->dwarf.ret_addr_column = 0;
+ c->dwarf.use_prev_instr = use_prev_instr;
c->dwarf.pi_valid = 0;
c->dwarf.pi_is_dynamic = 0;
c->dwarf.hint = 0;
diff --git a/src/dwarf/Gparser.c b/src/dwarf/Gparser.c
index 22dc01e..efaa1ad 100644
--- a/src/dwarf/Gparser.c
+++ b/src/dwarf/Gparser.c
@@ -76,7 +76,10 @@ run_cfi_program (struct dwarf_cursor *c,
dwarf_state_record_t *sr,
a = unw_get_accessors (as);
curr_ip = c->pi.start_ip;
- while (curr_ip < ip && *addr < end_addr)
+ /* Process everything up to and including the current 'ip',
+ including all the DW_CFA_advance_loc instructions. See
+ 'c->use_prev_instr' use in 'fetch_proc_info' for details. */
+ while (curr_ip <= ip && *addr < end_addr)
{
if ((ret = dwarf_readu8 (as, a, addr, &op, arg)) < 0)
return ret;
@@ -381,7 +384,23 @@ fetch_proc_info (struct dwarf_cursor *c, unw_word_t ip,
int need_unwind_info)
{
int ret, dynamic = 1;
- --ip;
+ /* The 'ip' can point either to the previous or next instruction
+ depending on what type of frame we have: normal call or a place
+ to resume execution (e.g. after signal frame).
+
+ For a normal call frame we need to back up so we point within the
+ call itself; this is important because a) the call might be the
+ very last instruction of the function and the edge of the FDE,
+ and b) so that run_cfi_program() runs locations up to the call
+ but not more.
+
+ For execution resume, we need to do the exact opposite and look
+ up using the current 'ip' value. That is where execution will
+ continue, and it's important we get this right, as 'ip' could be
+ right at the function entry and hence FDE edge, or at instruction
+ that manipulates CFA (push/pop). */
+ if (c->use_prev_instr)
+ --ip;
if (c->pi_valid && !need_unwind_info)
return 0;
@@ -405,6 +424,14 @@ fetch_proc_info (struct dwarf_cursor *c, unw_word_t ip,
int need_unwind_info)
if (ret >= 0)
tdep_fetch_frame (c, ip, need_unwind_info);
+ /* Update use_prev_instr for the next frame. */
+ if (need_unwind_info)
+ {
+ assert(c->pi.unwind_info);
+ struct dwarf_cie_info *dci = c->pi.unwind_info;
+ c->use_prev_instr = ! dci->signal_frame;
+ }
+
return ret;
}
@@ -810,7 +837,10 @@ dwarf_find_save_locs (struct dwarf_cursor *c)
rs = rs_lookup(cache, c);
if (rs)
- c->ret_addr_column = rs->ret_addr_column;
+ {
+ c->ret_addr_column = rs->ret_addr_column;
+ c->use_prev_instr = ! rs->signal_frame;
+ }
else
{
if ((ret = fetch_proc_info (c, c->ip, 1)) < 0 ||
diff --git a/src/hppa/Ginit_local.c b/src/hppa/Ginit_local.c
index 243ffd4..6d4dd3d 100644
--- a/src/hppa/Ginit_local.c
+++ b/src/hppa/Ginit_local.c
@@ -48,7 +48,7 @@ unw_init_local (unw_cursor_t *cursor, ucontext_t *uc)
c->dwarf.as = unw_local_addr_space;
c->dwarf.as_arg = uc;
- return common_init (c);
+ return common_init (c, 1);
}
#endif /* !UNW_REMOTE_ONLY */
diff --git a/src/hppa/Ginit_remote.c b/src/hppa/Ginit_remote.c
index 3d6606d..bd6093f 100644
--- a/src/hppa/Ginit_remote.c
+++ b/src/hppa/Ginit_remote.c
@@ -41,6 +41,6 @@ unw_init_remote (unw_cursor_t *cursor, unw_addr_space_t as,
void *as_arg)
c->dwarf.as = as;
c->dwarf.as_arg = as_arg;
- return common_init (c);
+ return common_init (c, 0);
#endif /* !UNW_LOCAL_ONLY */
}
diff --git a/src/hppa/init.h b/src/hppa/init.h
index d5f900d..d14354f 100644
--- a/src/hppa/init.h
+++ b/src/hppa/init.h
@@ -26,7 +26,7 @@ WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE. */
#include "unwind_i.h"
static inline int
-common_init (struct cursor *c)
+common_init (struct cursor *c, unsigned use_prev_instr)
{
int ret;
@@ -40,5 +40,7 @@ common_init (struct cursor *c)
ret = hppa_get (c, HPPA_REG_LOC (c, UNW_HPPA_SP), &c->sp);
if (ret < 0)
return ret;
+
+ c->dwarf.use_prev_instr = use_prev_instr;
return 0;
}
diff --git a/src/mips/Ginit_local.c b/src/mips/Ginit_local.c
index 7b2881e..debf5bb 100644
--- a/src/mips/Ginit_local.c
+++ b/src/mips/Ginit_local.c
@@ -47,7 +47,7 @@ unw_init_local (unw_cursor_t *cursor, ucontext_t *uc)
c->dwarf.as = unw_local_addr_space;
c->dwarf.as_arg = uc;
- return common_init (c);
+ return common_init (c, 1);
}
#endif /* !UNW_REMOTE_ONLY */
diff --git a/src/mips/Ginit_remote.c b/src/mips/Ginit_remote.c
index 3baf3f6..854f3b6 100644
--- a/src/mips/Ginit_remote.c
+++ b/src/mips/Ginit_remote.c
@@ -40,6 +40,6 @@ unw_init_remote (unw_cursor_t *cursor, unw_addr_space_t as,
void *as_arg)
c->dwarf.as = as;
c->dwarf.as_arg = as_arg;
- return common_init (c);
+ return common_init (c, 0);
#endif /* !UNW_LOCAL_ONLY */
}
diff --git a/src/mips/init.h b/src/mips/init.h
index 98956d4..e32e3c9 100644
--- a/src/mips/init.h
+++ b/src/mips/init.h
@@ -25,7 +25,7 @@ WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE. */
#include "unwind_i.h"
static inline int
-common_init (struct cursor *c)
+common_init (struct cursor *c, unsigned use_prev_instr)
{
int ret, i;
@@ -47,6 +47,7 @@ common_init (struct cursor *c)
c->dwarf.args_size = 0;
c->dwarf.ret_addr_column = 0;
+ c->dwarf.use_prev_instr = use_prev_instr;
c->dwarf.pi_valid = 0;
c->dwarf.pi_is_dynamic = 0;
c->dwarf.hint = 0;
diff --git a/src/ppc/Ginit_local.c b/src/ppc/Ginit_local.c
index 2d9ab2c..b931b5b 100644
--- a/src/ppc/Ginit_local.c
+++ b/src/ppc/Ginit_local.c
@@ -56,9 +56,9 @@ unw_init_local (unw_cursor_t *cursor, ucontext_t *uc)
c->dwarf.as = unw_local_addr_space;
c->dwarf.as_arg = uc;
#ifdef UNW_TARGET_PPC64
- return common_init_ppc64 (c);
+ return common_init_ppc64 (c, 1);
#else
- return common_init_ppc32 (c);
+ return common_init_ppc32 (c, 1);
#endif
}
diff --git a/src/ppc/Ginit_remote.c b/src/ppc/Ginit_remote.c
index 66269d2..0f4b0fd 100644
--- a/src/ppc/Ginit_remote.c
+++ b/src/ppc/Ginit_remote.c
@@ -50,9 +50,9 @@ unw_init_remote (unw_cursor_t *cursor, unw_addr_space_t as,
void *as_arg)
c->dwarf.as_arg = as_arg;
#ifdef UNW_TARGET_PPC64
- return common_init_ppc64(c);
+ return common_init_ppc64 (c, 0);
#elif UNW_TARGET_PPC32
- return common_init_ppc32 (c);
+ return common_init_ppc32 (c, 0);
#else
#error init_remote :: NO VALID PPC ARCH!
#endif
diff --git a/src/ppc32/init.h b/src/ppc32/init.h
index 8badb17..c2208ad 100644
--- a/src/ppc32/init.h
+++ b/src/ppc32/init.h
@@ -30,7 +30,7 @@ WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE. */
/* Here is the "common" init, for remote and local debuging" */
static inline int
-common_init_ppc32 (struct cursor *c)
+common_init_ppc32 (struct cursor *c, unsigned use_prev_instr)
{
int ret;
int i;
@@ -62,6 +62,7 @@ common_init_ppc32 (struct cursor *c)
c->dwarf.args_size = 0;
c->dwarf.ret_addr_column = 0;
+ c->dwarf.use_prev_instr = use_prev_instr;
c->dwarf.pi_valid = 0;
c->dwarf.pi_is_dynamic = 0;
c->dwarf.hint = 0;
diff --git a/src/ppc64/init.h b/src/ppc64/init.h
index 886f14c..64847b8 100644
--- a/src/ppc64/init.h
+++ b/src/ppc64/init.h
@@ -28,7 +28,7 @@ WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE. */
#include "unwind_i.h"
static inline int
-common_init_ppc64 (struct cursor *c)
+common_init_ppc64 (struct cursor *c, unsigned use_prev_instr)
{
int ret;
int i;
@@ -72,6 +72,7 @@ common_init_ppc64 (struct cursor *c)
c->dwarf.args_size = 0;
c->dwarf.ret_addr_column = 0;
+ c->dwarf.use_prev_instr = use_prev_instr;
c->dwarf.pi_valid = 0;
c->dwarf.pi_is_dynamic = 0;
c->dwarf.hint = 0;
diff --git a/src/x86/Ginit_local.c b/src/x86/Ginit_local.c
index 55ab749..5e8b697 100644
--- a/src/x86/Ginit_local.c
+++ b/src/x86/Ginit_local.c
@@ -50,7 +50,7 @@ unw_init_local (unw_cursor_t *cursor, ucontext_t *uc)
c->dwarf.as_arg = c;
c->uc = uc;
c->validate = 0;
- return common_init (c);
+ return common_init (c, 1);
}
#endif /* !UNW_REMOTE_ONLY */
diff --git a/src/x86/Ginit_remote.c b/src/x86/Ginit_remote.c
index 6949a73..aa92405 100644
--- a/src/x86/Ginit_remote.c
+++ b/src/x86/Ginit_remote.c
@@ -51,6 +51,6 @@ unw_init_remote (unw_cursor_t *cursor, unw_addr_space_t as,
void *as_arg)
c->dwarf.as_arg = as_arg;
c->uc = 0;
}
- return common_init (c);
+ return common_init (c, 0);
#endif /* !UNW_LOCAL_ONLY */
}
diff --git a/src/x86/init.h b/src/x86/init.h
index 675b77e..b59ad84 100644
--- a/src/x86/init.h
+++ b/src/x86/init.h
@@ -26,7 +26,7 @@ WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE. */
#include "unwind_i.h"
static inline int
-common_init (struct cursor *c)
+common_init (struct cursor *c, unsigned use_prev_instr)
{
int ret, i;
@@ -59,6 +59,7 @@ common_init (struct cursor *c)
c->dwarf.args_size = 0;
c->dwarf.ret_addr_column = 0;
+ c->dwarf.use_prev_instr = use_prev_instr;
c->dwarf.pi_valid = 0;
c->dwarf.pi_is_dynamic = 0;
c->dwarf.hint = 0;
diff --git a/src/x86_64/Ginit_local.c b/src/x86_64/Ginit_local.c
index 37fe0bd..18b3d98 100644
--- a/src/x86_64/Ginit_local.c
+++ b/src/x86_64/Ginit_local.c
@@ -56,7 +56,7 @@ unw_init_local (unw_cursor_t *cursor, ucontext_t *uc)
#else
c->validate = 0;
#endif
- return common_init (c);
+ return common_init (c, 1);
}
#endif /* !UNW_REMOTE_ONLY */
diff --git a/src/x86_64/Ginit_remote.c b/src/x86_64/Ginit_remote.c
index 8aa644d..25dd686 100644
--- a/src/x86_64/Ginit_remote.c
+++ b/src/x86_64/Ginit_remote.c
@@ -52,6 +52,6 @@ unw_init_remote (unw_cursor_t *cursor, unw_addr_space_t as,
void *as_arg)
c->dwarf.as_arg = as_arg;
c->uc = 0;
}
- return common_init (c);
+ return common_init (c, 0);
#endif /* !UNW_LOCAL_ONLY */
}
diff --git a/src/x86_64/init.h b/src/x86_64/init.h
index ae108b2..dcd4aea 100644
--- a/src/x86_64/init.h
+++ b/src/x86_64/init.h
@@ -28,7 +28,7 @@ WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE. */
#include "unwind_i.h"
static inline int
-common_init (struct cursor *c)
+common_init (struct cursor *c, unsigned use_prev_instr)
{
int ret;
@@ -64,6 +64,7 @@ common_init (struct cursor *c)
c->dwarf.args_size = 0;
c->dwarf.ret_addr_column = RIP;
+ c->dwarf.use_prev_instr = use_prev_instr;
c->dwarf.pi_valid = 0;
c->dwarf.pi_is_dynamic = 0;
c->dwarf.hint = 0;
_______________________________________________
Libunwind-devel mailing list
[email protected]
http://lists.nongnu.org/mailman/listinfo/libunwind-devel
