[PATCH v4 wayland] wayland-server: log an error for events with wrong client objects

[Bryce Harrington]

On Mon, Dec 12, 2016 at 12:46:30PM -0800, Yong Bakos wrote:
> Hi Derek,
> 
> > On Dec 12, 2016, at 12:39 PM, 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
> > abort with a cryptic message.
> > 
> > Instead the client will now be disconnected as it is when the compositor
> > tries to send a null for a non-nullable object, and a log message
> > will be printed by the compositor.
> > 
> > Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
> 
> Reviewed-by: Yong Bakos <ybakos at humanoriented.com>

Looks like a good approach to flag the potential issue.  And agreed,
logging an error is probably friendlier than forcing users to terminate
their server.

Reviewed-by: Bryce Harrington <bryce at osg.samsung.com>
 
> yong
> 
> 
> > ---
> > src/wayland-server.c | 38 ++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 38 insertions(+)
> > 
> > This started life as an assert, became an abort, and now it's a log
> > and disconnect.  Log and disconnect is already how we manage nullable
> > violation on the compositor side, so I'm hoping it won't be too
> > controversial.
> > 
> > For EFL clients the disconnect is recoverable under some circumstances with
> > our session recovery protocol, but the current client-abort()s behaviour
> > is not.
> > 
> > Changes from v1:
> > uses get_next_arguemnts and arg_count_for_signature instead of a bespoke
> > implementation.
> > 
> > Changes from v2:
> > Tests new_id objects as well
> > logs and disconnects instead of assert()/abort()
> > superficial changes to the log text
> > 
> > Changes from v3:
> > Actually printing the interface name and message name make this
> > useful for debugging - instead of actually less informative
> > than it was with a client side error.
> > 
> > diff --git a/src/wayland-server.c b/src/wayland-server.c
> > index 9d7d9c1..023d427 100644
> > --- a/src/wayland-server.c
> > +++ b/src/wayland-server.c
> > @@ -160,6 +160,36 @@ log_closure(struct wl_resource *resource,
> > 	}
> > }
> > 
> > +static bool
> > +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 'n':
> > +		case 'o':
> > +			res = (struct wl_resource *) (args[i].o);
> > +			if (res && res->client != resource->client) {
> > +				wl_log("compositor bug: The compositor "
> > +				       "tried to use an object from one "
> > +				       "client in a '%s.%s' for a different "
> > +				       "client.\n", object->interface->name,
> > +				       object->interface->events[opcode].name);
> > +				return false;
> > +			}
> > +		}
> > +	}
> > +	return true;
> > +}
> > +
> > WL_EXPORT void
> > wl_resource_post_event_array(struct wl_resource *resource, uint32_t opcode,
> > 			     union wl_argument *args)
> > @@ -167,6 +197,10 @@ wl_resource_post_event_array(struct wl_resource *resource, uint32_t opcode,
> > 	struct wl_closure *closure;
> > 	struct wl_object *object = &resource->object;
> > 
> > +	if (!verify_objects(resource, opcode, args)) {
> > +		resource->client->error = 1;
> > +		return;
> > +	}
> > 	closure = wl_closure_marshal(object, opcode, args,
> > 				     &object->interface->events[opcode]);
> > 
> > @@ -206,6 +240,10 @@ wl_resource_queue_event_array(struct wl_resource *resource, uint32_t opcode,
> > 	struct wl_closure *closure;
> > 	struct wl_object *object = &resource->object;
> > 
> > +	if (!verify_objects(resource, opcode, args)) {
> > +		resource->client->error = 1;
> > +		return;
> > +	}
> > 	closure = wl_closure_marshal(object, opcode, args,
> > 				     &object->interface->events[opcode]);
> > 
> > -- 
> > 2.11.0
> > 
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel