[FFmpeg-devel] [PATCH 2/2] libnut: check for av_malloc failure.

[Michael Niedermayer]

On Wed, Dec 07, 2011 at 08:50:14AM +0200, Oded wrote:
> Some trivial comments
> 
> On Mon, Nov 28, 2011 at 05:30:08AM +0100, Michael Niedermayer wrote:
> > @@ -70,6 +70,8 @@ static int nut_write_header(AVFormatContext * avf) {
> >      int i;
> >  
> >      priv->s = s = av_mallocz((avf->nb_streams + 1) * sizeof*s);
> > +    if(!s)
> > +        return AVERROR(ENOMEM);
> >  
> >      for (i = 0; i < avf->nb_streams; i++) {
> >          AVCodecContext * codec = avf->streams[i]->codec;
> 
> I assume nut_write_packet() and nut_write_trailer() will not be called if 
> nut_write_header() returned error?

They should not, no


> 
> There is also a tiny av_malloc() in the same function, in loop, for 
> fourcc. Requires rollback on error if handeled correctly...
>
> Also, no check for nut_muxer_init()
> 
> > @@ -223,6 +225,10 @@ static int nut_read_header(AVFormatContext * avf, AVFormatParameters * ap) {
> >          st->codec->extradata_size = s[i].codec_specific_len;
> >          if (st->codec->extradata_size) {
> >              st->codec->extradata = av_mallocz(st->codec->extradata_size);
> > +            if(!st->codec->extradata){
> > +                nut_demuxer_uninit(nut);
> > +                return AVERROR(ENOMEM);
> > +            }
> >              memcpy(st->codec->extradata, s[i].codec_specific, st->codec->extradata_size);
> >          }
> 
> Again, I assume nut_read_close() will not be called, otherwise there is 
> double free (which could even be an existing bug, in "return -1" above)

they should not but we can NULL the pointers just to be sure


> 
> I assume av_new_stream() allocations are handeled and freed outside 
> libnut.c ?

yes


> Also, can av_new_stream() fail? No check for it...

it can fail, yes


> Also, no check for nut_demuxer_init()

added


anyway, i wont fix all existing bugs in libnut.c, 
thats the job of the maintainer IMHO

I just wanted to make sure you are ok with the malloc checks and
that they are fixing bugs without introducing new ones.

ill send a new patchset with the changes in a moment

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20111207/1a18aa7d/attachment.asc>