| Message ID | 20180707202911.39315-1-ottoka@posteo.de |
|---|---|
| State | New |
| Headers | show |
On Sat, Jul 07, 2018 at 10:29:11PM +0200, Karsten Otto wrote: > Check the MPEG version ID for the reserved bit pattern 01, and abort header > parsing in that case. This reduces the chance of misinterpreting arbitrary > data as a valid frame start, and prevents the resulting audio artifacts. > --- > libavcodec/mpegaudiodecheader.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/mpegaudiodecheader.c b/libavcodec/mpegaudiodecheader.c > index 6cc79f18b5..23029f186a 100644 > --- a/libavcodec/mpegaudiodecheader.c > +++ b/libavcodec/mpegaudiodecheader.c > @@ -46,10 +46,11 @@ int avpriv_mpegaudio_decode_header(MPADecodeHeader *s, uint32_t header) > if (header & (1<<20)) { > s->lsf = (header & (1<<19)) ? 0 : 1; > mpeg25 = 0; > - } else { > + } else if (!(header & (1<<19))) { > s->lsf = 1; > mpeg25 = 1; > - } > + } else > + return -1; // reserved id This check (assuming it breaks no existing mp3) would fit better in ff_mpa_check_header() thanks [...]
> Am 08.07.2018 um 10:58 schrieb Michael Niedermayer <michael@niedermayer.cc>: > > Signierter PGP-Teil > On Sat, Jul 07, 2018 at 10:29:11PM +0200, Karsten Otto wrote: >> Check the MPEG version ID for the reserved bit pattern 01, and abort header >> parsing in that case. This reduces the chance of misinterpreting arbitrary >> data as a valid frame start, and prevents the resulting audio artifacts. >> --- >> libavcodec/mpegaudiodecheader.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/libavcodec/mpegaudiodecheader.c b/libavcodec/mpegaudiodecheader.c >> index 6cc79f18b5..23029f186a 100644 >> --- a/libavcodec/mpegaudiodecheader.c >> +++ b/libavcodec/mpegaudiodecheader.c >> @@ -46,10 +46,11 @@ int avpriv_mpegaudio_decode_header(MPADecodeHeader *s, uint32_t header) >> if (header & (1<<20)) { >> s->lsf = (header & (1<<19)) ? 0 : 1; >> mpeg25 = 0; >> - } else { >> + } else if (!(header & (1<<19))) { >> s->lsf = 1; >> mpeg25 = 1; >> - } >> + } else >> + return -1; // reserved id > > This check (assuming it breaks no existing mp3) would fit better in > ff_mpa_check_header() > I did not want to put it in ff_mpa_check_header(), since that is supposed to be a "fast header check for resync", emphasis *fast*. But if you think an extra check is acceptable, I can certainly put it there. Will send an update patch. Re breaking existing mp3: I haven't checked the official ISO standard, but all other free reference material agrees bit pattern 01 is supposed to be reserved. I have tested both with an mpeg 1 layer 3 (first branch) and mpeg2.5 layer 3 (second) branch, with files I found on samples.ffmpeg.org. The patch also passes make fate (though I don't quite understand what that does, I assume it checks for such things). Cheers, Karsten
diff --git a/libavcodec/mpegaudiodecheader.c b/libavcodec/mpegaudiodecheader.c index 6cc79f18b5..23029f186a 100644 --- a/libavcodec/mpegaudiodecheader.c +++ b/libavcodec/mpegaudiodecheader.c @@ -46,10 +46,11 @@ int avpriv_mpegaudio_decode_header(MPADecodeHeader *s, uint32_t header) if (header & (1<<20)) { s->lsf = (header & (1<<19)) ? 0 : 1; mpeg25 = 0; - } else { + } else if (!(header & (1<<19))) { s->lsf = 1; mpeg25 = 1; - } + } else + return -1; // reserved id s->layer = 4 - ((header >> 17) & 3); /* extract frequency */