[MPlayer-dev-eng] [PATCH]Fix possible memlead in vo_vdpau

[Carl Eugen Hoyos]

Reimar Döffinger <Reimar.Doeffinger <at> gmx.de> writes:

> > Index: libvo/vo_vdpau.c
> > ===================================================================
> > --- libvo/vo_vdpau.c	(revision 29675)
> > +++ libvo/vo_vdpau.c	(working copy)
> > @@ -479,6 +479,8 @@
> >  
> >      for (i = 0; i < MAX_VIDEO_SURFACES; i++) {
> >          if (surface_render[i].surface != VDP_INVALID_HANDLE) {
> > +          av_freep(&surface_render[i].bitstream_buffers);
> > +          surface_render[i].bitstream_buffers_allocated = 0;
> >            vdp_st = vdp_video_surface_destroy(surface_render[i].surface);
> >            CHECK_ST_WARNING("Error when calling vdp_video_surface_destroy")
> >          }
> 
> This looks wrong to me, bitstream_buffers gets neither overwritten nor
> reallocated nor anything here.
> I mean it may be correct in principle, but it violates the rule that
> things should be closed/freed in the same or a symmetric place to where
> they were allocated/opened.

Am I correct that libavcodec currently doesn't "know" about those pointers and
cannot free them?

> I don't have a suggestion how to fix this right now though (IMO this
> should be done on avcodec_close, but this opens up some ugly issues),
> but at least I'd suggest to do this in uninit.

Is the following (inlined) acceptable?

Carl Eugen

Index: libvo/vo_vdpau.c
===================================================================
--- libvo/vo_vdpau.c    (revision 29767)
+++ libvo/vo_vdpau.c    (working copy)
@@ -1015,6 +1015,12 @@ static void uninit(void)
         return;
     visible_buf = 0;

+    for (i = 0; i < MAX_VIDEO_SURFACES; i++) {
+        // Allocated in ff_vdpau_add_data_chunk()
+        av_freep(&surface_render[i].bitstream_buffers);
+        surface_render[i].bitstream_buffers_allocated = 0;
+    }
+
     /* Destroy all vdpau objects */
     DestroyVdpauObjects();