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

[Bruno Larsen <blarsen@redhat.com>]

On 4/4/22 16:06, Tom Tromey wrote:
>>>>>> "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...

This sounds like a good idea. I am just not sure if you are suggesting it as a fix instead of what I proposed, or to implement later, can you clarify it please?

> 
> 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?

If the cache has been cleared by printing a frame, it was done because a function was called manually (probably). If it did happen, the cache may have been invalidated and it is safer to rebuild everything. print_frame_cache by itself doesn't reinitialize the frame cache. I thought it crashed GDB if I did it there, but I was apparently mistaken, so I can move this code there and have a more robust solution.

I should probably also reword the comment to make things more clear.

> 
> 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

Will do.

> 
> Tom
> 


-- 
Cheers!
Bruno Larsen