[PATCH] client: Introduce functions to allocate and marshal proxies atomically
Bryce W. Harrington
b.harrington at samsung.com
Mon Nov 18 20:50:46 PST 2013
On Fri, Nov 15, 2013 at 08:51:50PM -0800, Kristian Høgsberg 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>
Is there a test case available for checking this problem?
Also, is it a theoretical issue or does it crop up in actual use? If
the latter, then might be worth including a test case in the test
suite.
> ---
> 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
More information about the wayland-devel
mailing list