[PATCH wayland] client: Don't segfault when receiving error on destroyed object

[Bryce Harrington]

On Mon, Feb 22, 2016 at 02:34:57PM +0100, Marek Chalupa wrote:
> Hi,
> 
> can confirm the segfault, tested it (will send the test I used for
> it as a follow-up). The only API change problem could be in
> returning
> NULL as the interface - if the user does not check for it, he/she
> dereferences NULL. But I don't think anybody (except us in tests) is
> using wl_display_get_protocol_error() for the error analysis, so IMO
> its OK break.
> 
> Reviewed-by: Marek Chalupa <mchqwerty at gmail.com>

I agree, given that any client in this particular situation would have
been crashing, I don't think anything would be dependent on interface
being non-NULL.

Reviewed-by: Bryce Harrington <bryce at osg.samsung.com>
 
> Cheers,
> Marek
> 
> On 02/22/16 06:37, Jonas Ådahl wrote:
> >If an error is received on a destroyed object, we'd get NULL passed
> >to display_handle_error() instead of a pointer to a valid wl_proxy.
> >
> >The logging is changed to report [unknown interface] and [unknown id]
> >instead of the actual interface name and id.
> >
> >The wl_display_get_protocol_error() documentation is updated to handle
> >the situation. For when the proxy was NULL, the object id 0 and
> >interface NULL is written.
> >
> >Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> >---
> >
> >This is technically an API change, but I see no less breaking change.
> >Considering that clients would segfault before ever reaching here without this
> >patch, maybe it's an Ok break.
> >
> >
> >Jonas
> >
> >
> >  src/wayland-client.c | 32 ++++++++++++++++++++++++--------
> >  1 file changed, 24 insertions(+), 8 deletions(-)
> >
> >diff --git a/src/wayland-client.c b/src/wayland-client.c
> >index ef12bfc..87fc0e4 100644
> >--- a/src/wayland-client.c
> >+++ b/src/wayland-client.c
> >@@ -177,7 +177,7 @@ display_protocol_error(struct wl_display *display, uint32_t code,
> >  		return;
> >
> >  	/* set correct errno */
> >-	if (wl_interface_equal(intf, &wl_display_interface)) {
> >+	if (intf && wl_interface_equal(intf, &wl_display_interface)) {
> >  		switch (code) {
> >  		case WL_DISPLAY_ERROR_INVALID_OBJECT:
> >  		case WL_DISPLAY_ERROR_INVALID_METHOD:
> >@@ -790,12 +790,26 @@ display_handle_error(void *data,
> >  		     uint32_t code, const char *message)
> >  {
> >  	struct wl_proxy *proxy = object;
> >+	uint32_t object_id;
> >+	const struct wl_interface *interface;
> >
> >-	wl_log("%s@%u: error %d: %s\n",
> >-	       proxy->object.interface->name, proxy->object.id, code, message);
> >+	if (proxy) {
> >+		wl_log("%s@%u: error %d: %s\n",
> >+		       proxy->object.interface->name,
> >+		       proxy->object.id,
> >+		       code, message);
> >
> >-	display_protocol_error(display, code, proxy->object.id,
> >-			       proxy->object.interface);
> >+		object_id = proxy->object.id;
> >+		interface = proxy->object.interface;
> >+	} else {
> >+		wl_log("[unknown interface]@[unknown id]: error %d: %s\n",
> >+		       code, message);
> >+
> >+		object_id = 0;
> >+		interface = NULL;
> >+	}
> >+
> >+	display_protocol_error(display, code, object_id, interface);
> >  }
> >
> >  static void
> >@@ -1756,10 +1770,12 @@ wl_display_get_error(struct wl_display *display)
> >  /** Retrieves the information about a protocol error:
> >   *
> >   * \param display    The Wayland display
> >- * \param interface  if not NULL, stores the interface where the error occurred
> >+ * \param interface  if not NULL, stores the interface where the error occurred,
> >+ *                   or NULL, if unknown.
> >   * \param id         if not NULL, stores the object id that generated
> >- *                   the error. There's no guarantee the object is
> >- *                   still valid; the client must know if it deleted the object.
> >+ *                   the error, or 0, if the object id is unknown. There's no
> >+ *                   guarantee the object is still valid; the client must know
> >+ *                   if it deleted the object.
> >   * \return           The error code as defined in the interface specification.
> >   *
> >   * \code
> >
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel