Re: RFC: one approach to fixing PR 14100

Mark Kettenis
2012-08-08T18:27:51Z

> From: Tom Tromey <tromey@redhat.com>
> Date: Thu, 02 Aug 2012 10:11:47 -0600
>
> PR 14100 concerns a way to crash gdb by C-c during a 'bt'.
>
> The way this happens is that dwarf2_frame_cache initializes the frame's
> prologue_cache. Then, it continues to do some more work, including
> (indirectly) reading target memory.
>
> Then, target_read invokes QUIT, throwing an exception.
> The cleanups are run, and eventually we get to
> frame_cleanup_after_sniffer, which asserts that prologue_cache==NULL.
>
> This fix assumes that what dwarf2_frame_cache is doing is not
> unreasonable, and simply clears the prologue_cache field.
>
> I am not sure whether this is really correct.
>
> Another approach would be to change dwarf2_frame_cache to set the
> prologue_cache at the end of its work rather than at the beginning.
> Then, I suppose, we'd have to document this restriction and audit all
> the other sniffers.
>
> Thoughts?

I'd say the gdb_assert() *is* the documentation of the restriction.
So I'd say the 2nd approach is the correct approach. Setting
prologue_cache at the very end certainly used to be the style that
most frame cache routines used.

> * frame.c (frame_cleanup_after_sniffer): Remove assert.
> Clear frame's prologue_cache.
> ---
> gdb/frame.c | 7 +++++--
> 1 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/frame.c b/gdb/frame.c
> index e012f2d..edb379c 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -2392,8 +2392,11 @@ frame_cleanup_after_sniffer (void *arg)
> struct frame_info *frame = arg;
>
> /* The sniffer should not allocate a prologue cache if it did not
> - match this frame. */
> - gdb_assert (frame->prologue_cache == NULL);
> + match this frame. We used to assert that prologue_cache was NULL
> + here -- however, that ran afoul of code paths where the
> + prologue_cache was set by the sniffer, but some later processing
> + called QUIT. */
> + frame->prologue_cache = NULL;

Not a big fan of these comments that try to document history. Better
to just state the reason for setting it to NULL.