[PATCH:libX11] If XGetImage fails to create image, don't dereference it to bounds check

[Emil Velikov]

On 7 March 2018 at 20:10, Alan Coopersmith <alan.coopersmith at oracle.com> wrote:
> On 03/ 7/18 05:36 AM, Emil Velikov wrote:
>> Hi Alan,
>>
>> On 6 March 2018 at 21:47, Alan Coopersmith <alan.coopersmith at oracle.com> wrote:
>>> Reported by gcc 7.3:
>>>
>>> GetImage.c:110:25: warning: potential null pointer dereference [-Wnull-dereference]
>>>   if (planes < 1 || image->height < 1 || image->bytes_per_line < 1 ||
>>>                     ~~~~~^~~~~~~~
>>>
>>> Introduced by 8ea762f94f4c942d898fdeb590a1630c83235c17 in Xlib 1.6.4
>>>
>>> Signed-off-by: Alan Coopersmith <alan.coopersmith at oracle.com>
>>> ---
>>>  src/GetImage.c | 16 +++++++++-------
>>>  1 file changed, 9 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/src/GetImage.c b/src/GetImage.c
>>> index ff32d589..44a576a1 100644
>>> --- a/src/GetImage.c
>>> +++ b/src/GetImage.c
>>> @@ -105,14 +105,16 @@ XImage *XGetImage (
>>>             planes = 1;
>>>         }
>>>
>>> -       if (!image)
>>> +       if (!image) {
>>>             Xfree(data);
>>> -       if (planes < 1 || image->height < 1 || image->bytes_per_line < 1 ||
>>> -           INT_MAX / image->height <= image->bytes_per_line ||
>>> -           INT_MAX / planes <= image->height * image->bytes_per_line ||
>>> -           nbytes < planes * image->height * image->bytes_per_line) {
>>> -           XDestroyImage(image);
>>> -           image = NULL;
>>> +       } else {
>>> +            if (planes < 1 || image->height < 1 || image->bytes_per_line < 1 ||
>>> +                INT_MAX / image->height <= image->bytes_per_line ||
>>> +                INT_MAX / planes <= image->height * image->bytes_per_line ||
>>> +                nbytes < planes * image->height * image->bytes_per_line) {
>>> +                XDestroyImage(image);
>>> +                image = NULL;
>>> +            }
>>
>> Instead of reshuffling, one could easily add the missing unlock/sync
>> in the error path.
>> Or even a goto unlock?
>
> Sorry - not sure I'm following - are you suggesting something like this?
>
> --- a/src/GetImage.c
> +++ b/src/GetImage.c
> @@ -104,17 +104,20 @@ XImage *XGetImage (
>                     _XGetScanlinePad(dpy, (int) rep.depth), 0);
>             planes = 1;
>         }
>
> -       if (!image)
> +       if (!image) {
>             Xfree(data);
> +           goto unlock;
> +       }
>         if (planes < 1 || image->height < 1 || image->bytes_per_line < 1 ||
>             INT_MAX / image->height <= image->bytes_per_line ||
>             INT_MAX / planes <= image->height * image->bytes_per_line ||
>             nbytes < planes * image->height * image->bytes_per_line) {
>             XDestroyImage(image);
>             image = NULL;
>         }
> +  unlock:
>         UnlockDisplay(dpy);
>         SyncHandle();
>         return (image);
>  }
>
Yes, precisely.

>
> That should be effectively equivalent, just less change in indentation for
> the lines in between.
>
You're correct - it's functionally identical although it seems cleaner.

Regardless of which version you opt for
Reviewed-by: Emil Velikov <emil.velikov at collabora.com>

-Emil