[patch] 2/3: Fix crash on self-looping DW_OP_fbreg

[patch] 2/3: Fix crash on self-looping DW_OP_fbreg

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 457 bytes --]

Hi,

I saw there GDB will naively loop and crash on DW_OP_fbreg contained in the
location list of DW_AT_frame_base.

I am not aware of an existing case using such broken debug info.

dwarf_expr_context->recursion_depth and dwarf_expr_context->max_recursion_depth
are declared but never used in the GDB sources.  IMO MAX_RECURSION_DEPTH could
be a constant but I left the code according to the existing declaration.

No regressions on x86_64.


Regards,
Jan

[-- Attachment #2: gdb-fbreg-loop.patch --]
[-- Type: text/plain, Size: 3193 bytes --]

2008-04-28  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* Makefile.in: Update dependencies.
	* dwarf2expr.c: New include "gdb_assert.h".
	(new_dwarf_expr_context): Initialize MAX_RECURSION_DEPTH.
	(struct dwarf_expr_eval_cleanup, dwarf_expr_eval_cleanup): New.
	(dwarf_expr_eval): Protect the RECURSION_DEPTH count for exceptions.
	(execute_stack_op): Error out on too large RECURSION_DEPTH.
	Increase/decrease RECURSION_DEPTH around the function.

--- ./gdb/Makefile.in	24 Apr 2008 10:21:44 -0000	1.1004
+++ ./gdb/Makefile.in	28 Apr 2008 00:26:38 -0000
@@ -2077,7 +2077,7 @@ dummy-frame.o: dummy-frame.c $(defs_h) $
 dfp.o: dfp.c $(defs_h) $(expression_h) $(gdbtypes_h) $(value_h) $(dfp_h) \
 	$(decimal128_h) $(decimal64_h) $(decimal32_h)
 dwarf2expr.o: dwarf2expr.c $(defs_h) $(symtab_h) $(gdbtypes_h) $(value_h) \
-	$(gdbcore_h) $(elf_dwarf2_h) $(dwarf2expr_h)
+	$(gdbcore_h) $(elf_dwarf2_h) $(dwarf2expr_h) $(gdb_assert_h)
 dwarf2-frame.o: dwarf2-frame.c $(defs_h) $(dwarf2expr_h) $(elf_dwarf2_h) \
 	$(frame_h) $(frame_base_h) $(frame_unwind_h) $(gdbcore_h) \
 	$(gdbtypes_h) $(symtab_h) $(objfiles_h) $(regcache_h) \
--- ./gdb/dwarf2expr.c	18 Mar 2008 19:40:47 -0000	1.25
+++ ./gdb/dwarf2expr.c	28 Apr 2008 00:26:50 -0000
@@ -27,6 +27,7 @@
 #include "gdbcore.h"
 #include "elf/dwarf2.h"
 #include "dwarf2expr.h"
+#include "gdb_assert.h"
 
 /* Local prototypes.  */
 
@@ -46,6 +47,7 @@ new_dwarf_expr_context (void)
   retval->stack = xmalloc (retval->stack_allocated * sizeof (CORE_ADDR));
   retval->num_pieces = 0;
   retval->pieces = 0;
+  retval->max_recursion_depth = 0x100;
   return retval;
 }
 
@@ -131,10 +133,35 @@ add_piece (struct dwarf_expr_context *ct
 /* Evaluate the expression at ADDR (LEN bytes long) using the context
    CTX.  */
 
+struct dwarf_expr_eval_cleanup
+  {
+    struct dwarf_expr_context *ctx;
+    int old_recursion_depth;
+  };
+
+static void
+dwarf_expr_eval_cleanup (void *data_voidp)
+{
+  struct dwarf_expr_eval_cleanup *data = data_voidp;
+
+  data->ctx->recursion_depth = data->old_recursion_depth;
+}
+
 void
 dwarf_expr_eval (struct dwarf_expr_context *ctx, gdb_byte *addr, size_t len)
 {
+  struct cleanup *back_to;
+  struct dwarf_expr_eval_cleanup data;
+
+  data.ctx = ctx;
+  data.old_recursion_depth = ctx->recursion_depth;
+  back_to = make_cleanup (dwarf_expr_eval_cleanup, &data);
+
   execute_stack_op (ctx, addr, addr + len);
+
+  gdb_assert (ctx->recursion_depth == data.old_recursion_depth);
+  /* It would be a NOP.  */
+  discard_cleanups (back_to);
 }
 
 /* Decode the unsigned LEB128 constant at BUF into the variable pointed to
@@ -281,6 +308,11 @@ execute_stack_op (struct dwarf_expr_cont
   ctx->in_reg = 0;
   ctx->initialized = 1;  /* Default is initialized.  */
 
+  if (ctx->recursion_depth > ctx->max_recursion_depth)
+    error (_("DWARF-2 expression error: Loop detected (%d)."),
+	   ctx->recursion_depth);
+  ctx->recursion_depth++;
+
   while (op_ptr < op_end)
     {
       enum dwarf_location_atom op = *op_ptr++;
@@ -739,4 +771,7 @@ execute_stack_op (struct dwarf_expr_cont
       dwarf_expr_push (ctx, result);
     no_push:;
     }
+
+  ctx->recursion_depth--;
+  gdb_assert (ctx->recursion_depth >= 0);
 }

Re: [patch] 2/3: Fix crash on self-looping DW_OP_fbreg

[Daniel Jacobowitz]

On Mon, Apr 28, 2008 at 10:37:32AM +0200, Jan Kratochvil wrote:
> 2008-04-28  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* Makefile.in: Update dependencies.
> 	* dwarf2expr.c: New include "gdb_assert.h".
> 	(new_dwarf_expr_context): Initialize MAX_RECURSION_DEPTH.
> 	(struct dwarf_expr_eval_cleanup, dwarf_expr_eval_cleanup): New.
> 	(dwarf_expr_eval): Protect the RECURSION_DEPTH count for exceptions.
> 	(execute_stack_op): Error out on too large RECURSION_DEPTH.
> 	Increase/decrease RECURSION_DEPTH around the function.

OK.  Do we need a cleanup for this though?  If we error out, we won't
get back until we create a new context, so missing the decrement
is not a problem.


-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] 2/3: Fix crash on self-looping DW_OP_fbreg

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 590 bytes --]

On Thu, 01 May 2008 22:29:04 +0200, Daniel Jacobowitz wrote:
...
> OK.  Do we need a cleanup for this though?  If we error out, we won't
> get back until we create a new context, so missing the decrement
> is not a problem.

I find it too fragile relying only on the current callers of DWARF_EXPR_EVAL.
There may be a hard to catch bug after someone wraps some part of the code by
CATCH_EXCEPTIONS and expects DWARF_EXPR_CONTEXT passed into the
CATCH_EXCEPTIONS block will stay uncorrupted afterwards.

Attached a discouraged simplified patch not using the exception system.


Regards,
Jan

[-- Attachment #2: gdb-fbreg-loop-nonex.patch --]
[-- Type: text/plain, Size: 2604 bytes --]

2008-05-04  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* Makefile.in: Update dependencies.
	* dwarf2expr.c: New include "gdb_assert.h".
	(new_dwarf_expr_context): Initialize MAX_RECURSION_DEPTH.
	(dwarf_expr_eval): Sanity check the RECURSION_DEPTH count.
	(execute_stack_op): Error out on too large RECURSION_DEPTH.
	Increase/decrease RECURSION_DEPTH around the function.

--- ./gdb/Makefile.in	24 Apr 2008 10:21:44 -0000	1.1004
+++ ./gdb/Makefile.in	28 Apr 2008 00:26:38 -0000
@@ -2077,7 +2077,7 @@ dummy-frame.o: dummy-frame.c $(defs_h) $
 dfp.o: dfp.c $(defs_h) $(expression_h) $(gdbtypes_h) $(value_h) $(dfp_h) \
 	$(decimal128_h) $(decimal64_h) $(decimal32_h)
 dwarf2expr.o: dwarf2expr.c $(defs_h) $(symtab_h) $(gdbtypes_h) $(value_h) \
-	$(gdbcore_h) $(elf_dwarf2_h) $(dwarf2expr_h)
+	$(gdbcore_h) $(elf_dwarf2_h) $(dwarf2expr_h) $(gdb_assert_h)
 dwarf2-frame.o: dwarf2-frame.c $(defs_h) $(dwarf2expr_h) $(elf_dwarf2_h) \
 	$(frame_h) $(frame_base_h) $(frame_unwind_h) $(gdbcore_h) \
 	$(gdbtypes_h) $(symtab_h) $(objfiles_h) $(regcache_h) \
--- ./gdb/dwarf2expr.c	18 Mar 2008 19:40:47 -0000	1.25
+++ ./gdb/dwarf2expr.c	4 May 2008 13:05:56 -0000
@@ -27,6 +27,7 @@
 #include "gdbcore.h"
 #include "elf/dwarf2.h"
 #include "dwarf2expr.h"
+#include "gdb_assert.h"
 
 /* Local prototypes.  */
 
@@ -46,6 +46,7 @@ new_dwarf_expr_context (void)
   retval->stack = xmalloc (retval->stack_allocated * sizeof (CORE_ADDR));
   retval->num_pieces = 0;
   retval->pieces = 0;
+  retval->max_recursion_depth = 0x100;
   return retval;
 }
 
@@ -134,7 +135,13 @@ add_piece (struct dwarf_expr_context *ct
 void
 dwarf_expr_eval (struct dwarf_expr_context *ctx, gdb_byte *addr, size_t len)
 {
+  int old_recursion_depth = ctx->recursion_depth;
+
   execute_stack_op (ctx, addr, addr + len);
+
+  /* CTX RECURSION_DEPTH becomes invalid if an exception was thrown here.  */
+
+  gdb_assert (ctx->recursion_depth == old_recursion_depth);
 }
 
 /* Decode the unsigned LEB128 constant at BUF into the variable pointed to
@@ -281,6 +286,11 @@ execute_stack_op (struct dwarf_expr_cont
   ctx->in_reg = 0;
   ctx->initialized = 1;  /* Default is initialized.  */
 
+  if (ctx->recursion_depth > ctx->max_recursion_depth)
+    error (_("DWARF-2 expression error: Loop detected (%d)."),
+	   ctx->recursion_depth);
+  ctx->recursion_depth++;
+
   while (op_ptr < op_end)
     {
       enum dwarf_location_atom op = *op_ptr++;
@@ -739,4 +749,7 @@ execute_stack_op (struct dwarf_expr_cont
       dwarf_expr_push (ctx, result);
     no_push:;
     }
+
+  ctx->recursion_depth--;
+  gdb_assert (ctx->recursion_depth >= 0);
 }

Re: [patch] 2/3: Fix crash on self-looping DW_OP_fbreg

[Daniel Jacobowitz]

On Sun, May 04, 2008 at 03:14:03PM +0200, Jan Kratochvil wrote:
> On Thu, 01 May 2008 22:29:04 +0200, Daniel Jacobowitz wrote:
> ...
> > OK.  Do we need a cleanup for this though?  If we error out, we won't
> > get back until we create a new context, so missing the decrement
> > is not a problem.
> 
> I find it too fragile relying only on the current callers of DWARF_EXPR_EVAL.
> There may be a hard to catch bug after someone wraps some part of the code by
> CATCH_EXCEPTIONS and expects DWARF_EXPR_CONTEXT passed into the
> CATCH_EXCEPTIONS block will stay uncorrupted afterwards.
> 
> Attached a discouraged simplified patch not using the exception system.

Thanks.  Could you please commit this version?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] 2/3: Fix crash on self-looping DW_OP_fbreg

[Jan Kratochvil]

On Thu, 05 Jun 2008 18:30:01 +0200, Daniel Jacobowitz wrote:
> On Sun, May 04, 2008 at 03:14:03PM +0200, Jan Kratochvil wrote:
> > Attached a discouraged simplified patch not using the exception system.
> 
> Thanks.  Could you please commit this version?

Committed.


Thanks,
Jan