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

[Alan Coopersmith]

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);
 }


That should be effectively equivalent, just less change in indentation for
the lines in between.

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