[FFmpeg-devel] [PATCH 2/3] vorbisdec: replace get_bits with get_bitsz where n can be 0

[Michael Niedermayer]

On Sun, Jan 03, 2016 at 07:50:39PM +0100, Andreas Cadhalpun wrote:
> On 03.01.2016 02:41, Michael Niedermayer wrote:
> > On Sun, Jan 03, 2016 at 01:36:13AM +0100, Andreas Cadhalpun wrote:
> >> get_bits is documented to only support reading 1-25 bits.
> >> get_bitsz was added for this purpose.
> >>
> >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> >> ---
> >>  libavcodec/vorbisdec.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c
> >> index f773afa..9ba0bce 100644
> >> --- a/libavcodec/vorbisdec.c
> >> +++ b/libavcodec/vorbisdec.c
> >> @@ -169,7 +169,7 @@ static const char idx_err_str[] = "Index value %d out of range (0 - %d) for %s a
> >>      }
> >>  #define GET_VALIDATED_INDEX(idx, bits, limit) \
> >>      {\
> >> -        idx = get_bits(gb, bits);\
> >> +        idx = get_bitsz(gb, bits);\
> >>          VALIDATE_INDEX(idx, limit)\
> >>      }
> > 
> > this looks wrong
> > bits is 8 or ilog() where ilog(0) is not allowed one way or another
> > i think
> > 
> > for example for the audio channels
> > "the numbers read in the above two steps are channel numbers representing
> >  the channel to treat as magnitude and the channel to treat as angle,
> >  respectively. If for any coupling step the angle channel number equals
> >  the magnitude channel number " ... "the stream is undecodable."
> > 
> > when reading 0 bits both would be 0
> > 
> > 
> >>  
> >> @@ -585,7 +585,7 @@ static int vorbis_parse_setup_hdr_floors(vorbis_context *vc)
> >>  
> >>              for (j = 0; j < floor_setup->data.t1.partitions; ++j) {
> >>                  for (k = 0; k < floor_setup->data.t1.class_dimensions[floor_setup->data.t1.partition_class[j]]; ++k, ++floor1_values) {
> >> -                    floor_setup->data.t1.list[floor1_values].x = get_bits(gb, rangebits);
> >> +                    floor_setup->data.t1.list[floor1_values].x = get_bitsz(gb, rangebits);
> > 
> > IIUC
> > "Vector [floor1_x_list] is limited to a maximum length of 65 elements;
> >  a setup indicating more than 65 total elements (including elements 0
> >  and 1 set prior to the read loop) renders the stream undecodable. All
> >  vector [floor1_x_list] element values must be unique within the vector;
> >  a non-unique value renders the stream undecodable. "
> > 
> > suggests that values cannot be 0 as the first is hardcoded to 0
> 
> Thanks a lot for finding the relevant places in the specification.
> 
> Based on that it's probably better to just error out in these cases.
> Patches for that are attached.
> 
> Best regards,
> Andreas
> 

>  vorbisdec.c |    5 +++++
>  1 file changed, 5 insertions(+)
> ba151dadb72b6c74e1139decf4b32c8676ddc58e  0001-vorbisdec-reject-rangebits-0.patch
> From d740a59b6e099c90504d55c65923def1ad6de234 Mon Sep 17 00:00:00 2001
> From: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> Date: Sun, 3 Jan 2016 19:11:24 +0100
> Subject: [PATCH 1/2] vorbisdec: reject rangebits 0
> 
> This causes non-unique elements in floor_setup->data.t1.list, which
> makes the stream undecodable according to the specification.
> 
> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> ---
>  libavcodec/vorbisdec.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c
> index f773afa..2792af1 100644
> --- a/libavcodec/vorbisdec.c
> +++ b/libavcodec/vorbisdec.c
> @@ -573,6 +573,11 @@ static int vorbis_parse_setup_hdr_floors(vorbis_context *vc)
>                  return AVERROR(ENOMEM);
>  
>              rangebits = get_bits(gb, 4);
> +            if (!rangebits) {
> +                av_log(vc->avctx, AV_LOG_ERROR,
> +                       "A rangebits value of 0 is not compliant with the Vorbis I specification.\n");
> +                return AVERROR_INVALIDDATA;
> +            }

this assumes partitions > 0
iam not sure if this is required or not, the spec does not seem to
explicitly state it


>              rangemax = (1 << rangebits);
>              if (rangemax > vc->blocksize[1] / 2) {
>                  av_log(vc->avctx, AV_LOG_ERROR,
> -- 
> 2.6.4
> 

>  vorbisdec.c |    5 +++++
>  1 file changed, 5 insertions(+)
> d8862de6632822fd066a585953e5966fd941ad93  0002-vorbisdec-reject-channel-mapping-with-less-than-two-.patch
> From 09bf8ca4d496518b876639b793cae12066b7ba3b Mon Sep 17 00:00:00 2001
> From: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> Date: Sun, 3 Jan 2016 19:20:54 +0100
> Subject: [PATCH 2/2] vorbisdec: reject channel mapping with less than two
>  channels
> 
> It causes the angle channel number to equal the magnitude channel
> number, which makes the stream undecodable according to the
> specification.


LGTM

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

During times of universal deceit, telling the truth becomes a
revolutionary act. -- George Orwell
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160103/e4d70ac0/attachment.sig>