[PATCH] wayland-server: Fix null pointer dereferencing

[ASHIM SINGH SHAH]

if wl_resource_create() fails to create the resource (i.e. client->display_resource) and returns NULL. 
For such case, the patch will post the message "no memory" for that client in "wl_resource_post_error()" and after that 
it will return at below check.
if (!client->display_resource)
    return;
I think patch is correct in above sense.


Regards,
Ashim Singh Shah


------- Original Message -------
Sender : Marek Chalupa<mchqwerty at gmail.com> 
Date   : Jul 23, 2015 11:33 (GMT+05:30)
Title  : Re: [PATCH] wayland-server: Fix null pointer dereferencing



On 07/16/2015 09:37 AM, Marek Chalupa wrote:
>
>
> 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(...);
> }

Found and read this one again, so one more comment in my monologue :)

Flag client->destroying is useless since client->display_resource being
NULL has the same semantics, so we could use that.

>
> 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;
>>>
>>>       /*
>>>

Cheers,
Marek