| Message ID | 20170723223336.GA22084@tdjones.localdomain |
|---|---|
| State | New |
| Headers | show |
2017-07-24 0:33 GMT+02:00 Tyler Jones <tdjones879@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@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
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@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@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
2017-07-24 2:46 GMT+02:00 Tyler Jones <tdjones879@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@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@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
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@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@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@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
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; + } + mode_setup->transformtype = get_bits(gb, 16); + if (mode_setup->transformtype) { + av_log(vc->avctx, AV_LOG_ERROR, "Invalid transform type, must equal 0.\n"); + return AVERROR_INVALIDDATA; + } GET_VALIDATED_INDEX(mode_setup->mapping, 8, vc->mapping_count); ff_dlog(NULL, " %u mode: blockflag %d, windowtype %d, transformtype %d, mapping %d\n", @@ -969,7 +977,11 @@ static int vorbis_parse_id_hdr(vorbis_context *vc) return AVERROR_INVALIDDATA; } - vc->version = get_bits_long(gb, 32); //FIXME check 0 + vc->version = get_bits_long(gb, 32); + if (vc->version) { + av_log(vc->avctx, AV_LOG_ERROR, "Invalid version number\n"); + return AVERROR_INVALIDDATA; + } vc->audio_channels = get_bits(gb, 8); if (vc->audio_channels <= 0) { av_log(vc->avctx, AV_LOG_ERROR, "Invalid number of channels\n");
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@gmail.com> --- libavcodec/vorbisdec.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)