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

[Tyler Jones]

On Mon, Jul 24, 2017 at 02:54:01AM +0200, Carl Eugen Hoyos wrote:
> 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.

My point was that the spec declares the stream undecodable, not to prove that they are
invalid. I communicated that poorly.

> > 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.

I'll do that instead.
 
> > 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?

You are correct, these cases have behaved the way they do for years. It was a
genuine question however, would it be prefered to log these similar errors and quit
decoding only when concerned about strict compliance? If so, I'll do that instead.
I was just following the convention in the file and mistakenly believed it followed
best practices, but I should've first checked against other decoders since it has
been left alone for so long.

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/5774f394/attachment.sig>