[RFC wayland] wayland-server: assert instead of posting events with wrong client objects

[Pekka Paalanen]

On Thu,  1 Dec 2016 17:19:41 -0600
Derek Foreman <derekf at osg.samsung.com> wrote:

> Check that all the objects in an event belong to the same client as
> the resource posting it.  This prevents a compositor from accidentally
> mixing client objects and posting an event that causes a client to
> kill itself.
> 
> It's intended that the compositor killing itself be easier to debug
> than the client killing itself for something that it's completely
> innocent of.
> 
> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
> ---
> 
> Recently caught a bug in Enlightenment where we were posting keyboard enter
> events with a surface from another client.  In most cases this results in
> the client disconnecting itself due to the target object being the wrong type.
> It might be more hilarious if the types just magically happened to match.
> 
> Are we interested in this level of footgun protection?
> 
> It's also been suggested that this might be better if conditional on
> WAYLAND_DEBUG.

Hi Derek,

this is something I have been wanting for a long time. So far I only
wrote down what we should have with libwayland-client:
https://phabricator.freedesktop.org/T50

IMO, your check should not be an assert, it should be an unconditional
abort() with no way to disable it and a clear log message. Like a
segfault is, only better.

A compositor that handles SEGVs and ABRTs will then also provide a
backtrace that should be very helpful in squashing the bug.

However, it seems our existing convention is to just kill the client on
compositor programming errors (e.g. wl_closure_marshal() failing on
nullable violation). I don't really like that, but it's a precedent and
the very least we can detect the problem and yell rather than have
clients mysteriously fail.

libwayland-client does call wl_abort() on such failures.

>  src/wayland-server.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index 9d7d9c1..bf1719a 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -160,6 +160,27 @@ log_closure(struct wl_resource *resource,
>  	}
>  }
>  
> +static bool
> +clients_match(struct wl_resource *resource, uint32_t opcode,
> +	      union wl_argument *args)
> +{
> +	struct wl_object *object = &resource->object;
> +	const char *sig = object->interface->events[opcode].signature;
> +
> +	for (;*sig;sig++) {
> +		if (*sig == '?')
> +			continue;

How about skipping version numbers as well?

I believe there should be similar parsing functions elsewhere to either
follow or even reuse/refactor.

> +
> +		if (*sig == 'o') {

I think you would also want to handle new_id args. Events with new_id
are rare, but they do exist (wl_data_device.data_offer).

> +			struct wl_resource *res = (struct wl_resource *) (args->o);
> +			if (res && res->client != resource->client)
> +				return false;
> +		}
> +		args++;
> +	}
> +	return true;
> +}
> +
>  WL_EXPORT void
>  wl_resource_post_event_array(struct wl_resource *resource, uint32_t opcode,
>  			     union wl_argument *args)
> @@ -167,6 +188,7 @@ wl_resource_post_event_array(struct wl_resource *resource, uint32_t opcode,
>  	struct wl_closure *closure;
>  	struct wl_object *object = &resource->object;
>  
> +	assert(clients_match(resource, opcode, args));
>  	closure = wl_closure_marshal(object, opcode, args,
>  				     &object->interface->events[opcode]);
>  
> @@ -206,6 +228,7 @@ wl_resource_queue_event_array(struct wl_resource *resource, uint32_t opcode,
>  	struct wl_closure *closure;
>  	struct wl_object *object = &resource->object;
>  
> +	assert(clients_match(resource, opcode, args));
>  	closure = wl_closure_marshal(object, opcode, args,
>  				     &object->interface->events[opcode]);
>  

A good idea, all in all.


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/20161202/343fbfee/attachment.sig>