Re: [PATCH] gdb: fix use-after-free in check_longjmp_breakpoint_for_call_dummy

[Andrew Burgess <aburgess@redhat.com>]

Simon Marchi <simark@simark.ca> writes:

> On 5/10/23 05:12, Andrew Burgess via Gdb-patches wrote:
>>> @@ -7608,9 +7609,13 @@ set_longjmp_breakpoint_for_call_dummy (void)
>>>  void
>>>  check_longjmp_breakpoint_for_call_dummy (struct thread_info *tp)
>>>  {
>>> -  for (struct breakpoint *b : all_breakpoints_safe ())
>>> +  /* We would need to delete breakpoints other than the current one while
>>> +     iterating, so all_breakpoints_safe is not sufficient to make that safe.
>>> +     Save all breakpoints to delete in that set and delete them at the end.  */
>>> +  std::unordered_set<breakpoint *> to_delete;
>> 
>> Hi,
>> 
>> For my own education: why did you choose a std::unordered_set here?  I
>> would assume that we will never find the same related breakpoint more
>> than once.  Indeed, if we did then I suspect the old code would have
>> resulted in a double free.
>> 
>> So why choose a set over a vector?
>
> We look for bp_longjmp_call_dummy breakpoints, which are documented like
> this:
>
>     /* Breakpoint placed to the same location(s) like bp_longjmp but used to
>        protect against stale DUMMY_FRAME.  Multiple bp_longjmp_call_dummy and
>        one bp_call_dummy are chained together by related_breakpoint for each
>        DUMMY_FRAME.  */
>
> I can imagine this happening: suppose X and Y are two related
> bp_longjmp_call_dummy breakpoints, following each other in
> breakpoint_chain.  When looking at X, we will insert X and Y in
> to_delete.  We will then look at X, and we will try to insert X and Y
> again in to_delete.
>
> The old code wouldn't double free or use-after-free, because of its
> special handling of B_TMP.  When looking at X, we would delete Y and
> then X.  And if Y happened to be the next iteration value (saved in the
> B_TMP variable), we would modify B_TMP to avoid iterating on Y.

Thanks for the explanation.

Andrew