[vlc-devel] [PATCH] interop_android: fix potential use-after-free of SurfaceTexture

[Alexandre Janniaux]

Hi,

It is recent, but technically not a real regression, since you
would need the same interop with multiple decoder before to
reproduce the issue, and there's currently no way to achieve
that.

I'll put it in the commit though, the issue would arise since
d4d3950541c504eea727d5e1f7ca941156e33656 which didn't even hold
the picture, and 2851b99820fc8dc0ba30f64ee2349e642b09eeea which
introduced the release of the picture at the wrong location.

On Fri, Jan 22, 2021 at 03:31:59PM +0100, Thomas Guillem wrote:
> LGTM.
>
> Is it a recent regression? If so, could you put the commit id in the commit log?
>
> On Fri, Jan 22, 2021, at 15:20, Alexandre Janniaux wrote:
> > picture_Release() might release the last reference to the video_context
> > holding the previous_texture SurfaceTexture, which still needs to be
> > detached and potentially have its bufer released.
> > ---
> >  modules/video_output/opengl/interop_android.c | 21 ++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/modules/video_output/opengl/interop_android.c
> > b/modules/video_output/opengl/interop_android.c
> > index 4f3f6dfce8..f94a7c26e9 100644
> > --- a/modules/video_output/opengl/interop_android.c
> > +++ b/modules/video_output/opengl/interop_android.c
> > @@ -62,18 +62,17 @@ tc_anop_update(struct vlc_gl_interop *interop,
> > GLuint *textures,
> >      assert(pic->context);
> >      assert(textures[0] != 0);
> >
> > -    if (priv->current_picture)
> > -        picture_Release(priv->current_picture);
> > +    picture_t *previous_picture = priv->current_picture;
> >      priv->current_picture = picture_Hold(pic);
> >
> >      struct vlc_video_context *vctx = pic->context->vctx;
> >      android_video_context_t *avctx =
> >          vlc_video_context_GetPrivate(vctx, VLC_VIDEO_CONTEXT_AWINDOW);
> >      if (avctx == NULL)
> > -        return VLC_EGENERIC;
> > +        goto error;
> >
> >      if (plane_offset != NULL)
> > -        return VLC_EGENERIC;
> > +        goto error;
> >
> >      struct vlc_asurfacetexture *texture =
> >          avctx->get_texture(pic->context);
> > @@ -91,14 +90,14 @@ tc_anop_update(struct vlc_gl_interop *interop,
> > GLuint *textures,
> >          }
> >
> >          if (SurfaceTexture_attachToGLContext(texture, textures[0]) != 0)
> > -            return VLC_EGENERIC;
> > +            goto error;
> >
> >          priv->stex_attached = true;
> >          priv->previous_texture = texture;
> >      }
> >
> >      if (!avctx->render(pic->context))
> > -        return VLC_SUCCESS; /* already rendered */
> > +        goto success; /* already rendered */
> >
> >      /* Release previous image */
> >      if (previous_texture && previous_texture != texture)
> > @@ -108,13 +107,21 @@ tc_anop_update(struct vlc_gl_interop *interop,
> > GLuint *textures,
> >          != VLC_SUCCESS)
> >      {
> >          priv->transform_mtx = NULL;
> > -        return VLC_EGENERIC;
> > +        goto error;
> >      }
> >
> >      interop->vt->ActiveTexture(GL_TEXTURE0);
> >      interop->vt->BindTexture(interop->tex_target, textures[0]);
> >
> > +success:
> > +    if (previous_picture)
> > +        picture_Release(previous_picture);
> >      return VLC_SUCCESS;
> > +
> > +error:
> > +    if (previous_picture)
> > +        picture_Release(previous_picture);
> > +    return VLC_EGENERIC;
> >  }
> >
> >  static const float *
> > --
> > 2.30.0
> >
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel