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

  I noticed this crash:

    $ ./gdb --data-directory=data-directory -nx -q \
          testsuite/outputs/gdb.python/pretty-print-call-by-hand/pretty-print-call-by-hand \
	  -x testsuite/outputs/gdb.python/pretty-print-call-by-hand/pretty-print-call-by-hand.py \
	  -ex "b g" -ex r
    (gdb) info frame
    Stack level 0, frame at 0x7fffffffdd80:
     rip = 0x555555555160 in g
        (/home/simark/src/binutils-gdb/gdb/testsuite/gdb.python/pretty-print-call-by-hand.c:41); saved rip = 0x5555555551a3
     called by frame at 0x7fffffffdda0
     source language c.
     Arglist at 0x7fffffffdd70, args: mt=mytype is 0x555555556004 "hello world",
        depth=10

    Fatal signal: Segmentation fault

This is another case of frame_info being invalidated under a function's
feet.  The stack trace when the frame_info get invalidated looks like:

    ... many frames to pretty print the arg, that eventually invalidate the frame_infos ...
    #35 0x00005568d0a8ab24 in print_frame_arg (fp_opts=..., arg=0x7ffc3216bcb0) at /home/simark/src/binutils-gdb/gdb/stack.c:489
    #36 0x00005568d0a8cc75 in print_frame_args (fp_opts=..., func=0x621000233210, frame=..., num=-1, stream=0x60b000000300)
        at /home/simark/src/binutils-gdb/gdb/stack.c:898
    #37 0x00005568d0a9536d in info_frame_command_core (fi=..., selected_frame_p=true) at /home/simark/src/binutils-gdb/gdb/stack.c:1682

print_frame_args knows that print_frame_arg can invalidate frame_info
objects, and therefore calls prepare_reinflate/reinflate.  However,
info_frame_command_core has a separate frame_info_ptr instance (it is
passed by value / copy).  So info_frame_command_core needs to know that
print_frame_args can invalidate frame_info objects, and therefore needs
to prepare_reinflate/reinflate as well.  Add those calls, and enhance
the gdb.python/pretty-print-call-by-hand.exp test to test that command.

Change-Id: I9edaae06d62e97ffdb30938d364437737238a960
---
 gdb/stack.c                                            | 4 ++++
 gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp | 8 ++++++++
 2 files changed, 12 insertions(+)
  

On 07/11/2022 16:53, Simon Marchi via Gdb-patches wrote:
> I noticed this crash:
>
>      $ ./gdb --data-directory=data-directory -nx -q \
>            testsuite/outputs/gdb.python/pretty-print-call-by-hand/pretty-print-call-by-hand \
> 	  -x testsuite/outputs/gdb.python/pretty-print-call-by-hand/pretty-print-call-by-hand.py \
> 	  -ex "b g" -ex r
>      (gdb) info frame
>      Stack level 0, frame at 0x7fffffffdd80:
>       rip = 0x555555555160 in g
>          (/home/simark/src/binutils-gdb/gdb/testsuite/gdb.python/pretty-print-call-by-hand.c:41); saved rip = 0x5555555551a3
>       called by frame at 0x7fffffffdda0
>       source language c.
>       Arglist at 0x7fffffffdd70, args: mt=mytype is 0x555555556004 "hello world",
>          depth=10
>
>      Fatal signal: Segmentation fault
>
> This is another case of frame_info being invalidated under a function's
> feet.  The stack trace when the frame_info get invalidated looks like:
>
>      ... many frames to pretty print the arg, that eventually invalidate the frame_infos ...
>      #35 0x00005568d0a8ab24 in print_frame_arg (fp_opts=..., arg=0x7ffc3216bcb0) at /home/simark/src/binutils-gdb/gdb/stack.c:489
>      #36 0x00005568d0a8cc75 in print_frame_args (fp_opts=..., func=0x621000233210, frame=..., num=-1, stream=0x60b000000300)
>          at /home/simark/src/binutils-gdb/gdb/stack.c:898
>      #37 0x00005568d0a9536d in info_frame_command_core (fi=..., selected_frame_p=true) at /home/simark/src/binutils-gdb/gdb/stack.c:1682
>
> print_frame_args knows that print_frame_arg can invalidate frame_info
> objects, and therefore calls prepare_reinflate/reinflate.  However,
> info_frame_command_core has a separate frame_info_ptr instance (it is
> passed by value / copy).  So info_frame_command_core needs to know that
> print_frame_args can invalidate frame_info objects, and therefore needs
> to prepare_reinflate/reinflate as well.  Add those calls, and enhance
> the gdb.python/pretty-print-call-by-hand.exp test to test that command.

Can confirm that the crash is reproducible and that the patch fixes the 
problem. Sorry for missing it the first time. Makes me wonder if I also 
missed this in print_frame... Either way:

Reviewed-By: Bruno Larsen <blarsen@redhat.com>
  

On 11/8/22 04:32, Bruno Larsen wrote:
> On 07/11/2022 16:53, Simon Marchi via Gdb-patches wrote:
>> I noticed this crash:
>>
>>      $ ./gdb --data-directory=data-directory -nx -q \
>>            testsuite/outputs/gdb.python/pretty-print-call-by-hand/pretty-print-call-by-hand \
>>       -x testsuite/outputs/gdb.python/pretty-print-call-by-hand/pretty-print-call-by-hand.py \
>>       -ex "b g" -ex r
>>      (gdb) info frame
>>      Stack level 0, frame at 0x7fffffffdd80:
>>       rip = 0x555555555160 in g
>>          (/home/simark/src/binutils-gdb/gdb/testsuite/gdb.python/pretty-print-call-by-hand.c:41); saved rip = 0x5555555551a3
>>       called by frame at 0x7fffffffdda0
>>       source language c.
>>       Arglist at 0x7fffffffdd70, args: mt=mytype is 0x555555556004 "hello world",
>>          depth=10
>>
>>      Fatal signal: Segmentation fault
>>
>> This is another case of frame_info being invalidated under a function's
>> feet.  The stack trace when the frame_info get invalidated looks like:
>>
>>      ... many frames to pretty print the arg, that eventually invalidate the frame_infos ...
>>      #35 0x00005568d0a8ab24 in print_frame_arg (fp_opts=..., arg=0x7ffc3216bcb0) at /home/simark/src/binutils-gdb/gdb/stack.c:489
>>      #36 0x00005568d0a8cc75 in print_frame_args (fp_opts=..., func=0x621000233210, frame=..., num=-1, stream=0x60b000000300)
>>          at /home/simark/src/binutils-gdb/gdb/stack.c:898
>>      #37 0x00005568d0a9536d in info_frame_command_core (fi=..., selected_frame_p=true) at /home/simark/src/binutils-gdb/gdb/stack.c:1682
>>
>> print_frame_args knows that print_frame_arg can invalidate frame_info
>> objects, and therefore calls prepare_reinflate/reinflate.  However,
>> info_frame_command_core has a separate frame_info_ptr instance (it is
>> passed by value / copy).  So info_frame_command_core needs to know that
>> print_frame_args can invalidate frame_info objects, and therefore needs
>> to prepare_reinflate/reinflate as well.  Add those calls, and enhance
>> the gdb.python/pretty-print-call-by-hand.exp test to test that command.
> 
> Can confirm that the crash is reproducible and that the patch fixes the problem. Sorry for missing it the first time. Makes me wonder if I also missed this in print_frame... Either way:

I checked print_frame, it could use prepare_reinflate/reinflate.  But
the only way for the frame variable to be re-used after the
print_frame_args call is if:

   (funname == NULL || sal.symtab == NULL)

If that expression is true, then it's likely that we don't have debug
info for the function, so it's not likely that some pretty printer was
used.

But this shows how the current frame_info_ptr is error-prone: you have
two know which functions can, deep down their call tree, reinit the
frame cache.  And all their callers that have a frame_info_ptr object,
recursively, must explicitly do prepare_reinflate / reinflate to protect
themselves against their frame_info object being invalidated.  It's very
easy to forget some spots.

I'm currently working on making frame_info_ptr work automatically,
meaning it would grab the wrapped frame id automatically on
construction, and reinflate the frame automatically if needed in
operator-> or operator*.  The thing that currently throws a wrench in
all that is the "frame view" command, which allows you to create and
select frames out of thin air.  And these frames are currently not
reinflatable.  And that prevents using my version of frame_info_ptr in
code paths that can process that kind of frame, like print_frame_args.
It defeats the purpose of frame_info_ptr, because those are the places
that would benefit from that the most.  So I'm currently checking if I
can improve "frame view" and the support for those user-created frames
just enough to make frame_info_ptr support them.

> Reviewed-By: Bruno Larsen <blarsen@redhat.com>

Thanks,

Simon
  

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

If this can be overcome then that would be great.

Tom
  

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