[PATCH wayland] tests: add a test for mixing up client resources in an event
Pekka Paalanen
ppaalanen at gmail.com
Wed Jan 25 15:13:20 UTC 2017
On Mon, 23 Jan 2017 11:49:24 -0600
Derek Foreman <derekf at osg.samsung.com> wrote:
> Tests that the compositor is prevented from sending a resource id
> from one client in an event to another client.
>
> This tests the (rare in practice) subset of the problem where both
> clients magically happen to have the same kind of object with the
> same id, so the client has no way to know it's being sent garbage.
>
> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
> ---
> tests/display-test.c | 242 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 242 insertions(+)
>
> This is a follow up to https://patchwork.freedesktop.org/patch/127077/
> (log an error for events with wrong client objects)
> The test fails with current git master, but passes with patch 127077
> applied.
Hi,
display-test.c is getting rather long. Could there be a new category of
tests? If it's appropriate here, then no worries.
> diff --git a/tests/display-test.c b/tests/display-test.c
> index 0c4df16..599db38 100644
> --- a/tests/display-test.c
> +++ b/tests/display-test.c
> @@ -926,3 +926,245 @@ TEST(error_on_destroyed_object)
> display_resume(d);
> display_destroy(d);
> }
> +
> +struct mixed_client_server_data {
> + struct display *d;
> + struct client_info *success_client;
> + struct wl_resource *success_mc;
> + struct wl_resource *success_cb;
> + struct client_info *fail_client;
> + struct wl_resource *fail_mc;
> + struct wl_resource *failure_cb;
> +};
> +
> +struct mixed_client_client_data {
> + struct wl_proxy *mc;
> + struct wl_callback *callback;
> + bool done;
> + bool should_fail;
> +};
> +
> +static const struct wl_interface *mixed_client_types[] = {
> + &wl_callback_interface,
> +};
> +
> +static const struct wl_message mixed_client_requests[] = {
> + { "callback", "n", mixed_client_types + 0 },
> +};
> +
> +static const struct wl_message mixed_client_events[] = {
> + { "right_client", "o", mixed_client_types + 0 },
> + { "wrong_client", "o", mixed_client_types + 0 },
> +};
> +
> +static const struct wl_interface mixed_client_interface = {
> + "mixed_client", 2,
> + 1, mixed_client_requests,
> + 2, mixed_client_events,
> +};
How about just writing the few lines of XML to describe this and run it
through the scanner instead?
I don't see any reason to have hand-crafted code here.
> +
> +static void
> +mixed_client_cb_done(void *data, struct wl_callback *callback,
> + uint32_t unused)
> +{
> + wl_callback_destroy(callback);
> +}
> +
> +static const struct wl_callback_listener mixed_client_cb_listener = {
> + mixed_client_cb_done
> +};
> +
> +static void
> +mixed_client_success(void *data, struct wl_proxy *mixed_client,
> + struct wl_callback *callback)
> +{
> + struct mixed_client_client_data *client_data = data;
> +
> + client_data->done = true;
> +}
> +
> +static void
> +mixed_client_failure(void *data, struct wl_proxy *mixed_client,
> + struct wl_callback *callback)
> +{
> + assert(false);
> +}
> +
> +static const struct {
> + void (*right_client)(void *data, struct wl_proxy *mixed_client,
> + struct wl_callback *new_callback);
> + void (*wrong_client)(void *data, struct wl_proxy *mixed_client,
> + struct wl_callback *broken_callback);
> +} mixed_client_listener = {
> + .right_client = mixed_client_success,
> + .wrong_client = mixed_client_failure,
> +};
> +
> +static void
> +mixed_client_registry_global(void *data, struct wl_registry *registry,
> + uint32_t id, const char *interface,
> + uint32_t version)
> +{
> + struct mixed_client_client_data *client_data = data;
> +
> + if (strcmp(interface, "mixed_client") != 0)
> + return;
> +
> + client_data->mc = wl_registry_bind(registry, id,
> + &mixed_client_interface,
> + client_data->should_fail + 1);
should_fail + 1, err, what?
> + wl_proxy_add_listener(client_data->mc,
> + (void (**)(void)) &mixed_client_listener,
> + data);
> +}
> +
> +static const struct wl_registry_listener mixed_client_registry_listener = {
> + mixed_client_registry_global,
> + NULL,
> +};
> +
> +static void
> +mixed_client_test(void *data)
> +{
> + struct client *client = client_connect();
> + bool should_fail = (bool) (intptr_t) data;
> + struct mixed_client_client_data client_data = {
> + .mc = NULL,
> + .done = false,
> + .should_fail = should_fail,
> + };
> + struct wl_registry *registry;
> + int ret;
> +
> + registry = wl_display_get_registry(client->wl_display);
> + wl_registry_add_listener(registry, &mixed_client_registry_listener,
> + &client_data);
> + wl_display_roundtrip(client->wl_display);
> + wl_registry_destroy(registry);
> + assert(client_data.mc);
> +
> + client_data.callback =
> + (struct wl_callback *)
> + wl_proxy_marshal_constructor(client_data.mc, 0,
> + &wl_callback_interface,
> + NULL);
> + wl_callback_add_listener(client_data.callback,
> + &mixed_client_cb_listener,
> + &client_data);
> +
> + while (!client_data.done && ret >= 0)
'ret' used uninitialized?
> + ret = wl_display_roundtrip(client->wl_display);
> +
> + wl_proxy_destroy(client_data.mc);
> + if (should_fail) {
> + assert(ret == -1);
> + assert(wl_display_get_error(client->wl_display) != 0);
> + assert(!client_data.done);
> + } else {
> + /* We never send a callback to the client that shouldn't
> + * fail, so we have to clean it up here */
> + wl_callback_destroy(client_data.callback);
> + assert(ret > 0);
> + assert(wl_display_get_error(client->wl_display) == 0);
> + assert(client_data.done);
> + }
> +
> + client_disconnect_nocheck(client);
> +}
> +
> +static void
> +mixed_client_set_callback(struct wl_client *client,
> + struct wl_resource *resource,
> + uint32_t callback_id)
> +{
> + struct mixed_client_server_data *mc =
> + wl_resource_get_user_data(resource);
> + bool should_fail = (mc->fail_client->wl_client == client);
> +
> + if (!should_fail) {
> + mc->success_cb = wl_resource_create(client,
> + &wl_callback_interface,
> + 1,
> + callback_id);
> + assert(mc->success_cb);
> + } else {
> + mc->failure_cb = wl_resource_create(client,
> + &wl_callback_interface,
> + 1,
> + callback_id);
> + assert(mc->failure_cb);
> + }
> +
> + if (!mc->fail_mc || !mc->success_mc)
> + return;
> +
> + wl_resource_post_event(mc->success_mc, 0, mc->success_cb);
> + /* This lets the failing client clean up its callback so there's no
> + * leaked memory allocations. It also means the mixed-client
> + * event will be on a zombie object, but that's still a valid test.
> + */
> + wl_callback_send_done(mc->failure_cb, 1);
> + wl_resource_post_event(mc->fail_mc, 1, mc->success_cb);
> +}
> +
> +static const struct {
> + void (*callback)(struct wl_client *client,
> + struct wl_resource *resource,
> + uint32_t callback_id);
> +} mixed_client_implementation = {
> + mixed_client_set_callback,
> +};
> +
> +static void
> +bind_mixed_client(struct wl_client *client, void *data,
> + uint32_t vers, uint32_t id)
> +{
> + struct mixed_client_server_data *mc = data;
> + struct display *d = mc->d;
> + struct client_info *ci;
> + struct wl_resource *res;
> + bool should_fail = (mc->fail_client->wl_client == client);
> +
> + ci = find_client_info(d, client);
> + assert(ci);
> +
> + res = wl_resource_create(client, &mixed_client_interface, vers, id);
> + assert(res);
> + wl_resource_set_implementation(res, &mixed_client_implementation,
> + mc, NULL);
> +
> + if (!should_fail)
> + mc->success_mc = res;
> + else
> + mc->fail_mc = res;
> +}
> +
> +TEST(error_on_mixed_client_resource)
> +{
> + struct client_info *c1, *c2;
> + struct mixed_client_server_data mc;
> + struct wl_global *global;
> +
> + memset(&mc, 0, sizeof(mc));
> +
> + mc.d = display_create();
> + assert(mc.d);
> +
> + global = wl_global_create(mc.d->wl_display, &mixed_client_interface,
> + 2, &mc, bind_mixed_client);
> + assert(global);
> +
> + c1 = client_create(mc.d, mixed_client_test, (void *)(intptr_t) false);
> + assert(c1);
> + mc.success_client = c1;
> + c2 = client_create(mc.d, mixed_client_test, (void *)(intptr_t) true);
> + assert(c2);
> + mc.fail_client = c2;
> +
> + display_run(mc.d);
> +
> + assert(mc.success_mc && mc.fail_mc && mc.success_cb && mc.failure_cb);
> +
> + wl_global_destroy(global);
> + display_destroy(mc.d);
> +}
Ok, I didn't get to the bottom of this today. I find that calling the
interface 'mixed_client' is quite confusing. There are 4 times more
'client' than 'server' and I found it hard to keep track what was what.
Am I just tired, or do you think the code could be clarified a bit?
The underlying idea is good, and I do see in the test logs that the
error message added earlier in wayland-server is working.
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170125/4e575d98/attachment-0001.sig>
More information about the wayland-devel
mailing list