Re: [PATCHv2] gdb/mi: fix use after free of frame_info causing spurious notifications

[Andrew Burgess <aburgess@redhat.com>]

Simon Marchi <simark@simark.ca> writes:

> On 2022-03-24 14:52, Andrew Burgess via Gdb-patches wrote:
>> Simon,
>>
>> Thanks for the feedback.  I believe this addresses all your points.
>> What do you think?
>>
>> Thanks,
>> Andrew
>>
>> ---
>>
>> In commit:
>>
>>   commit a2757c4ed693cef4ecc4dcdcb2518353eb6b3c3f
>>   Date:   Wed Mar 16 15:08:22 2022 +0000
>>
>>       gdb/mi: consistently notify user when GDB/MI client uses -thread-select
>>
>> Changes were made to GDB to address some inconsistencies in when
>> notifications are sent from a MI terminal to a CLI terminal (when
>> multiple terminals are in use, see new-ui command).
>>
>> Unfortunately, in order to track when the currently selected frame has
>> changed, that commit grabs a frame_info pointer before and after an MI
>> command has executed, and compares the pointers to see if the frame
>> has changed.
>>
>> This is not safe.
>>
>> If the frame cache is deleted for any reason then the frame_info
>> pointer captured before the command started, is no longer valid, and
>> any comparisons based on that pointer are undefined.
>>
>> This was leading to random test failures for some folk, see:
>>
>>   https://sourceware.org/pipermail/gdb-patches/2022-March/186867.html
>>
>> This commit changes GDB so we no longer hold frame_info pointers, but
>> instead store the frame_id and frame_level, this is safe even when the
>> frame cache is flushed.
>> ---
>>  gdb/mi/mi-main.c | 43 +++++++++++++++++++++++++++++++++----------
>>  1 file changed, 33 insertions(+), 10 deletions(-)
>>
>> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
>> index abd033b22ae..28d4afc28c0 100644
>> --- a/gdb/mi/mi-main.c
>> +++ b/gdb/mi/mi-main.c
>> @@ -1974,27 +1974,50 @@ struct user_selected_context
>>  {
>>    /* Constructor.  */
>>    user_selected_context ()
>> -    : m_previous_ptid (inferior_ptid),
>> -      m_previous_frame (deprecated_safe_get_selected_frame ())
>> -  { /* Nothing.  */ }
>> +    : m_previous_ptid (inferior_ptid)
>> +  {
>> +    save_selected_frame (&m_previous_frame_id, &m_previous_frame_level);
>> +  }
>>
>>    /* Return true if the user selected context has changed since this object
>>       was created.  */
>>    bool has_changed () const
>>    {
>> -    return ((m_previous_ptid != null_ptid
>> -	     && inferior_ptid != null_ptid
>> -	     && m_previous_ptid != inferior_ptid)
>> -	    || m_previous_frame != deprecated_safe_get_selected_frame ());
>> +    /* Did the selected thread change?  */
>> +    if (m_previous_ptid != null_ptid && inferior_ptid != null_ptid
>> +	&& m_previous_ptid != inferior_ptid)
>> +      return true;
>> +
>> +    /* Grab details of the currently selected frame, for comparison.  */
>> +    frame_id current_frame_id;
>> +    int current_frame_level;
>> +    save_selected_frame (&current_frame_id, &current_frame_level);
>> +
>> +    /* Did the selected frame level change?  */
>> +    if (current_frame_level != m_previous_frame_level)
>> +      return true;
>> +
>> +    /* Did the selected frame id change?  If the innermost frame is
>> +       selected then the level will be -1, and the frame-id will be
>> +       null_frame_id.  As comparing null_frame_id with itself always
>> +       reports not-equal, we only do the equality test if we have something
>> +       other than the innermost frame selected.  */
>> +    if (current_frame_level != -1
>> +	&& !frame_id_eq (current_frame_id, m_previous_frame_id))
>> +      return true;
>> +
>> +    /* Nothing changed!  */
>> +    return false;
>>    }
>>  private:
>>    /* The previously selected thread.  This might be null_ptid if there was
>>       no previously selected thread.  */
>>    ptid_t m_previous_ptid;
>>
>> -  /* The previously selected frame.  This might be nullptr if there was no
>> -     previously selected frame.  */
>> -  frame_info *m_previous_frame;
>> +  /* The previously selected frame.  If the innermost frame is selected
>> +     then the frame_id will be null_frame_id, and the level will be -1.  */
>
> I noticed you changed this from "if no frame is currently selected" to
> "if the innermost frame is selected".  I think that both can happen,
> actually:
>
>  - if you execute an MI command while the target is running, there is no
>    selected frame
>  - if you execute an MI command while the innermost frame is selected
>
> So my understanding is that you could mention both (although it might
> become a bit redundant with the big comment on top of selected_frame_id,
> in frame.c).
>
> LGTM in any case.

I updated the comment as you suggested and pushed this patch to mater
and gdb-12-branch.

Thanks,
Andrew