[PATCH] os: make sure the clientsWritable fd_set is initialized before use

[Alan Coopersmith]

On 07/20/15 12:37 AM, Martin Peres wrote:
> In WaitForSomething(), the fd_set clientsWritable may be used unitialized when
> the boolean AnyClientsWriteBlocked is set in the WakeupHandler(). This leads to
> a crash in FlushAllOutput() after x11proto's commit
> 2c94cdb453bc641246cc8b9a876da9799bee1ce7.
>
> The problem did not manifest before because both the XFD_SIZE and the maximum
> number of clients were set to 256. As the connectionTranslation table was
> initalized for the 256 clients to 0, the test on the index not being 0 was
> aborting before dereferencing the client #0.
>
> As of commit 2c94cdb453bc641246cc8b9a876da9799bee1ce7 in x11proto, the XFD_SIZE
> got bumped to 512. This lead the OutputPending fd_set to have any fd above 256
> to be uninitialized which in turns lead to reading an index after the end of
> the ConnectionTranslation table. This index would then be used to find the
> client corresponding to the fd marked as pending writes and would also result
> to an out-of-bound access which would usually be the fatal one.
>
> Fix this by zeroing the clientsWritable fd_set at the beginning of
> WaitForSomething(). In this case, the bottom part of the loop, which would
> indirectly call FlushAllOutput, will not do any work but the next call to
> select will result in the execution of the right codepath. This is exactly what
> we want because we need to know the writable clients before handling them. In
> the end, it also makes sure that the fds above MaxClient are initialized,
> preventing the crash in FlushAllOutput().
>
> Thanks to everyone involved in tracking this one down!
>
> Reported-by: Karol Herbst <freedesktop at karolherbst.de>
> Reported-by: Tobias Klausmann <tobias.klausmann at mni.thm.de>
> Signed-off-by: Martin Peres <martin.peres at linux.intel.com>
> Tested-by: Tobias Klausmann <tobias.klausmann at mni.thm.de>
> Tested-by: Martin Peres <martin.peres at linux.intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91316
> Cc: Ilia Mirkin  <imirkin at alum.mit.edu>
> Cc: Olivier Fourdan <ofourdan at redhat.com
> Cc: Adam Jackson <ajax at redhat.com>
> Cc: Alan Coopersmith <alan.coopersmith at oracle.com
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>   os/WaitFor.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/os/WaitFor.c b/os/WaitFor.c
> index 431f1a6..993c14e 100644
> --- a/os/WaitFor.c
> +++ b/os/WaitFor.c
> @@ -158,6 +158,7 @@ WaitForSomething(int *pClientsReady)
>       Bool someReady = FALSE;
>
>       FD_ZERO(&clientsReadable);
> +    FD_ZERO(&clientsWritable);
>
>       if (nready)
>           SmartScheduleStopTimer();
>

Reviewed-by: Alan Coopersmith <alan.coopersmith at oracle.com>

-- 
	-Alan Coopersmith-              alan.coopersmith at oracle.com
	 Oracle Solaris Engineering - http://blogs.oracle.com/alanc