[Xcb] [PATCH] _xcb_conn_wait: Shut down the connection on unexpected poll() events.

[Aaron Plattner]

On Thu, Aug 26, 2010 at 04:54:03PM -0700, Aaron Plattner wrote:
> From: Jamey Sharp <jamey at minilop.net>
> 
> If a client calls close(2) on the connection's file descriptor and then
> flushes writes, libxcb causes a hang in the client.
> 
> Any flush eventually calls _xcb_out_send() with has the following loop:
>    while(ret && *count)
>        ret = _xcb_conn_wait(c, &c->out.cond, vector, count);
> 
> _xcb_conn_wait(), if built with USE_POLL, gets the POLLNVAL error. It only
> checks for POLLIN and POLLOUT though, ignoring the error. Return value is 1,
> count is unmodified, leaving us with an endless loop and a client hang.
> 
> XTS testcase Xlib3/XConnectionNumber triggers this bug. It creates a display
> connection, closes its file descriptor, tries to send a no-op, and then expects
> an error.
> http://cgit.freedesktop.org/xorg/test/xts/tree/xts5/Xlib3/XConnectionNumber.m
> 
> If poll returned POLLHUP or POLLERR, we might see the same result.
> 
> If poll returns any event we didn't ask for, this patch causes
> _xcb_conn_shutdown() to be invoked and an error returned. This matches the
> behaviour if select(2) is used instead of poll(2): select(2) returns -1 and
> EBADF for an already closed file descriptor.
> 
> I believe this fix both is safe and will handle any similar error. POSIX says
> that the only bits poll is permitted to set in revents are those bits that were
> set in events, plus POLLHUP, POLLERR, and POLLNVAL. So if we see any flags we
> didn't ask for then something has gone wrong.
> 
> Patch inspired by earlier proposals from Peter Hutterer and Aaron
> Plattner--thanks!
> 
> Reported-by: Peter Hutterer <peter.hutterer at who-t.net>
> Reported-by: Aaron Plattner <aplattner at nvidia.com>
> Signed-off-by: Jamey Sharp <jamey at minilop.net>
> Reviewed-by: Aaron Plattner <aplattner at nvidia.com>
> Tested-by: Aaron Plattner <aplattner at nvidia.com>
> Cc: Peter Hutterer <peter.hutterer at who-t.net>
> Cc: Dan Nicholson <dbn.lists at gmail.com>
> ---
> Peter, does this address your concern about the loop not exiting if errno ==
> EINTR?  Alternatively, I could set errno = 0.
> 
> I left Jamey as the author since this is basically the same as his patch.
> 
>  src/xcb_conn.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/src/xcb_conn.c b/src/xcb_conn.c
> index 803f7aa..7b0c2ab 100644
> --- a/src/xcb_conn.c
> +++ b/src/xcb_conn.c
> @@ -319,6 +319,13 @@ int _xcb_conn_wait(xcb_connection_t *c, pthread_cond_t *cond, struct iovec **vec
>      do {
>  #if USE_POLL
>          ret = poll(&fd, 1, -1);
> +        /* If poll() returns an vent we didn't expect, such as POLLNVAL, treat

Aargh, I fail at vim.  This should obviously read "an event".

> +         * it as if it failed. */
> +        if(ret >= 0 && (fd.revents & ~fd.events))
> +        {
> +            ret = -1;
> +            break;
> +        }
>  #else
>          ret = select(c->fd + 1, &rfds, &wfds, 0, 0);
>  #endif
> -- 
> 1.6.0.4