[FFmpeg-devel] Memory leak using bitstream filters with shared libs

[Reimar Döffinger]

On Wed, Apr 02, 2008 at 01:37:58PM +0200, Michael Niedermayer wrote:
> On Wed, Apr 02, 2008 at 10:50:20AM +0200, Baptiste Coudurier wrote:
> > Hi guys,
> > 
> > M?ns Rullg?rd wrote:
> > > Baptiste discovered a rather nasty memory leak when using a bitstream
> > > filter with shared libraries.
> > > 
> > > In ffmpeg.c:write_frame() the output of bitstream filters is stored in
> > > an AVPacket, and its destruct function is set like this (line 417):
> > > 
> > >             new_pkt.destruct= av_destruct_packet;
> > > 
> > > Later on, in av_interleave_packet_per_dts(), AVPacket.destruct is
> > > compared against av_destruct_packet, like this (utils.c:2439):
> > > 
> > >         if(pkt->destruct == av_destruct_packet)
> > > 
> > > The trouble here is that with shared libraries, the address of
> > > av_destruct_packet when used in the main executable or libs other than
> > > the one defining it resolves to a trampoline that calls the real
> > > function using a GOT entry.  As a result, this comparison is always
> > > false when the destruct pointer was assigned outside libavformat.
> > > 
> > > Does anyone have an idea for fixing this?
> > > 
> > 
> > Sorry, but could we try to find an acceptable solution for this issue ?
> 
> as said elsewhere in this thread:
>  static inline void av_free_packet(AVPacket *pkt)
>  {
> -    if (pkt && pkt->destruct) {
> -        pkt->destruct(pkt);
> +    if(pkt){
> +        if(pkt->destruct)
> +            pkt->destruct(pkt);
> +        pkt->data = NULL; pkt->size = 0;
>      }
>  }
> ----
>  void av_destruct_packet(AVPacket *pkt)
>  {
>      av_free(pkt->data);
> -    pkt->data = NULL; pkt->size = 0;
>  }
> ----
> -void av_destruct_packet_nofree(AVPacket *pkt)
> -{
> -    pkt->data = NULL; pkt->size = 0;
> -}
> 
> and use NULL instead of av_destruct_packet_nofree
> -----------------

The reason I do not like this much is because it means you can't e.g.
set destruct to something that does only logging, the destruct function
implicitly defines how copies are made.
Anyway, while trying to implement this I came about something I don't
really understand, in ff_interleave_add_packet:
>     if(pkt->destruct == av_destruct_packet)
>         pkt->destruct= NULL; // not shared -> must keep original from being freed
>     else
>        av_dup_packet(&this_pktl->pkt);  //shared -> must dup

is this just a complete brainfart, or is that comparison against a
static function declared in a header on purpose?
And are the semantics really supposed to be that when when a
user-defined destruct it used it will be called multiple times on the
same pkt (more or less)?
And are there any reasons why it is assumed that the original AVPacket
passed into ff_interleave_add_packet will not be used longer than the
copy?
Does all this stuff work for any other reason than pure luck (SCNR ;-)
).

Greetings,
Reimar D?ffinger