[PATCH wayland 2/4 v2] tests/queue-test: Add tests for proxy wrappers
Pekka Paalanen
ppaalanen at gmail.com
Wed Apr 27 10:36:08 UTC 2016
On Wed, 27 Apr 2016 15:37:39 +0800
Jonas Ådahl <jadahl at gmail.com> wrote:
> Test that doing wl_display.sync on a wrapped proxy with a special queue
> works as expected.
>
> Test that creating a wrapper on a destroyed but not freed proxy fails.
>
> Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> ---
>
> Changes since v1:
>
> - Adepted to the API changes
> - Added a test for ensuring that a wrapper for a destroyed proxy isn't
> created
Hi Jonas,
what is the reason to actually use threads? Does it buy us something
that manually writing a single-threaded example with exactly the
problematic sequence does not?
We could simply say in comments which call is imagined to happen in
which thread, and we'd get the sequence guaranteed correct without any
mutexes, cond-variables, yielding, or anything.
It looks like you are running completely synchronous anyway, the code
is just harder to follow when threaded.
> tests/queue-test.c | 206 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 206 insertions(+)
>
> diff --git a/tests/queue-test.c b/tests/queue-test.c
> index 02865ae..917632c 100644
> --- a/tests/queue-test.c
> +++ b/tests/queue-test.c
> @@ -23,6 +23,8 @@
> * SOFTWARE.
> */
>
> +#define _GNU_SOURCE /* Needed for pthread_yield() */
Instead of this, I think we should have AC_USE_SYSTEM_EXTENSIONS in
configure.ac.
> +
> #include <stdlib.h>
> #include <stdio.h>
> #include <stdbool.h>
> @@ -30,6 +32,8 @@
> #include <sys/types.h>
> #include <sys/wait.h>
> #include <assert.h>
> +#include <poll.h>
> +#include <pthread.h>
>
> #include "wayland-client.h"
> #include "wayland-server.h"
> @@ -204,6 +208,184 @@ client_test_queue_roundtrip(void)
> wl_display_disconnect(display);
> }
>
> +struct thread_info {
> + struct wl_display *display;
> + pthread_mutex_t mutex;
> + pthread_cond_t cond;
> + bool run;
> + bool dispatch_roundtrip_done;
> +};
> +
> +static void *
> +dispatcher_thread_run(void *arg)
> +{
> + struct thread_info *info = arg;
> + struct pollfd pfd;
> + struct wl_callback *callback;
> + int ret;
> +
> + pfd.fd = wl_display_get_fd(info->display);
> + pfd.events = POLLIN;
> +
> + /* Simple command "queue" that does a round trip every time its woken
> + * up until its terminated (by setting info->run to false).
> + */
> + pthread_mutex_lock(&info->mutex);
> + do {
> + pthread_cond_wait(&info->cond, &info->mutex);
> + if (!info->run)
> + break;
> +
> + assert(!info->dispatch_roundtrip_done);
> + callback = wl_display_sync(info->display);
> + wl_callback_add_listener(callback,
> + &sync_listener_roundtrip,
> + &info->dispatch_roundtrip_done);
> + wl_display_flush(info->display);
> + while (true) {
> + while (true) {
> + ret = wl_display_dispatch_pending(info->display);
> + assert(ret >= 0);
> + if (ret == 0)
> + break;
> + }
> +
> + if (info->dispatch_roundtrip_done)
> + break;
> +
> + assert(wl_display_prepare_read(info->display) == 0);
This prepare-read is written quite differently than in
client_test_queue_set_queue_race().
> + assert(poll(&pfd, 1, -1) == 1);
> + assert(wl_display_read_events(info->display) == 0);
> + }
> + wl_callback_destroy(callback);
> + } while (true);
> + pthread_mutex_unlock(&info->mutex);
> +
> + return NULL;
> +}
> +
> +static void
> +wait_for_dispatch_thread_roundtrip(struct thread_info *info)
> +{
> + pthread_mutex_lock(&info->mutex);
> + info->dispatch_roundtrip_done = false;
> + pthread_cond_signal(&info->cond);
> + pthread_mutex_unlock(&info->mutex);
> + pthread_yield();
> +
> + while (true) {
> + pthread_mutex_lock(&info->mutex);
> + if (info->dispatch_roundtrip_done) {
> + pthread_mutex_unlock(&info->mutex);
> + break;
> + }
> + pthread_cond_signal(&info->cond);
> + pthread_mutex_unlock(&info->mutex);
> + }
> +}
> +
> +static void
> +client_test_queue_set_queue_race(void)
> +{
> + DISABLE_LEAK_CHECKS;
> +
> + struct wl_event_queue *queue;
> + struct wl_display *display;
> + struct wl_display *display_wrapper;
> + struct thread_info info;
> + pthread_t dispatcher_thread;
> + struct wl_callback *callback;
> + bool done = false;
> + struct pollfd pfd;
> +
> + display = wl_display_connect(NULL);
> + assert(display);
> +
> + info = (struct thread_info) {
> + .display = display,
> + .run = true,
> + .mutex = PTHREAD_MUTEX_INITIALIZER,
> + .cond = PTHREAD_COND_INITIALIZER,
> + };
> + assert(pthread_create(&dispatcher_thread,
> + NULL,
> + dispatcher_thread_run,
> + &info) == 0);
> +
> + queue = wl_display_create_queue(display);
> + assert(queue);
> +
> + display_wrapper = wl_proxy_create_wrapper(display);
> + assert(display_wrapper);
> + wl_proxy_set_queue((struct wl_proxy *) display_wrapper, queue);
> + callback = wl_display_sync(display_wrapper);
> + wl_proxy_wrapper_destroy(display_wrapper);
> + assert(callback != NULL);
> +
> + /* Check that the dispatcher thread didn't dispatch the event. */
> + wait_for_dispatch_thread_roundtrip(&info);
> +
> + wl_callback_add_listener(callback, &sync_listener_roundtrip, &done);
> + wl_display_flush(display);
> +
> + assert(!done);
> +
> + pfd.fd = wl_display_get_fd(info.display);
> + pfd.events = POLLIN;
> +
> + while (true) {
> + while (wl_display_prepare_read_queue(display, queue) != 0)
> + wl_display_dispatch_queue_pending(display, queue);
Missing failure check, but I suppose the test timeout will deal with
that.
> +
> + if (done)
> + break;
> +
> + assert(poll(&pfd, 1, -1) == 1);
> + assert(wl_display_read_events(display) == 0);
> + }
Missing wl_display_cancel_read()? Not that it'd make a difference.
> +
> + wl_callback_destroy(callback);
> + wl_event_queue_destroy(queue);
> +
> + info.run = false;
> + pthread_mutex_lock(&info.mutex);
> + pthread_cond_signal(&info.cond);
> + pthread_mutex_unlock(&info.mutex);
> + pthread_join(dispatcher_thread, NULL);
> +
> + wl_display_disconnect(display);
I stared at this test code long and hard, and it seems like it should
work.
Should we also have a negative test for the old API to show how it does
fail with the problematic sequence? Yeah, it would just test that the
old API indeed has the problem, but I think it would make a nice
reference for explaining people how things can go wrong.
> +}
> +
> +static void
> +client_test_queue_wrap_destroyed_proxy(void)
> +{
> + struct wl_display *display;
> + struct wl_event_queue *queue;
> + struct wl_callback *callback;
> + struct wl_callback *callback_wrapper;
> +
> + display = wl_display_connect(NULL);
> + assert(display);
> +
> + queue = wl_display_create_queue(display);
> + assert(queue);
> +
> + callback = wl_display_sync(display);
> + assert(callback);
> + wl_proxy_set_queue((struct wl_proxy *) callback, queue);
> +
> + wl_display_roundtrip(display);
> +
> + /* The callback will now be queued in 'queue'. */
> + wl_callback_destroy(callback);
> +
> + callback_wrapper = wl_proxy_create_wrapper(callback);
You just called wl_callback_destroy() and now you still use 'callback'.
That is very sketchy to do, even if you documented that you rely on
'queue' holding a reference. Normally this would be a use-after-free
bug.
How about doing it differently, since wl_callback.done event
auto-destructs the protocol object, which should be practically
equivalent?
No need for 'queue', and do not wl_callback_destroy() before
wl_proxy_create_wrapper():
cb = wl_display_sync();
wl_display_rountrip();
Now 'cb' refers to a deleted protocol object, so
wl_proxy_create_wrapper() on it would fail. No?
Or is this use-after-free case somehow important to test also?
I still claim, that if that code sequence ever happens, then the user
has a bug, and libwayland-client has no obligation to work for it.
Later we could even add a poison flag to wl_proxy saying that any ABI
calls from the user to this destroyed but still held unfreed by internal
references proxy are programmer errors, and abort(), while any use
after a real free() errors are discoverable with Valgrind.
> + assert(!callback_wrapper);
> +
> + wl_event_queue_destroy(queue);
> + wl_display_disconnect(display);
> +}
> +
> static void
> dummy_bind(struct wl_client *client,
> void *data, uint32_t version, uint32_t id)
> @@ -259,3 +441,27 @@ TEST(queue_roundtrip)
>
> display_destroy(d);
> }
> +
> +TEST(queue_set_queue_race)
> +{
> + struct display *d = display_create();
> +
> + test_set_timeout(2);
> +
> + client_create_noarg(d, client_test_queue_set_queue_race);
> + display_run(d);
> +
> + display_destroy(d);
> +}
> +
> +TEST(queue_wrap_destroyed_proxy)
> +{
> + struct display *d = display_create();
> +
> + test_set_timeout(2);
> +
> + client_create_noarg(d, client_test_queue_wrap_destroyed_proxy);
> + display_run(d);
> +
> + display_destroy(d);
> +}
Awesome to have these tests.
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160427/1833105f/attachment.sig>
More information about the wayland-devel
mailing list