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

[Tyler Jones]

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.

These values are unused in the decoder and otherwise ignored in the specification.
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.

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?

Thanks,

Tyler Jones
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170723/039c41bd/attachment.sig>