[FFmpeg-devel] [PATCH v3 2/4] cbs_mpeg2: Improve checks for invalid values

[Andreas Rheinhardt]

Mark Thompson:
> On 23/04/2019 23:55, Andreas Rheinhardt wrote:
>> horizontal/vertical_size_value (containing the twelve least significant
>> bits of the frame size) mustn't be zero according to the specifications;
>> and the value 0 is forbidden for the colour_description elements.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
>> ---
>> The actual function calls after macro expansion are the same as in the
>> earlier versions with the exception of the extra_bit_slice calls.
>>  libavcodec/cbs_mpeg2.c                 | 14 ++++++++------
>>  libavcodec/cbs_mpeg2_syntax_template.c | 20 ++++++++++----------
>>  2 files changed, 18 insertions(+), 16 deletions(-)
>>
>> diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c
>> index cdde68ea38..fc21745a51 100644
>> --- a/libavcodec/cbs_mpeg2.c
>> +++ b/libavcodec/cbs_mpeg2.c
>> @@ -41,20 +41,22 @@
>>  #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
>>  
>>  #define ui(width, name) \
>> -        xui(width, name, current->name, 0)
>> +        xui(width, name, current->name, 0, MAX_UINT_BITS(width), 0)
>> +#define uir(width, name, range_min, range_max) \
>> +        xui(width, name, current->name, range_min, range_max, 0)
>>  #define uis(width, name, subs, ...) \
>> -        xui(width, name, current->name, subs, __VA_ARGS__)
>> +        xui(width, name, current->name, 0, MAX_UINT_BITS(width), subs, __VA_ARGS__)
>>  
>>  
>>  #define READ
>>  #define READWRITE read
>>  #define RWContext GetBitContext
>>  
>> -#define xui(width, name, var, subs, ...) do { \
>> +#define xui(width, name, var, range_min, range_max, subs, ...) do { \
>>          uint32_t value = 0; \
>>          CHECK(ff_cbs_read_unsigned(ctx, rw, width, #name, \
>>                                     SUBSCRIPTS(subs, __VA_ARGS__), \
>> -                                   &value, 0, (1 << width) - 1)); \
>> +                                   &value, range_min, range_max)); \
>>          var = value; \
>>      } while (0)
>>  
>> @@ -81,10 +83,10 @@
>>  #define READWRITE write
>>  #define RWContext PutBitContext
>>  
>> -#define xui(width, name, var, subs, ...) do { \
>> +#define xui(width, name, var, range_min, range_max, subs, ...) do { \
>>          CHECK(ff_cbs_write_unsigned(ctx, rw, width, #name, \
>>                                      SUBSCRIPTS(subs, __VA_ARGS__), \
>> -                                    var, 0, (1 << width) - 1)); \
>> +                                    var, range_min, range_max)); \
>>      } while (0)
>>  
>>  #define marker_bit() do { \
>> diff --git a/libavcodec/cbs_mpeg2_syntax_template.c b/libavcodec/cbs_mpeg2_syntax_template.c
>> index 10aaea7734..27dcaad316 100644
>> --- a/libavcodec/cbs_mpeg2_syntax_template.c
>> +++ b/libavcodec/cbs_mpeg2_syntax_template.c
>> @@ -26,8 +26,8 @@ static int FUNC(sequence_header)(CodedBitstreamContext *ctx, RWContext *rw,
>>  
>>      ui(8,  sequence_header_code);
>>  
>> -    ui(12, horizontal_size_value);
>> -    ui(12, vertical_size_value);
>> +    uir(12, horizontal_size_value, 1, 4095);
>> +    uir(12, vertical_size_value,   1, 4095);
>>  
>>      mpeg2->horizontal_size = current->horizontal_size_value;
>>      mpeg2->vertical_size   = current->vertical_size_value;
>> @@ -79,7 +79,7 @@ static int FUNC(user_data)(CodedBitstreamContext *ctx, RWContext *rw,
>>  #endif
>>  
>>      for (k = 0; k < current->user_data_length; k++)
>> -        xui(8, user_data, current->user_data[k], 0);
>> +        xui(8, user_data, current->user_data[k], 0, 255, 0);
> 
> This could include the subscript while you're changing it.
> 
> uis(8, user_data[k], 1, k);
> 
Yes, James remarked the same.
>>  
>>      return 0;
>>  }
>> @@ -125,9 +125,9 @@ static int FUNC(sequence_display_extension)(CodedBitstreamContext *ctx, RWContex
>>  
>>      ui(1, colour_description);
>>      if (current->colour_description) {
>> -        ui(8, colour_primaries);
>> -        ui(8, transfer_characteristics);
>> -        ui(8, matrix_coefficients);
>> +        uir(8, colour_primaries,         1, 255);
>> +        uir(8, transfer_characteristics, 1, 255);
>> +        uir(8, matrix_coefficients,      1, 255);
> 
> I'm a bit unsure about this one.  While the zero value is indeed banned by the standard, it isn't uncommon to find (at least in H.264) streams with zeroes in these fields.  (The libavcodec encoder for MPEG-2 is happy to write such a stream, too.)  Start code aliasing is presumably the reason for the standard disallowing zeroes, but they probably won't actually be a problem unless display_horizontal_size happens to have a specific range of bad values (in which case we already fail in a different way).
> 
Yeah, that is certainly because of start code emulation. What do you
think about correcting these values while reading? Would be especially
good considering that mpeg2_metadata under certain circumstances
created such files in the first place.

- Andreas