Re: [PATCH] gdb/stack.c: avoid stale pointers when printing frame arguments

[Tom Tromey <tom@tromey.com>]

>>>>> "Bruno" == Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:

Bruno> To fix this solution, we must ensure that the frame cache is up to date
Bruno> after printing any arguments, so we cache the frame_id of the pointer
Bruno> that will become stale, and reset that pointer using frame_find_by_id.

Makes sense.

Bruno> We have to use it directly instead of using the simpler
Bruno> scoped_restore_selected_frame because the latter may save a null frame_ID
Bruno> when the saved frame is the current frame, and restoring the null frame
Bruno> wouldn't rebuild the frame cache.

Also it's not the selected frame that is being restored.

Bruno> This commit also adds a testcase that exercises this codepath with 7
Bruno> different triggers, run, continue, step, backtrace, finish, up and down.
Bruno> Some of them can seem to be testing the same thing twice, but since this
Bruno> test relies on stale pointers, there is always a chance that GDB got lucky
Bruno> when testing, so better to test extra.

One thing I've toyed with a couple of times is replacing "frame_info *"
with a "frame_info_ptr" smart pointer that automatically clears itself
when the frame cache is cleared.  This would then cause reliable crashes
in scenarios like this -- with the caveat, of course, that some test has
to actually exercise the failing code path.

This got bogged down a bit because I then tried to have frame_info_ptr
automatically reinflate, which turns out to be hard for some reason I
don't fully recall.

Maybe a reinflating frame_info_ptr could be done piecemeal, starting
with the spots you identified in this patch, though...

Bruno> diff --git a/gdb/infrun.c b/gdb/infrun.c
Bruno> index 737710f5bae..bf519737d12 100644
Bruno> --- a/gdb/infrun.c
Bruno> +++ b/gdb/infrun.c
Bruno> @@ -8308,8 +8308,15 @@ print_stop_location (const target_waitstatus &ws)
Bruno>       SRC_LINE: Print only source line
Bruno>       LOCATION: Print only location
Bruno>       SRC_AND_LOC: Print location and source line.  */
Bruno> +  int frame_cache_count = get_frame_cache_generation ();
Bruno>    if (do_frame_printing)
Bruno>      print_stack_frame (get_selected_frame (NULL), 0, source_flag, 1);
Bruno> +  /* If there are more frame cache generations after we print a stack frame
Bruno> +     we may have invalidated the cache with a pretty printer.
Bruno> +     If this happened, the safest option is to restart the whole cache,
Bruno> +     otherwise GDB may crash because of an use-after-free bug.  */
Bruno> +  if (frame_cache_count < get_frame_cache_generation ())
Bruno> +    reinit_frame_cache ();
Bruno>  }

I don't think I understand this bit.  If the generation changes, hasn't
the cache already been cleared?

Bruno> +      for (struct frame_id fid;
Bruno>  	   fi && (i <= frame_high || frame_high == -1);
Bruno>  	   i++, fi = get_prev_frame (fi))

Better to declare fid where it's first used.

Tom