diff mbox series

[FFmpeg-devel,1/7] avformat/vivo: Do not use the general expression evaluator for parsing a floating point value

Message ID 20211205211907.30010-1-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/7] avformat/vivo: Do not use the general expression evaluator for parsing a floating point value
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/makex86 warning New warnings during build
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished
andriy/makeppc warning New warnings during build

Commit Message

Michael Niedermayer Dec. 5, 2021, 9:19 p.m. UTC
Fixes: Timeout
Fixes: 41564/clusterfuzz-testcase-minimized-ffmpeg_dem_VIVO_fuzzer-6309014024093696

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/vivo.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Anton Khirnov Dec. 6, 2021, 9:25 a.m. UTC | #1
Quoting Michael Niedermayer (2021-12-05 22:19:01)
> Fixes: Timeout
> Fixes: 41564/clusterfuzz-testcase-minimized-ffmpeg_dem_VIVO_fuzzer-6309014024093696
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/vivo.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/libavformat/vivo.c b/libavformat/vivo.c
> index b2904cd25a7..8e819d910b7 100644
> --- a/libavformat/vivo.c
> +++ b/libavformat/vivo.c
> @@ -206,11 +206,12 @@ static int vivo_read_header(AVFormatContext *s)
>                      return AVERROR_INVALIDDATA;
>                  value_used = 1;
>              } else if (!strcmp(key, "FPS")) {
> -                AVRational tmp;
> +                double d;
> +                if (sscanf(value, "%f", &d) != 1)
> +                    return AVERROR_INVALIDDATA;
>  
>                  value_used = 1;
> -                if (!av_parse_ratio(&tmp, value, 10000, AV_LOG_WARNING, s))
> -                    fps = av_inv_q(tmp);
> +                fps = av_d2q(1/d, 10000);
>              }
>  
>              if (!value_used)
> -- 
> 2.17.1

Won't this be inexact? You might also skip parsing the field if the
relevant key has already been seen.
Michael Niedermayer Dec. 6, 2021, 10:35 a.m. UTC | #2
On Mon, Dec 06, 2021 at 10:25:01AM +0100, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2021-12-05 22:19:01)
> > Fixes: Timeout
> > Fixes: 41564/clusterfuzz-testcase-minimized-ffmpeg_dem_VIVO_fuzzer-6309014024093696
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavformat/vivo.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libavformat/vivo.c b/libavformat/vivo.c
> > index b2904cd25a7..8e819d910b7 100644
> > --- a/libavformat/vivo.c
> > +++ b/libavformat/vivo.c
> > @@ -206,11 +206,12 @@ static int vivo_read_header(AVFormatContext *s)
> >                      return AVERROR_INVALIDDATA;
> >                  value_used = 1;
> >              } else if (!strcmp(key, "FPS")) {
> > -                AVRational tmp;
> > +                double d;
> > +                if (sscanf(value, "%f", &d) != 1)
> > +                    return AVERROR_INVALIDDATA;
> >  
> >                  value_used = 1;
> > -                if (!av_parse_ratio(&tmp, value, 10000, AV_LOG_WARNING, s))
> > -                    fps = av_inv_q(tmp);
> > +                fps = av_d2q(1/d, 10000);
> >              }
> >  
> >              if (!value_used)
> > -- 
> > 2.17.1
> 
> Won't this be inexact? 

you mean the 1/d or in general ?
the code before used av_expr_parse_and_eval() in av_parse_ratio() so was
using floating point already. The 1/d could be done in AVRational if you
prefer?
The intermediate is inexact, the end result i would expect to be exact
as it should be the "closest" fraction with num and den below 10000

we also could do the whole without floating point. The actual fields
all where of the format AB.XY so 2 digits after the decimal dot.
if it was always that then its very easy to parse. But if some
oddball 3E1 or .99 something turns up that could add bugs for little
gain


> You might also skip parsing the field if the
> relevant key has already been seen.

yes, but it didnt do that before, ill add a seperate patch for that

thx

[...]
Anton Khirnov Dec. 6, 2021, 2:53 p.m. UTC | #3
Quoting Michael Niedermayer (2021-12-06 11:35:50)
> On Mon, Dec 06, 2021 at 10:25:01AM +0100, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2021-12-05 22:19:01)
> > > Fixes: Timeout
> > > Fixes: 41564/clusterfuzz-testcase-minimized-ffmpeg_dem_VIVO_fuzzer-6309014024093696
> > > 
> > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavformat/vivo.c | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/libavformat/vivo.c b/libavformat/vivo.c
> > > index b2904cd25a7..8e819d910b7 100644
> > > --- a/libavformat/vivo.c
> > > +++ b/libavformat/vivo.c
> > > @@ -206,11 +206,12 @@ static int vivo_read_header(AVFormatContext *s)
> > >                      return AVERROR_INVALIDDATA;
> > >                  value_used = 1;
> > >              } else if (!strcmp(key, "FPS")) {
> > > -                AVRational tmp;
> > > +                double d;
> > > +                if (sscanf(value, "%f", &d) != 1)
> > > +                    return AVERROR_INVALIDDATA;
> > >  
> > >                  value_used = 1;
> > > -                if (!av_parse_ratio(&tmp, value, 10000, AV_LOG_WARNING, s))
> > > -                    fps = av_inv_q(tmp);
> > > +                fps = av_d2q(1/d, 10000);
> > >              }
> > >  
> > >              if (!value_used)
> > > -- 
> > > 2.17.1
> > 
> > Won't this be inexact? 
> 
> you mean the 1/d or in general ?
> the code before used av_expr_parse_and_eval() in av_parse_ratio() so was
> using floating point already. The 1/d could be done in AVRational if you
> prefer?
> The intermediate is inexact, the end result i would expect to be exact
> as it should be the "closest" fraction with num and den below 10000
> 
> we also could do the whole without floating point. The actual fields
> all where of the format AB.XY so 2 digits after the decimal dot.
> if it was always that then its very easy to parse. But if some
> oddball 3E1 or .99 something turns up that could add bugs for little
> gain

I mean involving floats at all. If that's already the case, then I don't
object to the patch (though it would be nice to know if it is in fact
fixed point and parse it as such).
Marvin Scholz Dec. 6, 2021, 3 p.m. UTC | #4
On 5 Dec 2021, at 22:19, Michael Niedermayer wrote:

> Fixes: Timeout
> Fixes: 
> 41564/clusterfuzz-testcase-minimized-ffmpeg_dem_VIVO_fuzzer-6309014024093696
>
> Found-by: continuous fuzzing process 
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/vivo.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/libavformat/vivo.c b/libavformat/vivo.c
> index b2904cd25a7..8e819d910b7 100644
> --- a/libavformat/vivo.c
> +++ b/libavformat/vivo.c
> @@ -206,11 +206,12 @@ static int vivo_read_header(AVFormatContext *s)
>                      return AVERROR_INVALIDDATA;
>                  value_used = 1;
>              } else if (!strcmp(key, "FPS")) {
> -                AVRational tmp;
> +                double d;
> +                if (sscanf(value, "%f", &d) != 1)
> +                    return AVERROR_INVALIDDATA;
>

Shouldn't this use av_sscanf, so it's not locale dependent?

>                  value_used = 1;
> -                if (!av_parse_ratio(&tmp, value, 10000, 
> AV_LOG_WARNING, s))
> -                    fps = av_inv_q(tmp);
> +                fps = av_d2q(1/d, 10000);
>              }
>
>              if (!value_used)
> -- 
> 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".
diff mbox series

Patch

diff --git a/libavformat/vivo.c b/libavformat/vivo.c
index b2904cd25a7..8e819d910b7 100644
--- a/libavformat/vivo.c
+++ b/libavformat/vivo.c
@@ -206,11 +206,12 @@  static int vivo_read_header(AVFormatContext *s)
                     return AVERROR_INVALIDDATA;
                 value_used = 1;
             } else if (!strcmp(key, "FPS")) {
-                AVRational tmp;
+                double d;
+                if (sscanf(value, "%f", &d) != 1)
+                    return AVERROR_INVALIDDATA;
 
                 value_used = 1;
-                if (!av_parse_ratio(&tmp, value, 10000, AV_LOG_WARNING, s))
-                    fps = av_inv_q(tmp);
+                fps = av_d2q(1/d, 10000);
             }
 
             if (!value_used)