[FFmpeg-devel] [PATCH v3 2/3] Fix leak and crash in tee muxer when open_slave fails

[Marton Balint]

On Tue, 29 Mar 2016, sebechlebskyjan at gmail.com wrote:

The commit message is a bit measleasing, because as far as I see a crash 
could not happen with the old code, only a leak. Obviously the patch and 
the commit description is correct and bsfs needs to be checked in the new 
setup.

Also try to use commit messages in a "topic: short description" form, 
e.g.:

avformat/tee: fix leak in tee muxer when open_slave fails

> From: Jan Sebechlebsky <sebechlebskyjan at gmail.com>
>
> Calling close_slave in case error is to be returned from open_slave
> will free allocated resources.
>
> Since failure can happen before bsfs array is initialized,
> close_slave must check that bsfs is not NULL before accessing
> tee_slave->bsfs[i] element.
>
> Signed-off-by: Jan Sebechlebsky <sebechlebskyjan at gmail.com>
> ---
> libavformat/tee.c | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/libavformat/tee.c b/libavformat/tee.c
> index 50c81c4..f5dc443 100644
> --- a/libavformat/tee.c
> +++ b/libavformat/tee.c
> @@ -141,12 +141,14 @@ static void close_slave(TeeSlave* tee_slave)
>     unsigned i;
>
>     avf = tee_slave->avf;
> -    for (i=0; i < avf->nb_streams; ++i) {
> -        AVBitStreamFilterContext *bsf_next, *bsf = tee_slave->bsfs[i];
> -        while (bsf) {
> -            bsf_next = bsf->next;
> -            av_bitstream_filter_close(bsf);
> -            bsf = bsf_next;
> +    if (tee_slave->bsfs) {
> +        for (i=0; i < avf->nb_streams; ++i) {

Same breathing space as in the previous patch...

> +            AVBitStreamFilterContext *bsf_next, *bsf = tee_slave->bsfs[i];
> +            while (bsf) {
> +                bsf_next = bsf->next;
> +                av_bitstream_filter_close(bsf);
> +                bsf = bsf_next;
> +            }
>         }
>     }
>     av_freep(&tee_slave->stream_map);
> @@ -197,6 +199,7 @@ static int open_slave(AVFormatContext *avf, char *slave, TeeSlave *tee_slave)
>     ret = avformat_alloc_output_context2(&avf2, NULL, format, filename);
>     if (ret < 0)
>         goto end;
> +    tee_slave->avf = avf2;
>     av_dict_copy(&avf2->metadata, avf->metadata, 0);
>     avf2->opaque   = avf->opaque;
>     avf2->io_open  = avf->io_open;
> @@ -276,7 +279,6 @@ static int open_slave(AVFormatContext *avf, char *slave, TeeSlave *tee_slave)
>         goto end;
>     }
> 
> -    tee_slave->avf = avf2;
>     tee_slave->bsfs = av_calloc(avf2->nb_streams, sizeof(TeeSlave));
>     if (!tee_slave->bsfs) {
>         ret = AVERROR(ENOMEM);
> @@ -291,7 +293,8 @@ static int open_slave(AVFormatContext *avf, char *slave, TeeSlave *tee_slave)
>                 av_log(avf, AV_LOG_ERROR,
>                        "Specifier separator in '%s' is '%c', but only characters '%s' "
>                        "are allowed\n", entry->key, *spec, slave_bsfs_spec_sep);
> -                return AVERROR(EINVAL);
> +                ret = AVERROR(EINVAL);
> +                goto end;
>             }
>             spec++; /* consume separator */
>         }
> --

This looks good to me as well. Nicolas?

Thanks,
Marton