[PATCH] wayland-server: Fix null pointer dereferencing

[Marek Chalupa]

On 07/16/2015 09:27 AM, Marek Chalupa wrote:
> Nice catch (some static analysis tool? :).
>
> I'm not sure if this is the right way to fix it, though. If somebody
> calls wl_resource_post_error with NULL resource, it is his fault and
> program should crash to reveal this error. So the right fix in this case
> would be to delete wl_client_post_no_memory() on line 783 in bind_display.
> Can wl_resource_post_error be called on some other place with NULL?
> The only thing I can make up is when destroying client - resources are
> being destroyed and the first is destroyed display_resource, thus if
> any destroy handler after that calls wl_client_post_no_memory, we get
> wl_resource_post_error(NULL, ...).
>
> So the fix is correct in the sense that there are paths with resource =
> NULL. On the other hand, we'd like to discover cases where programmer
> passes NULL. Hmm, maybe something like client->destroying + NULL checks
> could solve this. Or we'll just go with this patch - since the NULL
> paths are (I hope :) only on client destruction, this should be ok.

Or we could add check to wl_client_post_no_memory(). That would solve 
the problem when client is being destroyed and wl_resource_post_error 
would be untouched and passing NULL would crash the program as "desired".

Hmm.. Still keep the though with client->destroying. If we did:

wl_client_post_no_memory(client)
{
     if (client->destroying)
         return;

     wl_resource_post_error(...);
}

Then all the programming errors would go through, reveling error in 
program, yet calling this on client destruction would be safe.
(We still need to fix bind_display with this approach)

Cheers,
Marek

>
> On 07/16/2015 08:27 AM, Ashim wrote:
>> Initialising 'wl_client *client = NULL' and checking 'resource' for
>> NULL and returning if found.
>> This patch will avoid dereferencing of 'resource' if NULL
>>
>> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=91356
>>
>> Signed-off-by: Ashim <ashim.shah at samsung.com>
>> ---
>>   src/wayland-server.c |    7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/wayland-server.c b/src/wayland-server.c
>> index 0f04f66..2a0d19c 100644
>> --- a/src/wayland-server.c
>> +++ b/src/wayland-server.c
>> @@ -207,7 +207,7 @@ WL_EXPORT void
>>   wl_resource_post_error(struct wl_resource *resource,
>>                  uint32_t code, const char *msg, ...)
>>   {
>> -    struct wl_client *client = resource->client;
>> +    struct wl_client *client = NULL;
>>       char buffer[128];
>>       va_list ap;
>>
>> @@ -215,6 +215,11 @@ wl_resource_post_error(struct wl_resource *resource,
>>       vsnprintf(buffer, sizeof buffer, msg, ap);
>>       va_end(ap);
>>
>> +    if (resource == NULL)
>> +        return;
>> +    else
>> +        client = resource->client;
>> +
>>       client->error = 1;
>>
>>       /*
>>