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

[Martin Peres]

On 21/07/15 03:20, Alan Coopersmith wrote:
> 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>
>
Hey anyone, could we land this? I have the push rights but I do not know 
what is the expected procedure for this (if any).

It would be nice to backport to older releases too, since it likely 
plagues them all!