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

[Derek Foreman]

On 02/12/16 02:07 AM, Pekka Paalanen wrote:
> 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

That is a nice list. :)

Glad I'm not the only one that wants this.

> 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.

I think it would be trivial to catch nullable violation from the 
compositor in the same place where I'm testing for accidental mixing of 
client objects.

I don't think we should remove any client side sanity checks, but adding 
that check on the compositor side seems like a win for debugging to me. 
Should I do this too?

> 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?

Are there actually version numbers that have a place in the signature 
but no associated argument?  My read of get_next_argument left me 
thinking that only ? was present without an argument.

This is really an academic question because...

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

Yes, there's get_next_argument, which I've re-written my patch to use. 
So I think that should handle any appropriate skipping that I may have 
otherwise missed?

>> +
>> +		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).

I'm not sure how to do this.  new_id will just give me a number, and I 
think it would be complicated to ensure that the id is for the correct 
client?

I wouldn't mind putting this down for "further work at a later date". :)

I'll follow up shortly with a new patch.

Thanks,
Derek

>> +			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
>