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