[PATCH] client: Introduce functions to allocate and marshal proxies atomically
Jason Ekstrand
jason at jlekstrand.net
Thu Nov 21 15:13:54 PST 2013
Kristian,
Over-all i think it looks good. I do have a few thoughts:
1) Do we want to deprecate wl_proxy_create and make a note about the race
so we throw warnings in case people (Qt) are doing things themselves?
2) Do we want to add a flag to use the old version on the chance that
someone wants to be able to link against older versions? I'm thinking
that's probably not an issue.
3) might be good to add a note specifying that the marshalling functions
will create and return exactly one proxy if the given interface is non-null
and then enforce that.
Hope that helps.
--Jason Ekstrand
On Nov 15, 2013 10:52 PM, "Kristian Høgsberg" <krh at bitplanet.net> wrote:
>
> The server requires clients to only allocate one ID ahead of the
previously
> highest ID in order to keep the ID range tight. Failure to do so will
> make the server close the client connection. However, the way we allocate
> new IDs is racy. The generated code looks like:
>
> new_proxy = wl_proxy_create(...);
> wl_proxy_marshal(proxy, ... new_proxy, ...);
>
> If two threads do this at the same time, there's a chance that thread A
> will allocate a proxy, then get pre-empted by thread B which then
allocates
> a proxy and then passes it to wl_proxy_marshal(). The ID for thread As
> proxy will be one higher that the currently highest ID, but the ID for
> thread Bs proxy will be two higher. But since thread B prempted thread A
> before it could send its new ID, B will send its new ID first, the server
> will see the ID from thread Bs proxy first, and will reject it.
>
> We fix this by introducing wl_proxy_marshal_constructor(). This
> function is identical to wl_proxy_marshal(), except that it will
> allocate a wl_proxy for NEW_ID arguments and send it, all under the
> display mutex. By introducing a new function, we maintain backwards
> compatibility with older code from the generator, and make sure that
> the new generated code has an explicit dependency on a new enough
> libwayland-client.so.
>
> A virtual Wayland merit badge goes to Kalle Vahlman, who tracked this
> down and analyzed the issue.
>
> Reported-by: Kalle Vahlman <kalle.vahlman at movial.com>
> ---
> src/Makefile.am | 2 +-
> src/scanner.c | 43 ++++++-----
> src/wayland-client.c | 211
++++++++++++++++++++++++++++++++++++++-------------
> src/wayland-client.h | 8 ++
> 4 files changed, 189 insertions(+), 75 deletions(-)
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 4226f63..fb2df1c 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -27,7 +27,7 @@ libwayland_server_la_SOURCES = \
> event-loop.c
>
> libwayland_client_la_LIBADD = $(FFI_LIBS) libwayland-util.la -lrt -lm
> -libwayland_client_la_LDFLAGS = -version-info 1:0:1
> +libwayland_client_la_LDFLAGS = -version-info 2:0:2
> libwayland_client_la_SOURCES = \
> wayland-protocol.c \
> wayland-client.c
> diff --git a/src/scanner.c b/src/scanner.c
> index 9624618..a030181 100644
> --- a/src/scanner.c
> +++ b/src/scanner.c
> @@ -699,31 +699,34 @@ emit_stubs(struct wl_list *message_list, struct
interface *interface)
> "{\n");
> if (ret) {
> printf("\tstruct wl_proxy *%s;\n\n"
> - "\t%s = wl_proxy_create("
> - "(struct wl_proxy *) %s,\n",
> - ret->name, ret->name, interface->name);
> + "\t%s = wl_proxy_marshal_constructor("
> + "(struct wl_proxy *) %s,\n"
> + "\t\t\t %s_%s, ",
> + ret->name, ret->name,
> + interface->name,
> + interface->uppercase_name,
> + m->uppercase_name);
> +
> if (ret->interface_name == NULL)
> - printf("\t\t\t interface);\n");
> + printf("interface");
> else
> - printf("\t\t\t &%s_interface);\n",
> - ret->interface_name);
> -
> - printf("\tif (!%s)\n"
> - "\t\treturn NULL;\n\n",
> - ret->name);
> + printf("&%s_interface",
ret->interface_name);
> + } else {
> + printf("\twl_proxy_marshal((struct wl_proxy *)
%s,\n"
> + "\t\t\t %s_%s",
> + interface->name,
> + interface->uppercase_name,
> + m->uppercase_name);
> }
>
> - printf("\twl_proxy_marshal((struct wl_proxy *) %s,\n"
> - "\t\t\t %s_%s",
> - interface->name,
> - interface->uppercase_name,
> - m->uppercase_name);
> -
> wl_list_for_each(a, &m->arg_list, link) {
> - if (a->type == NEW_ID && a->interface_name ==
NULL)
> - printf(", interface->name, version");
> - printf(", ");
> - printf("%s", a->name);
> + if (a->type == NEW_ID) {
> + if (a->interface_name == NULL)
> + printf(", interface->name,
version");
> + printf(", NULL");
> + } else {
> + printf(", %s", a->name);
> + }
> }
> printf(");\n");
>
> diff --git a/src/wayland-client.c b/src/wayland-client.c
> index e92317a..1486b73 100644
> --- a/src/wayland-client.c
> +++ b/src/wayland-client.c
> @@ -194,6 +194,29 @@ wl_display_create_queue(struct wl_display *display)
> return queue;
> }
>
> +static struct wl_proxy *
> +proxy_create(struct wl_proxy *factory, const struct wl_interface
*interface)
> +{
> + struct wl_proxy *proxy;
> + struct wl_display *display = factory->display;
> +
> + proxy = malloc(sizeof *proxy);
> + if (proxy == NULL)
> + return NULL;
> +
> + proxy->object.interface = interface;
> + proxy->object.implementation = NULL;
> + proxy->dispatcher = NULL;
> + proxy->display = display;
> + proxy->queue = factory->queue;
> + proxy->flags = 0;
> + proxy->refcount = 1;
> +
> + proxy->object.id = wl_map_insert_new(&display->objects, 0, proxy);
> +
> + return proxy;
> +}
> +
> /** Create a proxy object with a given interface
> *
> * \param factory Factory proxy object
> @@ -216,23 +239,11 @@ wl_display_create_queue(struct wl_display *display)
> WL_EXPORT struct wl_proxy *
> wl_proxy_create(struct wl_proxy *factory, const struct wl_interface
*interface)
> {
> - struct wl_proxy *proxy;
> struct wl_display *display = factory->display;
> -
> - proxy = malloc(sizeof *proxy);
> - if (proxy == NULL)
> - return NULL;
> -
> - proxy->object.interface = interface;
> - proxy->object.implementation = NULL;
> - proxy->dispatcher = NULL;
> - proxy->display = display;
> - proxy->queue = factory->queue;
> - proxy->flags = 0;
> - proxy->refcount = 1;
> + struct wl_proxy *proxy;
>
> pthread_mutex_lock(&display->mutex);
> - proxy->object.id = wl_map_insert_new(&display->objects, 0, proxy);
> + proxy = proxy_create(factory, interface);
> pthread_mutex_unlock(&display->mutex);
>
> return proxy;
> @@ -382,27 +393,107 @@ wl_proxy_add_dispatcher(struct wl_proxy *proxy,
> return 0;
> }
>
> +static struct wl_proxy *
> +create_outgoing_proxy(struct wl_proxy *proxy, const struct wl_message
*message,
> + union wl_argument *args,
> + const struct wl_interface *interface)
> +{
> + int i, count;
> + const char *signature;
> + struct argument_details arg;
> + struct wl_proxy *new_proxy = NULL;
> +
> + signature = message->signature;
> + count = arg_count_for_signature(signature);
> + for (i = 0; i < count; i++) {
> + signature = get_next_argument(signature, &arg);
> +
> + switch (arg.type) {
> + case 'n':
> + new_proxy = proxy_create(proxy, interface);
> + if (new_proxy == NULL)
> + return NULL;
> +
> + args[i].o = &new_proxy->object;
> + break;
> + }
> + }
> +
> + return new_proxy;
> +}
> +
> /** Prepare a request to be sent to the compositor
> *
> * \param proxy The proxy object
> * \param opcode Opcode of the request to be sent
> - * \param ... Extra arguments for the given request
> + * \param args Extra arguments for the given request
> + * \param iterface The interface to use for the new proxy
> *
> * Translates the request given by opcode and the extra arguments into
the
> - * wire format and write it to the connection buffer.
> + * wire format and write it to the connection buffer. This version
takes an
> + * array of the union type wl_argument.
> *
> - * The example below creates a proxy object with the wl_surface_interface
> - * using a wl_compositor factory interface and sends the
> - * \c compositor.create_surface request using \ref wl_proxy_marshal().
Note
> - * the \c id is the extra argument to the request as specified by the
> - * protocol.
> + * For new-id arguments, this function will allocate a new wl_proxy
> + * and send the ID to the server. The new wl_proxy will be returned
> + * on success or NULL on errror with errno set accordingly.
> + *
> + * \note This is intended to be used by language bindings and not in
> + * non-generated code.
> *
> - * \code
> - * id = wl_proxy_create((struct wl_proxy *) wl_compositor,
> - * &wl_surface_interface);
> - * wl_proxy_marshal((struct wl_proxy *) wl_compositor,
> - * WL_COMPOSITOR_CREATE_SURFACE, id);
> - * \endcode
> + * \sa wl_proxy_marshal()
> + *
> + * \memberof wl_proxy
> + */
> +WL_EXPORT struct wl_proxy *
> +wl_proxy_marshal_array_constructor(struct wl_proxy *proxy,
> + uint32_t opcode, union wl_argument
*args,
> + const struct wl_interface *interface)
> +{
> + struct wl_closure *closure;
> + struct wl_proxy *new_proxy = NULL;
> + const struct wl_message *message;
> +
> + pthread_mutex_lock(&proxy->display->mutex);
> +
> + message = &proxy->object.interface->methods[opcode];
> + if (interface) {
> + new_proxy = create_outgoing_proxy(proxy, message,
> + args, interface);
> + if (new_proxy == NULL)
> + goto err_unlock;
> + }
> +
> + closure = wl_closure_marshal(&proxy->object, opcode, args,
message);
> + if (closure == NULL) {
> + fprintf(stderr, "Error marshalling request\n");
> + abort();
> + }
> +
> + if (wl_debug)
> + wl_closure_print(closure, &proxy->object, true);
> +
> + if (wl_closure_send(closure, proxy->display->connection)) {
> + fprintf(stderr, "Error sending request: %m\n");
> + abort();
> + }
> +
> + wl_closure_destroy(closure);
> +
> + err_unlock:
> + pthread_mutex_unlock(&proxy->display->mutex);
> +
> + return new_proxy;
> +}
> +
> +
> +/** Prepare a request to be sent to the compositor
> + *
> + * \param proxy The proxy object
> + * \param opcode Opcode of the request to be sent
> + * \param ... Extra arguments for the given request
> + *
> + * This function is similar to wl_proxy_marshal_constructor(), except
> + * it doesn't create proxies for new-id arguments.
> *
> * \note This should not normally be used by non-generated code.
> *
> @@ -421,18 +512,52 @@ wl_proxy_marshal(struct wl_proxy *proxy, uint32_t
opcode, ...)
> args, WL_CLOSURE_MAX_ARGS, ap);
> va_end(ap);
>
> - wl_proxy_marshal_array(proxy, opcode, args);
> + wl_proxy_marshal_array_constructor(proxy, opcode, args, NULL);
> }
>
> /** Prepare a request to be sent to the compositor
> *
> * \param proxy The proxy object
> * \param opcode Opcode of the request to be sent
> - * \param args Extra arguments for the given request
> + * \param iterface The interface to use for the new proxy
> + * \param ... Extra arguments for the given request
> + * \return A new wl_proxy for the new_id argument or NULL on error
> *
> * Translates the request given by opcode and the extra arguments into
the
> - * wire format and write it to the connection buffer. This version
takes an
> - * array of the union type wl_argument.
> + * wire format and write it to the connection buffer.
> + *
> + * For new-id arguments, this function will allocate a new wl_proxy
> + * and send the ID to the server. The new wl_proxy will be returned
> + * on success or NULL on errror with errno set accordingly.
> + *
> + * \note This should not normally be used by non-generated code.
> + *
> + * \memberof wl_proxy
> + */
> +WL_EXPORT struct wl_proxy *
> +wl_proxy_marshal_constructor(struct wl_proxy *proxy, uint32_t opcode,
> + const struct wl_interface *interface, ...)
> +{
> + union wl_argument args[WL_CLOSURE_MAX_ARGS];
> + va_list ap;
> +
> + va_start(ap, interface);
> +
wl_argument_from_va_list(proxy->object.interface->methods[opcode].signature,
> + args, WL_CLOSURE_MAX_ARGS, ap);
> + va_end(ap);
> +
> + return wl_proxy_marshal_array_constructor(proxy, opcode,
> + args, interface);
> +}
> +
> +/** Prepare a request to be sent to the compositor
> + *
> + * \param proxy The proxy object
> + * \param opcode Opcode of the request to be sent
> + * \param args Extra arguments for the given request
> + *
> + * This function is similar to wl_proxy_marshal_array_constructor(),
except
> + * it doesn't create proxies for new-id arguments.
> *
> * \note This is intended to be used by language bindings and not in
> * non-generated code.
> @@ -445,29 +570,7 @@ WL_EXPORT void
> wl_proxy_marshal_array(struct wl_proxy *proxy, uint32_t opcode,
> union wl_argument *args)
> {
> - struct wl_closure *closure;
> -
> - pthread_mutex_lock(&proxy->display->mutex);
> -
> - closure = wl_closure_marshal(&proxy->object, opcode, args,
> -
&proxy->object.interface->methods[opcode]);
> -
> - if (closure == NULL) {
> - fprintf(stderr, "Error marshalling request\n");
> - abort();
> - }
> -
> - if (wl_debug)
> - wl_closure_print(closure, &proxy->object, true);
> -
> - if (wl_closure_send(closure, proxy->display->connection)) {
> - fprintf(stderr, "Error sending request: %m\n");
> - abort();
> - }
> -
> - wl_closure_destroy(closure);
> -
> - pthread_mutex_unlock(&proxy->display->mutex);
> + wl_proxy_marshal_array_constructor(proxy, opcode, args, NULL);
> }
>
> static void
> diff --git a/src/wayland-client.h b/src/wayland-client.h
> index 43ba3fc..2a32785 100644
> --- a/src/wayland-client.h
> +++ b/src/wayland-client.h
> @@ -126,6 +126,14 @@ void wl_proxy_marshal_array(struct wl_proxy *p,
uint32_t opcode,
> union wl_argument *args);
> struct wl_proxy *wl_proxy_create(struct wl_proxy *factory,
> const struct wl_interface *interface);
> +struct wl_proxy *wl_proxy_marshal_constructor(struct wl_proxy *proxy,
> + uint32_t opcode,
> + const struct wl_interface
*interface,
> + ...);
> +struct wl_proxy *
> +wl_proxy_marshal_array_constructor(struct wl_proxy *proxy,
> + uint32_t opcode, union wl_argument
*args,
> + const struct wl_interface *interface);
>
> void wl_proxy_destroy(struct wl_proxy *proxy);
> int wl_proxy_add_listener(struct wl_proxy *proxy,
> --
> 1.8.3.1
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20131121/65f1ea30/attachment-0001.html>
More information about the wayland-devel
mailing list