[Mesa-dev] [PATCH 01/10] egl: move eglCreateDRMImageMESA's malloc later

[Eric Engestrom]

On Friday, 2017-06-30 12:15:11 +0100, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov at collabora.com>
> 
> Don't bother allocating any memory until we're finished parsing and
> sanitising all the attributes.
> 
> As a nice side effect we now consistently set eglError when any of
> the attrib/values are not correct.

Not initialising `err` would've caught this bug; please add the
following hunk to this commit?

---8<---
diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index cf26242702..1ed511d6da 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -2298,7 +2298,7 @@ dri2_create_drm_image_mesa(_EGLDriver *drv, _EGLDisplay *disp,
    _EGLImageAttribs attrs;
    unsigned int dri_use, valid_mask;
    int format;
-   EGLint err = EGL_SUCCESS;
+   EGLint err;
 
    (void) drv;
 
--->8---

Alternatively, you could move the call to _eglParseImageAttribList()
inside the `if()` and get rid or `err` altogether.

> 
> Strangely enough the spec does not mention _anything_ about what error
> should be set where, even if the implementation already sets the odd
> one.
> 
> Cc: Kristian Høgsberg <krh at bitplanet.net>
> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
> ---
> Kristian can you shed some light? The extension seems quite sparse.
> 
> Weston used the extension back in 2011. While the Glamor bit were
> dropped somewhat recently in May 2017.
> 
> Other than those I cannot find any users of the extension - there's no
> piglit tests even :-\
> 
> A crazy idea: Should we deprecate the extension?
> ---
>  src/egl/drivers/dri2/egl_dri2.c | 52 ++++++++++++++++++-----------------------
>  1 file changed, 23 insertions(+), 29 deletions(-)
> 
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index cf262427020..e55bff6dbbf 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -2302,27 +2302,20 @@ dri2_create_drm_image_mesa(_EGLDriver *drv, _EGLDisplay *disp,
>  
>     (void) drv;
>  
> -   dri2_img = malloc(sizeof *dri2_img);
> -   if (!dri2_img) {
> -      _eglError(EGL_BAD_ALLOC, "dri2_create_image_khr");
> -      return EGL_NO_IMAGE_KHR;
> -   }
> -
>     if (!attr_list) {
> -      err = EGL_BAD_PARAMETER;
> -      goto cleanup_img;
> +      _eglError(EGL_BAD_PARAMETER, __func__);
> +      return EGL_NO_IMAGE_KHR;
>     }
>  
> -   _eglInitImage(&dri2_img->base, disp);
> -
>     err = _eglParseImageAttribList(&attrs, disp, attr_list);
> -   if (err != EGL_SUCCESS)
> -      goto cleanup_img;
> +   if (err != EGL_SUCCESS) {
> +      _eglError(EGL_BAD_PARAMETER, __func__);
> +      return EGL_NO_IMAGE_KHR;
> +   }
>  
>     if (attrs.Width <= 0 || attrs.Height <= 0) {
> -      _eglLog(_EGL_WARNING, "bad width or height (%dx%d)",
> -            attrs.Width, attrs.Height);
> -      goto cleanup_img;
> +      _eglError(EGL_BAD_PARAMETER, __func__);
> +      return EGL_NO_IMAGE_KHR;
>     }
>  
>     switch (attrs.DRMBufferFormatMESA) {
> @@ -2330,9 +2323,8 @@ dri2_create_drm_image_mesa(_EGLDriver *drv, _EGLDisplay *disp,
>        format = __DRI_IMAGE_FORMAT_ARGB8888;
>        break;
>     default:
> -      _eglLog(_EGL_WARNING, "bad image format value 0x%04x",
> -            attrs.DRMBufferFormatMESA);
> -      goto cleanup_img;
> +      _eglError(EGL_BAD_PARAMETER, __func__);
> +      return EGL_NO_IMAGE_KHR;
>     }
>  
>     valid_mask =
> @@ -2340,9 +2332,8 @@ dri2_create_drm_image_mesa(_EGLDriver *drv, _EGLDisplay *disp,
>        EGL_DRM_BUFFER_USE_SHARE_MESA |
>        EGL_DRM_BUFFER_USE_CURSOR_MESA;
>     if (attrs.DRMBufferUseMESA & ~valid_mask) {
> -      _eglLog(_EGL_WARNING, "bad image use bit 0x%04x",
> -            attrs.DRMBufferUseMESA & ~valid_mask);
> -      goto cleanup_img;
> +      _eglError(EGL_BAD_PARAMETER, __func__);
> +      return EGL_NO_IMAGE_KHR;
>     }
>  
>     dri_use = 0;
> @@ -2353,22 +2344,25 @@ dri2_create_drm_image_mesa(_EGLDriver *drv, _EGLDisplay *disp,
>     if (attrs.DRMBufferUseMESA & EGL_DRM_BUFFER_USE_CURSOR_MESA)
>        dri_use |= __DRI_IMAGE_USE_CURSOR;
>  
> +   dri2_img = malloc(sizeof *dri2_img);
> +   if (!dri2_img) {
> +      _eglError(EGL_BAD_ALLOC, "dri2_create_image_khr");
> +      return EGL_NO_IMAGE_KHR;
> +   }
> +
> +   _eglInitImage(&dri2_img->base, disp);
> +
>     dri2_img->dri_image =
>        dri2_dpy->image->createImage(dri2_dpy->dri_screen,
>                                     attrs.Width, attrs.Height,
>                                     format, dri_use, dri2_img);
>     if (dri2_img->dri_image == NULL) {
> -      err = EGL_BAD_ALLOC;
> -      goto cleanup_img;
> +      free(dri2_img);
> +       _eglError(EGL_BAD_ALLOC, "dri2_create_drm_image_mesa");
> +      return EGL_NO_IMAGE_KHR;
>     }
>  
>     return &dri2_img->base;
> -
> - cleanup_img:
> -   free(dri2_img);
> -   _eglError(err, "dri2_create_drm_image_mesa");
> -
> -   return EGL_NO_IMAGE_KHR;
>  }
>  
>  static EGLBoolean
> -- 
> 2.13.0
>