Re: [PATCH 2/7] gdb: add prepare_reinflate/reinflate around print_frame_args in info_frame_command_core

[Simon Marchi <simon.marchi@polymtl.ca>]

On 11/8/22 14:39, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> But this shows how the current frame_info_ptr is error-prone: you have
> Simon> two know which functions can, deep down their call tree, reinit the
> Simon> frame cache.  And all their callers that have a frame_info_ptr object,
> Simon> recursively, must explicitly do prepare_reinflate / reinflate to protect
> Simon> themselves against their frame_info object being invalidated.  It's very
> Simon> easy to forget some spots.
> 
> Yeah.  This problem already existed, and the rationale behind
> frame_info_ptr wasn't to fix it, but rather to expose it when it happens
> -- by crashing rather than allowing a UAF.
> 
> Simon> I'm currently working on making frame_info_ptr work automatically,
> Simon> meaning it would grab the wrapped frame id automatically on
> Simon> construction, and reinflate the frame automatically if needed
> 
> We tried this a bit, but the problem we hit was that computing the
> frame id require unwinding a bit, and since the code generally uses
> frame_info_ptr everywhere, gdb would end up unwinding everything.

With the final patch of this series ("gdb: add special handling for
frame level 0 in frame_info_ptr"), frame_info_ptr won't compute frame
0's frame id in order to prepare_reinflate or reinflate, so won't cause
extra unwinding.  For other frames, the frame id is already computed
when the frame is created, so frame_info_ptr won't cause any more
unwinding that already happens by creating the frame_info objects.

With my prototype that grabs the frame id in frame_info_ptr's
constructor (except for frame 0), it requires that for the frame id to
be computed already.  This means that the code paths involved in
computing the frame id wouldn't use frame_info_ptr anymore.  So, for
instance, I would revert frame_this_id_ftype to take a plain
`frame_info *` (and that leads to pretty much the whole struct
frame_unwind API to go back to `frame_info *`.  The point being that as
long as a frame doesn't have a computed id, it is not reinflatable
anyway.  And this is not really the kind of spot that is at risk of
using a stale frame_info.

However, if we really want to keep using frame_info_ptr everywhere (even
in things like frame_this_id_ftype, just to be extra safe), then perhaps
we can have two classes.  One that would provide the protection against
stale frame_info objects, which could be used everywhere, and one that
would do that + automatic reinflation.

Simon