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

[Pekka Paalanen]

On Mon, 22 Feb 2016 13:22:05 -0800
Bryce Harrington <bryce at osg.samsung.com> wrote:

> 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>

And R-b me and pushed:
   cde251a..5646236  master -> master

with a small change noted below...

> > 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",

I changed "[unknown interface]@[unknown id]" to "[destroyed object]"
after talking with Jason in irc. Seems like a more helpful message that
is practically always correct.


Thanks,
pq

> > >+		       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  
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160226/26d27b2a/attachment.sig>