diff mbox series

[FFmpeg-devel,2/2] lavfi/avf_concat: check input timestamp.

Message ID 20200809105653.93859-2-george@nsup.org
State New
Headers show
Series [FFmpeg-devel,1/2] lavfi/avf_concat: add some doxy.
Related show

Checks

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

Commit Message

Nicolas George Aug. 9, 2020, 10:56 a.m. UTC
If an input has an undefined timestamp, the computation
for silence at stitches can overflow.

Partial fix for trac ticket #8843.

Signed-off-by: Nicolas George <george@nsup.org>
---
 libavfilter/avf_concat.c | 4 ++++
 1 file changed, 4 insertions(+)


"Partial fix" means it does not segfault and exits with an error instead.

The actual problem in this ticket is amix sending a frame with no PTS.

Comments

Nicolas George Aug. 22, 2020, 11:03 a.m. UTC | #1
Nicolas George (12020-08-09):
> If an input has an undefined timestamp, the computation
> for silence at stitches can overflow.
> 
> Partial fix for trac ticket #8843.
> 
> Signed-off-by: Nicolas George <george@nsup.org>
> ---
>  libavfilter/avf_concat.c | 4 ++++
>  1 file changed, 4 insertions(+)

Ping? Will apply sometime soon if nobody objects.

Regards,
Michael Niedermayer Aug. 23, 2020, 8:19 a.m. UTC | #2
On Sun, Aug 09, 2020 at 12:56:53PM +0200, Nicolas George wrote:
> If an input has an undefined timestamp, the computation
> for silence at stitches can overflow.
> 
> Partial fix for trac ticket #8843.
> 
> Signed-off-by: Nicolas George <george@nsup.org>
> ---
>  libavfilter/avf_concat.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> 
> "Partial fix" means it does not segfault and exits with an error instead.
> 
> The actual problem in this ticket is amix sending a frame with no PTS.
> 
> 
> diff --git a/libavfilter/avf_concat.c b/libavfilter/avf_concat.c
> index 246628498a..f376f72754 100644
> --- a/libavfilter/avf_concat.c
> +++ b/libavfilter/avf_concat.c
> @@ -179,6 +179,10 @@ static int push_frame(AVFilterContext *ctx, unsigned in_no, AVFrame *buf)
>      AVFilterLink *outlink = ctx->outputs[out_no];
>      struct concat_in *in = &cat->in[in_no];
>  
> +    if (buf->pts == AV_NOPTS_VALUE) {
> +        av_log(ctx, AV_LOG_ERROR, "Frame without timestamp on input %d\n", in_no);
> +        return AVERROR(EINVAL);
> +    }

what if the timestamp is "AV_NOPTS_VALUE + 1" ?
would that work or overflow ?

also could the filter not use the previous frame pts + "duration" when a PTS
is unavailable (maybe that would make sense in some cases) (with a warning)

thx

[...]
Nicolas George Aug. 23, 2020, 10:31 a.m. UTC | #3
Michael Niedermayer (12020-08-23):
> > +    if (buf->pts == AV_NOPTS_VALUE) {
> > +        av_log(ctx, AV_LOG_ERROR, "Frame without timestamp on input %d\n", in_no);
> > +        return AVERROR(EINVAL);
> > +    }
> 
> what if the timestamp is "AV_NOPTS_VALUE + 1" ?
> would that work or overflow ?

I have no idea, I suspect it would overflow too. But "every single
filter that needs valid timestamps" is not the place to protect against
overflows caused by widely invalid timestamps. The framework is; I have
considered going at it, some kind of "require_valid_pts" flag on input
pads, but it is not my priority right now.

This test is already too much, it should be part of the framework, but
it is very simple and protects against a real current bug.

> also could the filter not use the previous frame pts + "duration" when a PTS
> is unavailable (maybe that would make sense in some cases) (with a warning)

Not really.

First, again, an individual filter is not the place to implement an
heuristic.

Second, duration is not part of lavfi's working at all, and could not be
without a significant redesign of many filters, and increasing the
latency.

Regards,
Paul B Mahol Aug. 30, 2020, 10:33 a.m. UTC | #4
On 8/23/20, Nicolas George <george@nsup.org> wrote:
> Michael Niedermayer (12020-08-23):
>> > +    if (buf->pts == AV_NOPTS_VALUE) {
>> > +        av_log(ctx, AV_LOG_ERROR, "Frame without timestamp on input
>> > %d\n", in_no);
>> > +        return AVERROR(EINVAL);
>> > +    }
>>
>> what if the timestamp is "AV_NOPTS_VALUE + 1" ?
>> would that work or overflow ?
>
> I have no idea, I suspect it would overflow too. But "every single
> filter that needs valid timestamps" is not the place to protect against
> overflows caused by widely invalid timestamps. The framework is; I have
> considered going at it, some kind of "require_valid_pts" flag on input
> pads, but it is not my priority right now.

What is valid timestamp?

Filter currently crashes because it receives nopts timestamps.
Why it crashes at all?

Does it crash because it access out of range of allocated memory?
If yes, make sure this patch is real fix and not just band-aid.

>
> This test is already too much, it should be part of the framework, but
> it is very simple and protects against a real current bug.
>
>> also could the filter not use the previous frame pts + "duration" when a
>> PTS
>> is unavailable (maybe that would make sense in some cases) (with a
>> warning)
>
> Not really.
>
> First, again, an individual filter is not the place to implement an
> heuristic.
>
> Second, duration is not part of lavfi's working at all, and could not be
> without a significant redesign of many filters, and increasing the
> latency.
>
> Regards,
>
> --
>  Nicolas George
>
diff mbox series

Patch

diff --git a/libavfilter/avf_concat.c b/libavfilter/avf_concat.c
index 246628498a..f376f72754 100644
--- a/libavfilter/avf_concat.c
+++ b/libavfilter/avf_concat.c
@@ -179,6 +179,10 @@  static int push_frame(AVFilterContext *ctx, unsigned in_no, AVFrame *buf)
     AVFilterLink *outlink = ctx->outputs[out_no];
     struct concat_in *in = &cat->in[in_no];
 
+    if (buf->pts == AV_NOPTS_VALUE) {
+        av_log(ctx, AV_LOG_ERROR, "Frame without timestamp on input %d\n", in_no);
+        return AVERROR(EINVAL);
+    }
     buf->pts = av_rescale_q(buf->pts, inlink->time_base, outlink->time_base);
     in->pts = buf->pts;
     in->nb_frames++;