diff mbox series

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

Message ID 20200913133856.1411-1-onemda@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel] avfilter/avf_concat: check for possible integer overflow
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Paul B Mahol Sept. 13, 2020, 1:38 p.m. UTC
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(+)

Comments

Nicolas George Sept. 13, 2020, 2:14 p.m. UTC | #1
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.
Paul B Mahol Sept. 13, 2020, 2:35 p.m. UTC | #2
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.
Nicolas George Sept. 13, 2020, 2:39 p.m. UTC | #3
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.
Marton Balint Sept. 13, 2020, 4:39 p.m. UTC | #4
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".
Nicolas George Sept. 13, 2020, 4:59 p.m. UTC | #5
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,
diff mbox series

Patch

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 */