[FFmpeg-devel] [PATCH 3/7] lavc/utils: check av_frame_alloc() failure.

Michael Niedermayer michaelni at gmx.at
Mon Dec 30 13:44:47 CET 2013


On Mon, Dec 30, 2013 at 10:56:51AM +0100, Nicolas George wrote:
> Le decadi 10 nivôse, an CCXXII, Michael Niedermayer a écrit :
> > i test almost all my comits but its quite possible that what i tested
> > (fate probably) did never test the code i changed, thats my mistake
> > no question about that
> 
> I never doubted you did test, it was intended as a joke. The code is indeed
> certainly not covered by FATE, or we would probably have noticed the
> breakage I am reporting sooner, but your commit was perfectly correct
> (except for the error check part, and this is of dubious use due to
> overcommit).
> 
> > I think we should add some fate tests for old APIs
> > maybe the examples could be adapted to do that, i could look into that
> > but as you tested the code, i assume you already have a test for them?
> 
> I had the same idea.
> 
> > I possibly worded this unclearly
> > what i meant was that if setting
> > release_buffer= avcodec_default_release_buffer
> > causes some problem then an application that calls
> > avcodec_default_release_buffer from its release_buffer implementation
> > would also cause the problem, and not setting release_buffer would
> > likely leak and i saw one case where its called without a null
> > pointer check so it could crash
> > 
> > but maybe iam missing something, can you elaborate about the issue
> > you found ?
> 
> I suppose you may be forgetting something: since avcodec_decode_audio4() was
> introduced (2011-09-06 / 0eea21294), using a custom get_buffer with compat
> avcodec_decode_audio3() does not work and is forbidden. The original check
> was:
> 
> +    if (avctx->get_buffer != avcodec_default_get_buffer) {
> +        av_log(avctx, AV_LOG_ERROR, "A custom get_buffer() cannot be used with "
> +               "avcodec_decode_audio3()\n");
> +        return AVERROR(EINVAL);
> +    }
> 
> And it was later amended to make the message clearer and override the custom
> callback instead of failing (2012-01-15 / e2ff436ef):
> 
> +        av_log(avctx, AV_LOG_ERROR, "Custom get_buffer() for use with"
> +               "avcodec_decode_audio3() detected. Overriding with
> avcodec_default_get_buffer\n");
> +        av_log(avctx, AV_LOG_ERROR, "Please port your application to "
> +               "avcodec_decode_audio4()\n");
> +        avctx->get_buffer = avcodec_default_get_buffer;
> 
> Unfortunately, since the refcounted frames, i.e. "the evil plan"
> (2012-11-21, 759001c), the codec context initialization no longer sets the
> get_buffer field (nor release_buffer, of course).
> 
> -    s->get_buffer          = avcodec_default_get_buffer;
> -    s->release_buffer      = avcodec_default_release_buffer;
> +    s->get_buffer2         = avcodec_default_get_buffer2;
> 
> Note that the fork ignores the problem since they dropped
> FF_API_OLD_DECODE_AUDIO at the 54->55 version bump (refcounted frames),
> while the test in FFmpeg is currently for the 56 bump.
> 
> I realize it was a bit exaggerated to state that avcodec_decode_audio3() is
> broken: it only prints an obnoxious warning. The patch I posted removes that
> warning by initing the get_buffer, but only when needed (no point in leaving
> an obsolete field loaded unless necessary). I realize this is strange it did
> work since I did not address the release_buffer part.

your patch is ok but get_buffer_internal() seems to call
avctx->release_buffer() on the failure path unconditionally
i didnt check if that can be reached, but it should probably be
under a null pointer check anyway

Thanks

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

DNS cache poisoning attacks, popular search engine, Google internet authority
dont be evil, please
-------------- 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/20131230/29cfc7b0/attachment.asc>


More information about the ffmpeg-devel mailing list