| 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 | expand |
| 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 |
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.
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 [...]
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).
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 --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)
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(-)