[PATCH v2 0/3] Deal with destroy signal use after free issues

[Derek Foreman]

On 2018-04-16 04:29 PM, Markus Ongyerth wrote:
> Hi,
> 
> Thanks for getting to this. I was waiting for the release, but I'm currently 
> not at full capacity, so you got it before me.
> 
> The commit message of patch 1 contains a lie. The second paragraph should 
> contain "IF there was only one listener object", which the testcase in Patch 3 
> shows. But I don't think that's a deal breaker.

Oops, you're absolutely right, I didn't re-write the text after seeing
your test case.  Until then I hadn't thought about that case.  (I'm
still surprised it took us until now to actually hit this.)

If none object I'll correct that while landing.  If another revision is
required I'll update the text then.

> For Patch 1/2:
> Reviewed-by: Markus Ongyerth <wl at ongy.net>
> 
> I'm fine with moving/resubmit of my code and am happy I could provide the 
> testcase that shows an issue.
> Since it's originally authored by me, I guess my R-B would be weird there :)

I didn't put my R-B on it because I made a (mostly cosmetic) change to
it, and wasn't sure if that was ok.

Thanks,
Derek

> Cheers,
> ongy
> 
> On 2018/April/16 03:00, Derek Foreman wrote:
>> Now that the release is out, I'd like to dig back into this mess.
>> This is a round up of some patches that were on list shortly before
>> the release to deal with a problem where many existing libwayland
>> users don't delete their destroy signal listeners before freeing
>> them.
>>
>> These leads to a bit of a mess (as Markus' test illustrates) if there
>> are multiple destroy listeners.
>>
>> I've included:
>> My test patch to ensure the existing behaviour continues to work
>> (users like weston and enlightenment can free during destroy
>> listener)
>>
>> The special case destroy emit path for wl_priv_signal - this is
>> an attempt to "fix" the problem, by making the destroy signal
>> emit operate without ever touching potentially free()d elements
>> again.
>>
>> Markus' test that would fail without patch 2/3, as it catches the
>> free() without removal case we've all come to know any love.
>>
>> Derek Foreman (2):
>>   tests: Test for use after free in resource destruction signals
>>    changes since first appearance: none
>>
>>   server: Add special case destroy signal emitter
>>    changes since first appearance:  stop trying to maintain a list head
>>
>> Markus Ongyerth (1):
>>   tests: Add free-without-remove test
>>    changes since first appearance: I moved it into an existing file
>>
>>  src/wayland-private.h  |  3 +++
>>  src/wayland-server.c   | 46 +++++++++++++++++++++++++++++++++++++++++++---
>>  tests/resources-test.c | 39 +++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 85 insertions(+), 3 deletions(-)
>>
>> -- 
>> 2.14.3
>>