[PATCH] Fix re-entrancy issues in wl_display_iterate().

[Pekka Paalanen]

On Fri, 13 Apr 2012 01:08:29 -0700 (PDT)
yan.wang at linux.intel.com wrote:

> > This fixes a possible re-entrancy issue in case of nested requests.
> >
> > ---
> >  src/connection.c      |    8 +++++++-
> >  src/wayland-client.c  |    6 ++++--
> >  src/wayland-private.h |    1 +
> >  3 files changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/connection.c b/src/connection.c
> > index 2795481..c5c9c77 100644
> > --- a/src/connection.c
> > +++ b/src/connection.c
> > @@ -259,6 +259,12 @@ decode_cmsg(struct wl_buffer *buffer, struct msghdr
> > *msg)
> >  }
> >
> >  int
> > +wl_connection_data_length(struct wl_connection *connection)
> > +{
> > +	return connection->in.head - connection->in.tail;
> > +}
> > +
> > +int
> >  wl_connection_data(struct wl_connection *connection, uint32_t mask)
> >  {
> >  	struct iovec iov[2];
> > @@ -335,7 +341,7 @@ wl_connection_data(struct wl_connection *connection,
> > uint32_t mask)
> >  		connection->in.head += len;
> >  	}
> >
> > -	return connection->in.head - connection->in.tail;
> > +	return wl_connection_data_length(connection);
> >  }
> >
> >  int
> > diff --git a/src/wayland-client.c b/src/wayland-client.c
> > index 9057b4f..0bf8c81 100644
> > --- a/src/wayland-client.c
> > +++ b/src/wayland-client.c
> > @@ -509,9 +509,11 @@ wl_display_iterate(struct wl_display *display,
> > uint32_t mask)
> >  		return;
> >  	}
> >
> > -	len = wl_connection_data(display->connection, mask);
> > +	wl_connection_data(display->connection, mask);
> > +
> > +	for (;;) {
> > +		len = wl_connection_data_length(display->connection);
> >
> > -	while (len > 0) {
> >  		if ((size_t) len < sizeof p)
> >  			break;
> >

> wl_display_iterate will be called in multiply threads? Normally, I think
> it may be called in message loop thread.
> Thanks.

It does not seem to be about threads or parallelism, but nested calls.
wl_display_iterate() will dispatch events, therefore call your handler
functions, and if a handler function happens to call
wl_display_iterate(), you get screwed. This concerns only
wl_display_iterate(WL_DISPLAY_READABLE) type of calls.

Hannu, I am not sure allowing this is actually sane. Can you clarify
your case, why you need it?

If you call wl_display_iterate() (or a function calling that) from an
event handler function path, it will not do what you might expect. It
will resume dispatching events from where it was, it will not skip any.
This means you will get re-entrancy issues all over, since it forces
basically all handler functions and all functions called from handlers
to have to be re-entrant safe. If you happen to get more events, that
again call wl_display_iterate() in their handlers, you are facing an
unlimited recursion driven by another (e.g. malicious/faulty) process.

Specifically this means, that wl_display_roundtrip() must not be used
from an event handler path.

We work around these problems with display_defer() in the toytoolkit,
and wl_event_loop_add_idle() in Weston.

Hannu, or is this about Mesa calling wl_display_iterate()? That one is
a bug, and needs to be fixed in Mesa. It might trigger what you
describe, if you call eglSwapBuffers in a Wayland client from an event
handler path.


Thanks,
pq