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

[Pekka Paalanen]

On Fri, 26 Feb 2016 11:57:51 +0200
Pekka Paalanen <ppaalanen at gmail.com> wrote:

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

*cough*
I mean Jonas, not Jason. Sorry :-p


Thanks,
pq
-------------- 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/6cd39aec/attachment.sig>