[Mesa-dev] [PATCH] st/dri: Fix dangling pointer to a destroyed dri_drawable

[Emil Velikov]

Hi Johan,

On 20 April 2018 at 11:29, Johan Klokkhammer Helsing
<johan.helsing at qt.io> wrote:
> If an EGLSurface is created, made current and destroyed, and then a second
> EGLSurface is created. Then the second malloc in driCreateNewDrawable may
> return the same pointer address the first surface's drawable had.
What do you mean with "second malloc in driCreateNewDrawable"? There's
only a single malloc in the function.

Are you saying the C runtime returns identical pointers when creating
the first and second surface?

> Consequently, when dri_make_current later tries to determine if it should
> update the texture_stamp it compares the surface's drawable pointer against
> the drawable in the last call to dri_make_current and assumes it's the same
> surface (which it isn't).
>
> When texture_stamp is left unset, then dri_st_framebuffer_validate thinks
> it has already called update_drawable_info for that drawable, leaving it
> unvalidated and this is when bad things starts to happen. In my case it
> manifested itself by the width and height of the surface being unset.
>
> This is fixed this by setting the pointer to NULL before freeing the
> surface.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106126
> Signed-off-by: Johan Klokkhammer Helsing <johan.helsing at qt.io>
> ---
>  src/gallium/state_trackers/dri/dri_drawable.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/src/gallium/state_trackers/dri/dri_drawable.c b/src/gallium/state_trackers/dri/dri_drawable.c
> index e5a7537e47..02328acd98 100644
> --- a/src/gallium/state_trackers/dri/dri_drawable.c
> +++ b/src/gallium/state_trackers/dri/dri_drawable.c
> @@ -185,6 +185,7 @@ fail:
>  void
>  dri_destroy_buffer(__DRIdrawable * dPriv)
>  {
> +   struct dri_context *ctx = dri_context(dPriv->driContextPriv);
>     struct dri_drawable *drawable = dri_drawable(dPriv);
>     struct dri_screen *screen = drawable->screen;
>     struct st_api *stapi = screen->st_api;
> @@ -202,6 +203,9 @@ dri_destroy_buffer(__DRIdrawable * dPriv)
>     /* Notify the st manager that this drawable is no longer valid */
>     stapi->destroy_drawable(stapi, &drawable->base);
>
> +   if (ctx && ctx->dPriv == dPriv)
> +      ctx->dPriv = NULL;
> +
Context references in here feels wrong.

Are you sure we shouldn't be removing the context/drawable link in
dri_make_current?
After all it seems to be the one that adds it in the first place.

Something like the following:

--- a/src/gallium/state_trackers/dri/dri_context.c
+++ b/src/gallium/state_trackers/dri/dri_context.c
@@ -278,9 +278,16 @@ dri_make_current(__DRIcontext * cPriv,

    ++ctx->bind_count;

-   if (!draw && !read)
-      return ctx->stapi->make_current(ctx->stapi, ctx->st, NULL, NULL);
-   else if (!draw || !read)
+   if (!draw && !read) {
+      if (!ctx->stapi->make_current(ctx->stapi, ctx->st, NULL, NULL))
+         return GL_FALSE;
+
+      ctx->dPriv = NULL;
+      ctx->rPriv = NULL;
+      return GL_TRUE;
+   }
+
+   if (!draw || !read)
       return GL_FALSE;

    if (ctx->dPriv != driDrawPriv) {

-Emil