[FFmpeg-devel] avfilter/avf_concat: check for possible integer overflow

There is nothing much currently that can be done to recover from
this situation so just return AVERROR_BUG error code.

Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 libavfilter/avf_concat.c | 2 ++
 1 file changed, 2 insertions(+)

Paul B Mahol (12020-09-13):
> There is nothing much currently that can be done to recover from
> this situation so just return AVERROR_BUG error code.
> 
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavfilter/avf_concat.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libavfilter/avf_concat.c b/libavfilter/avf_concat.c
> index 5608ed9ac6..295a340515 100644
> --- a/libavfilter/avf_concat.c
> +++ b/libavfilter/avf_concat.c
> @@ -251,6 +251,8 @@ static int send_silence(AVFilterContext *ctx, unsigned in_no, unsigned out_no,
>  
>      if (!rate_tb.den)
>          return AVERROR_BUG;
> +    if (seg_delta < -cat->in[in_no].pts)
> +        return AVERROR_BUG;
>      nb_samples = av_rescale_q(seg_delta - cat->in[in_no].pts,
>                                outlink->time_base, rate_tb);
>      frame_nb_samples = FFMAX(9600, rate_tb.den / 5); /* arbitrary */

Catching the problem here is probably ok.

But it is not a bug in this filter, and therefore AVERROR_BUG is not the
correct error message. I suppose AVERROR_INVALIDDATA would be ok.

On Sun, Sep 13, 2020 at 04:14:30PM +0200, Nicolas George wrote:
> Paul B Mahol (12020-09-13):
> > There is nothing much currently that can be done to recover from
> > this situation so just return AVERROR_BUG error code.
> > 
> > Signed-off-by: Paul B Mahol <onemda@gmail.com>
> > ---
> >  libavfilter/avf_concat.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/libavfilter/avf_concat.c b/libavfilter/avf_concat.c
> > index 5608ed9ac6..295a340515 100644
> > --- a/libavfilter/avf_concat.c
> > +++ b/libavfilter/avf_concat.c
> > @@ -251,6 +251,8 @@ static int send_silence(AVFilterContext *ctx, unsigned in_no, unsigned out_no,
> >  
> >      if (!rate_tb.den)
> >          return AVERROR_BUG;
> > +    if (seg_delta < -cat->in[in_no].pts)
> > +        return AVERROR_BUG;
> >      nb_samples = av_rescale_q(seg_delta - cat->in[in_no].pts,
> >                                outlink->time_base, rate_tb);
> >      frame_nb_samples = FFMAX(9600, rate_tb.den / 5); /* arbitrary */
> 
> Catching the problem here is probably ok.
> 
> But it is not a bug in this filter, and therefore AVERROR_BUG is not the
> correct error message. I suppose AVERROR_INVALIDDATA would be ok.

It is bug in this filter.

Filter should avoid integer overflows.
Also expecting only monotonous timestamps from input is not valid.
Filter should not try to allocate very big number of frame samples.
Filter should not try to allocate negative number of frame samples.
I think this last one should be checked also in calling function.

Paul B Mahol (12020-09-13):
> It is bug in this filter.
> 
> Filter should avoid integer overflows.
> Also expecting only monotonous timestamps from input is not valid.

Requiring monotonous and continuous timestamps on filters input is not
only valid but widely accepted and almost mandatory. No operation is
possible on several streams without valid timestamps. The bug is in the
filters that do not produce valid timestamps; we could mitigate it in
the framework. Individual filters are not the place to implement this.

On Sun, 13 Sep 2020, Paul B Mahol wrote:

> There is nothing much currently that can be done to recover from
> this situation so just return AVERROR_BUG error code.
>
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
> libavfilter/avf_concat.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/libavfilter/avf_concat.c b/libavfilter/avf_concat.c
> index 5608ed9ac6..295a340515 100644
> --- a/libavfilter/avf_concat.c
> +++ b/libavfilter/avf_concat.c
> @@ -251,6 +251,8 @@ static int send_silence(AVFilterContext *ctx, unsigned in_no, unsigned out_no,
>
>     if (!rate_tb.den)
>         return AVERROR_BUG;
> +    if (seg_delta < -cat->in[in_no].pts)
> +        return AVERROR_BUG;

Isn't this supposed to be simply (seg_delta < cat->in[in_no].pts) ?

Thanks,
Marton


>     nb_samples = av_rescale_q(seg_delta - cat->in[in_no].pts,
>                               outlink->time_base, rate_tb);
>     frame_nb_samples = FFMAX(9600, rate_tb.den / 5); /* arbitrary */
> -- 
> 2.17.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Marton Balint (12020-09-13):
> > +    if (seg_delta < -cat->in[in_no].pts)
> > +        return AVERROR_BUG;
> Isn't this supposed to be simply (seg_delta < cat->in[in_no].pts) ?

You are right, thanks for noticing. Was this tested?

Regards,