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

[Yong Bakos]

Hi Derek,


> On Dec 8, 2016, at 11:36 AM, 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>
> ---
> 
> 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;

I know that `res` is used throughout the source, but since there are
two resources here, it might be nice to add some semantics, eg. `event_resource`
or `argument_resource`.


> +	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();

This should probably wl_abort, right?

Regards,
yong


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