| Message ID | e1d6e9c8-a1ee-12f9-e8e4-7d9d6ff000f3@googlemail.com |
|---|---|
| State | Accepted |
| Commit | eb751f06db9f627c8b5c63d08836a39f7572bf56 |
| Headers | show |
On 10/16/2016 5:11 PM, Andreas Cadhalpun wrote: > The problem was introduced in commit 1273bc6. > > Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > --- > libavformat/matroskadec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c > index 8847c62..a5d3c0e 100644 > --- a/libavformat/matroskadec.c > +++ b/libavformat/matroskadec.c > @@ -1759,7 +1759,7 @@ static int mkv_field_order(MatroskaDemuxContext *matroska, int64_t field_order) > > /* workaround a bug in our Matroska muxer, introduced in version 57.36 alongside > * this function, and fixed in 57.52 */ > - if (sscanf(matroska->muxingapp, "Lavf%d.%d.%d", &major, &minor, µ) == 3) > + if (matroska->muxingapp && sscanf(matroska->muxingapp, "Lavf%d.%d.%d", &major, &minor, µ) == 3) LGTM. Matroska files are supposed to always have that element, but even ffmpeg used to mux files without it at some point when bitexact flag was enabled, so i guess plenty of files out there are missing it. > bttb = (major == 57 && minor >= 36 && minor <= 51 && micro >= 100); > > switch (field_order) { >
On 10/16/2016 9:30 PM, James Almer wrote: > On 10/16/2016 5:11 PM, Andreas Cadhalpun wrote: >> The problem was introduced in commit 1273bc6. >> >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> >> --- >> libavformat/matroskadec.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c >> index 8847c62..a5d3c0e 100644 >> --- a/libavformat/matroskadec.c >> +++ b/libavformat/matroskadec.c >> @@ -1759,7 +1759,7 @@ static int mkv_field_order(MatroskaDemuxContext *matroska, int64_t field_order) >> >> /* workaround a bug in our Matroska muxer, introduced in version 57.36 alongside >> * this function, and fixed in 57.52 */ >> - if (sscanf(matroska->muxingapp, "Lavf%d.%d.%d", &major, &minor, µ) == 3) >> + if (matroska->muxingapp && sscanf(matroska->muxingapp, "Lavf%d.%d.%d", &major, &minor, µ) == 3) > > LGTM. > > Matroska files are supposed to always have that element, but even ffmpeg used > to mux files without it at some point when bitexact flag was enabled, so i > guess plenty of files out there are missing it. > >> bttb = (major == 57 && minor >= 36 && minor <= 51 && micro >= 100); >> >> switch (field_order) { >> Just tried a file missing the muxingapp element, meaning matroska->muxingapp is NULL, and sscanf simply returns -1 and sets errno to EINVAL. Where does it crash for you and using what file?
Hi, On 17/10/2016 06:49, James Almer wrote: > On 10/16/2016 9:30 PM, James Almer wrote: >> On 10/16/2016 5:11 PM, Andreas Cadhalpun wrote: >>> The problem was introduced in commit 1273bc6. >>> >>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> >>> --- >>> libavformat/matroskadec.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c >>> index 8847c62..a5d3c0e 100644 >>> --- a/libavformat/matroskadec.c >>> +++ b/libavformat/matroskadec.c >>> @@ -1759,7 +1759,7 @@ static int mkv_field_order(MatroskaDemuxContext *matroska, int64_t field_order) >>> >>> /* workaround a bug in our Matroska muxer, introduced in version 57.36 alongside >>> * this function, and fixed in 57.52 */ >>> - if (sscanf(matroska->muxingapp, "Lavf%d.%d.%d", &major, &minor, µ) == 3) >>> + if (matroska->muxingapp && sscanf(matroska->muxingapp, "Lavf%d.%d.%d", &major, &minor, µ) == 3) >> LGTM. >> >> Matroska files are supposed to always have that element, but even ffmpeg used >> to mux files without it at some point when bitexact flag was enabled, so i >> guess plenty of files out there are missing it. >> >>> bttb = (major == 57 && minor >= 36 && minor <= 51 && micro >= 100); >>> >>> switch (field_order) { >>> > Just tried a file missing the muxingapp element, meaning matroska->muxingapp > is NULL, and sscanf simply returns -1 and sets errno to EINVAL. > > Where does it crash for you and using what file? FWIW, I've just written a quick test on my machine, and an "sscanf(NULL, "%d", &i)" segfaults.
On Sun, 16 Oct 2016 22:11:03 +0200 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> wrote: > The problem was introduced in commit 1273bc6. > > Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > --- > libavformat/matroskadec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c > index 8847c62..a5d3c0e 100644 > --- a/libavformat/matroskadec.c > +++ b/libavformat/matroskadec.c > @@ -1759,7 +1759,7 @@ static int mkv_field_order(MatroskaDemuxContext *matroska, int64_t field_order) > > /* workaround a bug in our Matroska muxer, introduced in version 57.36 alongside > * this function, and fixed in 57.52 */ > - if (sscanf(matroska->muxingapp, "Lavf%d.%d.%d", &major, &minor, µ) == 3) > + if (matroska->muxingapp && sscanf(matroska->muxingapp, "Lavf%d.%d.%d", &major, &minor, µ) == 3) > bttb = (major == 57 && minor >= 36 && minor <= 51 && micro >= 100); > > switch (field_order) { Why do we even have this dumb check? The files that have already produced will be misinterpreted by compliant demuxers, and adding ugly workarounds is unfair to those compliant demuxers.
On Mon, Oct 17, 2016 at 12:13 PM, wm4 <nfxjfg@googlemail.com> wrote: > On Sun, 16 Oct 2016 22:11:03 +0200 > Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> wrote: > >> The problem was introduced in commit 1273bc6. >> >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> >> --- >> libavformat/matroskadec.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c >> index 8847c62..a5d3c0e 100644 >> --- a/libavformat/matroskadec.c >> +++ b/libavformat/matroskadec.c >> @@ -1759,7 +1759,7 @@ static int mkv_field_order(MatroskaDemuxContext *matroska, int64_t field_order) >> >> /* workaround a bug in our Matroska muxer, introduced in version 57.36 alongside >> * this function, and fixed in 57.52 */ >> - if (sscanf(matroska->muxingapp, "Lavf%d.%d.%d", &major, &minor, µ) == 3) >> + if (matroska->muxingapp && sscanf(matroska->muxingapp, "Lavf%d.%d.%d", &major, &minor, µ) == 3) >> bttb = (major == 57 && minor >= 36 && minor <= 51 && micro >= 100); >> >> switch (field_order) { > > Why do we even have this dumb check? > > The files that have already produced will be misinterpreted by > compliant demuxers, and adding ugly workarounds is unfair to those > compliant demuxers. This way you can at least fix it by remuxing with a newer ffmpeg, instead of it being broken forever. - Hendrik
On 10/17/2016 4:47 AM, Benoit Fouet wrote: > Hi, > > On 17/10/2016 06:49, James Almer wrote: >> On 10/16/2016 9:30 PM, James Almer wrote: >>> On 10/16/2016 5:11 PM, Andreas Cadhalpun wrote: >>>> The problem was introduced in commit 1273bc6. >>>> >>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> >>>> --- >>>> libavformat/matroskadec.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c >>>> index 8847c62..a5d3c0e 100644 >>>> --- a/libavformat/matroskadec.c >>>> +++ b/libavformat/matroskadec.c >>>> @@ -1759,7 +1759,7 @@ static int mkv_field_order(MatroskaDemuxContext *matroska, int64_t field_order) >>>> /* workaround a bug in our Matroska muxer, introduced in version 57.36 alongside >>>> * this function, and fixed in 57.52 */ >>>> - if (sscanf(matroska->muxingapp, "Lavf%d.%d.%d", &major, &minor, µ) == 3) >>>> + if (matroska->muxingapp && sscanf(matroska->muxingapp, "Lavf%d.%d.%d", &major, &minor, µ) == 3) >>> LGTM. >>> >>> Matroska files are supposed to always have that element, but even ffmpeg used >>> to mux files without it at some point when bitexact flag was enabled, so i >>> guess plenty of files out there are missing it. >>> >>>> bttb = (major == 57 && minor >= 36 && minor <= 51 && micro >= 100); >>>> switch (field_order) { >>>> >> Just tried a file missing the muxingapp element, meaning matroska->muxingapp >> is NULL, and sscanf simply returns -1 and sets errno to EINVAL. >> >> Where does it crash for you and using what file? > > FWIW, I've just written a quick test on my machine, and an "sscanf(NULL, "%d", &i)" segfaults. Guess it's up to the implementation, then. I tried with mingw-w64 only. The documentation for sscanf says it sets errno to EINVAL if *format is NULL, but doesn't mention *str. Patch still LGTM. It doesn't hurt either way.
On 17.10.2016 15:44, James Almer wrote: > On 10/17/2016 4:47 AM, Benoit Fouet wrote: >> On 17/10/2016 06:49, James Almer wrote: >>> On 10/16/2016 9:30 PM, James Almer wrote: >>>> On 10/16/2016 5:11 PM, Andreas Cadhalpun wrote: >>>>> The problem was introduced in commit 1273bc6. >>>>> >>>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> >>>>> --- >>>>> libavformat/matroskadec.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c >>>>> index 8847c62..a5d3c0e 100644 >>>>> --- a/libavformat/matroskadec.c >>>>> +++ b/libavformat/matroskadec.c >>>>> @@ -1759,7 +1759,7 @@ static int mkv_field_order(MatroskaDemuxContext *matroska, int64_t field_order) >>>>> /* workaround a bug in our Matroska muxer, introduced in version 57.36 alongside >>>>> * this function, and fixed in 57.52 */ >>>>> - if (sscanf(matroska->muxingapp, "Lavf%d.%d.%d", &major, &minor, µ) == 3) >>>>> + if (matroska->muxingapp && sscanf(matroska->muxingapp, "Lavf%d.%d.%d", &major, &minor, µ) == 3) >>>> LGTM. >>>> >>>> Matroska files are supposed to always have that element, but even ffmpeg used >>>> to mux files without it at some point when bitexact flag was enabled, so i >>>> guess plenty of files out there are missing it. >>>> >>>>> bttb = (major == 57 && minor >= 36 && minor <= 51 && micro >= 100); >>>>> switch (field_order) { >>>>> >>> Just tried a file missing the muxingapp element, meaning matroska->muxingapp >>> is NULL, and sscanf simply returns -1 and sets errno to EINVAL. >>> >>> Where does it crash for you and using what file? >> >> FWIW, I've just written a quick test on my machine, and an "sscanf(NULL, "%d", &i)" segfaults. With glibc it crashes: $ gdb --batch -ex r -ex bt -ex q --args ./ffmpeg_g -i ./id_b42167853bffe74a3a93ea312837dd8a2a25a951.matroska -f null /dev/null [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". ffmpeg version N-82012-gd3874b7 Copyright (c) 2000-2016 the FFmpeg developers built with gcc 6.2.0 (Debian 6.2.0-6) 20161010 configuration: --disable-optimizations libavutil 55. 32.100 / 55. 32.100 libavcodec 57. 61.103 / 57. 61.103 libavformat 57. 52.100 / 57. 52.100 libavdevice 57. 0.102 / 57. 0.102 libavfilter 6. 64.100 / 6. 64.100 libswscale 4. 1.100 / 4. 1.100 libswresample 2. 2.100 / 2. 2.100 [matroska,webm @ 0x2380fc0] Unknown EBML doctype 'ma@roUka' Truncating packet of size 134217728 to 1551 Truncating packet of size 1431346 to 1507 Truncating packet of size 4467 to 1401 [matroska,webm @ 0x2380fc0] Duplicate element [matroska,webm @ 0x2380fc0] Unknown/unsupported AVCodecID ��OPU=. Ignoring attempt to set invalid timebase 0/1 for st:0 Program received signal SIGSEGV, Segmentation fault. rawmemchr () at ../sysdeps/x86_64/rawmemchr.S:37 37 ../sysdeps/x86_64/rawmemchr.S: Datei oder Verzeichnis nicht gefunden. #0 rawmemchr () at ../sysdeps/x86_64/rawmemchr.S:37 #1 0x00007ffff4b383a2 in _IO_str_init_static_internal (sf=sf@entry=0x7fffffffd360, ptr=ptr@entry=0x0, size=size@entry=0, pstart=pstart@entry=0x0) at strops.c:41 #2 0x00007ffff4b27567 in __GI___isoc99_vsscanf (string=0x0, format=0x15e597c "Lavf%d.%d.%d", args=args@entry=0x7fffffffd488) at isoc99_vsscanf.c:41 #3 0x00007ffff4b27507 in __isoc99_sscanf (s=<optimized out>, format=<optimized out>) at isoc99_sscanf.c:31 #4 0x00000000006ca2eb in mkv_field_order (matroska=0x2381ba0, field_order=2) at src/libavformat/matroskadec.c:1762 #5 0x00000000006cc0c2 in matroska_parse_tracks (s=0x2380fc0) at src/libavformat/matroskadec.c:2293 #6 0x00000000006ccc83 in matroska_read_header (s=0x2380fc0) at src/libavformat/matroskadec.c:2470 #7 0x00000000007bbd13 in avformat_open_input (ps=0x7fffffffd948, filename=0x7fffffffe3a4 "./id_b42167853bffe74a3a93ea312837dd8a2a25a951.matroska", fmt=0x0, options=0x2380d68) at src/libavformat/utils.c:587 #8 0x0000000000411aeb in open_input_file (o=0x7fffffffda50, filename=0x7fffffffe3a4 "./id_b42167853bffe74a3a93ea312837dd8a2a25a951.matroska") at src/ffmpeg_opt.c:997 #9 0x000000000041a9af in open_files (l=0x2380c98, inout=0x154c797 "input", open_file=0x411330 <open_input_file>) at src/ffmpeg_opt.c:3135 #10 0x000000000041ab10 in ffmpeg_parse_options (argc=6, argv=0x7fffffffe048) at src/ffmpeg_opt.c:3175 #11 0x000000000042e78a in main (argc=6, argv=0x7fffffffe048) at src/ffmpeg.c:4564 > Guess it's up to the implementation, then. I tried with mingw-w64 only. > The documentation for sscanf says it sets errno to EINVAL if *format is NULL, > but doesn't mention *str. > > Patch still LGTM. It doesn't hurt either way. Pushed. Best regards, Andreas
diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c index 8847c62..a5d3c0e 100644 --- a/libavformat/matroskadec.c +++ b/libavformat/matroskadec.c @@ -1759,7 +1759,7 @@ static int mkv_field_order(MatroskaDemuxContext *matroska, int64_t field_order) /* workaround a bug in our Matroska muxer, introduced in version 57.36 alongside * this function, and fixed in 57.52 */ - if (sscanf(matroska->muxingapp, "Lavf%d.%d.%d", &major, &minor, µ) == 3) + if (matroska->muxingapp && sscanf(matroska->muxingapp, "Lavf%d.%d.%d", &major, &minor, µ) == 3) bttb = (major == 57 && minor >= 36 && minor <= 51 && micro >= 100); switch (field_order) {
The problem was introduced in commit 1273bc6. Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> --- libavformat/matroskadec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)