[PATCH wayland] tests: Test for use after free in resource destruction signals

[Pekka Paalanen]

On Wed, 21 Feb 2018 13:08:51 -0600
Derek Foreman <derekf at osg.samsung.com> wrote:

> For years it's been common practice to free the object containing
> the wl_listner inside resource destruction notifiers, but not
> remove the listener from the list.
> 
> That is: It's been safe to assume that the wl_listener will never be
> touched again, since this is a destruction callback.
> 
> Recently some patches were reviewed that made some positive changes
> to our internal signal handling code, but would've violated this
> assumption, and changed free()d memory in several existing compositors
> (weston, mutter, enlightenment).
> 
> Since the breakage was extremely subtle, codify this assumption in
> a test case (thus promoting it to an ABI promise).
> 
> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
> ---
>  tests/resources-test.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/tests/resources-test.c b/tests/resources-test.c
> index 59d8beb..76c9eb8 100644
> --- a/tests/resources-test.c
> +++ b/tests/resources-test.c
> @@ -84,6 +84,17 @@ destroy_notify(struct wl_listener *l, void *data)
>  {
>  	assert(l && data);
>  	notify_called = 1;
> +
> +	/* In real code it's common to free the structure holding the
> +	 * listener at this point, but not to remove it from the list.
> +	 *
> +	 * That's fine since this is a destruction notification and
> +	 * it's the last time this signal can fire.  We set these
> +	 * to NULL so we can check them later to ensure no write after
> +	 * "free" occurred.
> +	 */
> +	l->link.prev = NULL;
> +	l->link.next = NULL;
>  }
>  
>  TEST(destroy_res_tst)
> @@ -119,6 +130,8 @@ TEST(destroy_res_tst)
>  	assert(destroyed);
>  	assert(notify_called); /* check if signal was emitted */
>  	assert(wl_client_get_object(client, id) == NULL);
> +	assert(destroy_listener.link.prev == NULL);
> +	assert(destroy_listener.link.next == NULL);
>  
>  	res = wl_resource_create(client, &wl_seat_interface, 2, 0);
>  	assert(res);
> @@ -131,6 +144,8 @@ TEST(destroy_res_tst)
>  	wl_client_destroy(client);
>  	assert(destroyed);
>  	assert(notify_called);
> +	assert(destroy_listener.link.prev == NULL);
> +	assert(destroy_listener.link.next == NULL);
>  
>  	wl_display_destroy(display);
>  	close(s[1]);

Hi Derek,

I have to agree with your analysis on the proposed patch to improve the
wl_signal implementation. It would have been nice to have been less
lazy many years ago, but my timemachine seems to be broken today.

Too bad we can't test reading of "freed" memory, but at least we can
test for writing and dereferencing which you have done here.

Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180222/5cccf1e3/attachment.sig>