[PATCH] client: Introduce functions to allocate and marshal proxies atomically
Kristian Høgsberg
krh at bitplanet.net
Fri Nov 15 20:51:50 PST 2013
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
More information about the wayland-devel
mailing list