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

[Andreas Cadhalpun]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-vorbisdec-reject-rangebits-0.patch
Type: text/x-diff
Size: 1203 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160103/6b3c1927/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-vorbisdec-reject-channel-mapping-with-less-than-two-.patch
Type: text/x-diff
Size: 1434 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160103/6b3c1927/attachment-0001.patch>