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

[Simon Marchi <simark@simark.ca>]

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.

Simon