tests: Test for use after free in resource destruction signals

Submitted by Derek Foreman on Feb. 21, 2018, 7:08 p.m.

Details

Message ID 20180221190851.21517-1-derekf@osg.samsung.com
State Accepted
Commit 58ee271bff499b6b0d865fa0126990dc478bff24
Headers show
Series "tests: Test for use after free in resource destruction signals" ( rev: 1 ) in Wayland

Not browsing as part of any series.

Commit Message

Derek Foreman Feb. 21, 2018, 7:08 p.m.
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@osg.samsung.com>
---
 tests/resources-test.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Patch hide | download patch | download mbox

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]);

Comments

On Wed, 21 Feb 2018 13:08:51 -0600
Derek Foreman <derekf@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@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@collabora.co.uk>


Thanks,
pq