[FFmpeg-devel] [PATCH] avcodec/vorbisdec: Check for legal version, window and transform types

Carl Eugen Hoyos ceffmpeg at gmail.com
Mon Jul 24 03:54:01 EEST 2017


2017-07-24 2:46 GMT+02:00 Tyler Jones <tdjones879 at gmail.com>:
> On Mon, Jul 24, 2017 at 01:52:20AM +0200, Carl Eugen Hoyos wrote:
>> 2017-07-24 0:33 GMT+02:00 Tyler Jones <tdjones879 at gmail.com>:
>> > Vorbis I specification requires that the version number as well as the
>> > window and transform types in the setup header be equal to 0.
>> >
>> > Signed-off-by: Tyler Jones <tdjones879 at gmail.com>
>> > ---
>> >  libavcodec/vorbisdec.c | 18 +++++++++++++++---
>> >  1 file changed, 15 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c
>> > index 2a4f482031..f9c3848c4e 100644
>> > --- a/libavcodec/vorbisdec.c
>> > +++ b/libavcodec/vorbisdec.c
>> > @@ -898,8 +898,16 @@ static int vorbis_parse_setup_hdr_modes(vorbis_context *vc)
>> >          vorbis_mode *mode_setup = &vc->modes[i];
>> >
>> >          mode_setup->blockflag     = get_bits1(gb);
>> > -        mode_setup->windowtype    = get_bits(gb, 16); //FIXME check
>> > -        mode_setup->transformtype = get_bits(gb, 16); //FIXME check
>> > +        mode_setup->windowtype    = get_bits(gb, 16);
>> > +        if (mode_setup->windowtype) {
>> > +            av_log(vc->avctx, AV_LOG_ERROR, "Invalid window type,
>> > must equal 0.\n");
>> > +            return AVERROR_INVALIDDATA;
>>
>> Does this fix anything?
>>
>> By default, FFmpeg decoders should not (and, more so, should not
>> suddenly start to) reject files that can be decoded without any
>> effort.
>> Or are such files already unplayable, the error message was
>> just missing?
>>
>> You can reject such files for strict conformance mode.
>>
>> Carl Eugen
>
> I'll defer to your judgement, but this is how the specifications defines it:
>
> (4.2.4 -- Modes)
>     verify ranges; zero is the only legal value in Vorbis I for [vorbis_mode_windowtype]
>     and [vorbis_mode_transformtype]. [vorbis_mode_mapping] must not be greater than the
>     highest number mapping in use. Any illegal values render the stream undecodable.

My mail was not meant to imply that the values you reject
are valid.

> These values are unused in the decoder and otherwise ignored in the specification.

I may misunderstand this but an unused or ignored value
should never be a reason to reject an input stream by
default.

> What is even the value of storing these values beyond a temporary value?

> I believed that it is important to check these values so that an encoder cannot produce
> a stream that comes out of sync and end up failing a later test anyways.

I don't understand how this argument is related to a decoder
patch.

In any case: Please add a check for
"avctx->strict_std_compliance >= FF_COMPLIANCE_STRICT"
to make it possible to reject such "invalid" files without breaking
playback of such files for unexpecting users.

> In the interest of consistency, there are several identical cases where values
> have the potential to make the stream 'undecodable' even if they have no impact
> on the behavior of the decoder. In all of these other cases, the decoding quits
> immediately. Should these be reverted to only log an error message and not
> return error values?

>From a quick look at git log, these checks were not introduced lately or
am I wrong?

Thank you, Carl Eugen


More information about the ffmpeg-devel mailing list