[FFmpeg-devel] [RFC] missing av_free_packet on error?

[Reimar Döffinger]

On Thu, Jul 10, 2008 at 10:31:32PM +0200, Michael Niedermayer wrote:
> On Thu, Jul 10, 2008 at 09:53:15PM +0200, Reimar D?ffinger wrote:
> > I have been trying to fix
> > http://bugzilla.mplayerhq.hu/show_bug.cgi?id=1136
> > and thus noticed the following code in libavformat/eacdata.c:
> > ================================
> > if (av_get_packet(s->pb, pkt, packet_size) != packet_size)
> >    return AVERROR(EIO);
> > ================================
> > 
> > Whereas av_find_stream_info() assumes that if there is an error, there
> > is no packet to free.
> > My question is: where is the error, where should it be fixed?
> > In the demuxer, in av_find_stream_info, av_read_... or maybe this check
> > should be moved to av_get_packet, to make it always fail when not all
> > requested data could be read?
> > IMO in the demuxer, in which case I suggest this change:
> 
> Your change is buggy.
> You either need to limit the freeing for >=0 return or
> make av_new_packet() set data=NULL in case of malloc failure

Hm, I first wanted to change av_init_packet, until I read its doxy...
That really is a badly chosen name. Changing it would change the public
API though, and may not be acceptable (and if someone has time, could
you check its use in oggenc.c? It looks somewhat suspicious to me,
though I do not know the exact semantic of the interleave_packet
functions)...
So I propose the attached patch. The memset should not be necessary
currently, but the way I interpret the av_init_packet description that
should not be assumed for the future.
Sorry in case I did overcomplicate all this.

Greetings,
Reimar D?ffinger
-------------- next part --------------
Index: libavformat/utils.c
===================================================================
--- libavformat/utils.c	(revision 14158)
+++ libavformat/utils.c	(working copy)
@@ -205,6 +205,9 @@
 int av_new_packet(AVPacket *pkt, int size)
 {
     uint8_t *data;
+    memset(pkt, 0, sizeof(*pkt));
+    av_init_packet(pkt);
+
     if((unsigned)size > (unsigned)size + FF_INPUT_BUFFER_PADDING_SIZE)
         return AVERROR(ENOMEM);
     data = av_malloc(size + FF_INPUT_BUFFER_PADDING_SIZE);
@@ -212,7 +215,6 @@
         return AVERROR(ENOMEM);
     memset(data + size, 0, FF_INPUT_BUFFER_PADDING_SIZE);
 
-    av_init_packet(pkt);
     pkt->data = data;
     pkt->size = size;
     pkt->destruct = av_destruct_packet;
Index: libavformat/eacdata.c
===================================================================
--- libavformat/eacdata.c	(revision 14158)
+++ libavformat/eacdata.c	(working copy)
@@ -83,8 +83,10 @@
     CdataDemuxContext *cdata = s->priv_data;
     int packet_size = 76*cdata->channels;
 
-    if (av_get_packet(s->pb, pkt, packet_size) != packet_size)
+    if (av_get_packet(s->pb, pkt, packet_size) != packet_size) {
+        av_free_packet(pkt);
         return AVERROR(EIO);
+    }
     pkt->pts = cdata->audio_pts++;
     return 1;
 }