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

[Emil Velikov]

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?

-Emil