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" <<a href="mailto:krh@bitplanet.net">krh@bitplanet.net</a>> 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 <<a href="mailto:kalle.vahlman@movial.com">kalle.vahlman@movial.com</a>> > --- > 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) <a href="http://libwayland-util.la">libwayland-util.la</a> -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-><a href="http://object.id">object.id</a> = 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-><a href="http://object.id">object.id</a> = 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 > <a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a>