[PATCH wayland v2] wayland-server: abort instead of posting events with wrong client objects

[Derek Foreman]

On 09/12/16 08:51 AM, Giulio Camuffo wrote:
> 2016-12-08 20:36 GMT+01:00 Derek Foreman <derekf at osg.samsung.com>:
>> 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.
>
> It may be easier, but as an user the compositor crashing is a highly
> disruptive event, and i think we should strive to avoid it at all
> times. By all means, log the problem, get a backtrace and print it,
> maybe, but i don't agree with the abort(). Unless the problem really
> is unrecoverable, but it seems this is not the case. If it was an
> assert(0) it may be better, but i'm not sure i'd like that either.
>

Yeah, Pekka and I have been discussing that further on-list (and off). 
At this point I'm thinking it should just wl_log(), and drop the client 
the same way nullable violation does.

I wouldn't mind a conditional assert on WAYLAND_DEBUG=server or such...

I'm also wondering if other aborts() that happen in wl_closure_marshal() 
should be reconsidered...  I think perhaps they were added with client 
side in mind, without paying attention to the fact that 
wl_closure_marshal() is called from both client and server.

Thanks,
Derek

> Cheers,
> Giulio
>
>>
>> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
>> ---
>>
>> Changes since v1:
>> uses get_next_arguments and arg_count_for_signature in the normal fashion
>> abort instead of assert
>>
>>
>> This does not address Pekka's request for a new_id test, as it's not
>> immediately clear to me how to write it.
>>
>>  src/wayland-server.c | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/src/wayland-server.c b/src/wayland-server.c
>> index 9d7d9c1..429dbef 100644
>> --- a/src/wayland-server.c
>> +++ b/src/wayland-server.c
>> @@ -160,6 +160,33 @@ log_closure(struct wl_resource *resource,
>>         }
>>  }
>>
>> +static void
>> +verify_objects(struct wl_resource *resource, uint32_t opcode,
>> +             union wl_argument *args)
>> +{
>> +       struct wl_object *object = &resource->object;
>> +       const char *signature = object->interface->events[opcode].signature;
>> +       struct argument_details arg;
>> +       struct wl_resource *res;
>> +       int count, i;
>> +
>> +       count = arg_count_for_signature(signature);
>> +       for (i = 0; i < count; i++) {
>> +               signature = get_next_argument(signature, &arg);
>> +               switch (arg.type) {
>> +               case 'o':
>> +                       res = (struct wl_resource *) (args[i].o);
>> +                       if (res && res->client != resource->client) {
>> +                               wl_log("unrecoverable error: The compositor "
>> +                                      "tried to use an object from one "
>> +                                      "client in an event for a different "
>> +                                      "client.\n");
>> +                               abort();
>> +                       }
>> +               }
>> +       }
>> +}
>> +
>>  WL_EXPORT void
>>  wl_resource_post_event_array(struct wl_resource *resource, uint32_t opcode,
>>                              union wl_argument *args)
>> @@ -167,6 +194,7 @@ wl_resource_post_event_array(struct wl_resource *resource, uint32_t opcode,
>>         struct wl_closure *closure;
>>         struct wl_object *object = &resource->object;
>>
>> +       verify_objects(resource, opcode, args);
>>         closure = wl_closure_marshal(object, opcode, args,
>>                                      &object->interface->events[opcode]);
>>
>> @@ -206,6 +234,7 @@ wl_resource_queue_event_array(struct wl_resource *resource, uint32_t opcode,
>>         struct wl_closure *closure;
>>         struct wl_object *object = &resource->object;
>>
>> +       verify_objects(resource, opcode, args);
>>         closure = wl_closure_marshal(object, opcode, args,
>>                                      &object->interface->events[opcode]);
>>
>> --
>> 2.10.2
>>
>> _______________________________________________
>> wayland-devel mailing list
>> wayland-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/wayland-devel