[FFmpeg-devel] [PATCH 1/7] avformat/avc: Fix undefined shift and assert when reading exp-golomb num

[James Almer]

On 7/9/2020 7:35 AM, Andreas Rheinhardt wrote:
> The current code for parsing unsigned exponential-golomb codes is not
> suitable for long codes: If there are enough leading zeroes, it
> left-shifts 1 by 32 places and uses get_bitsz to read 32 bits, although
> it is only suitable to read 0-25 bits. There is an av_assert2 to
> ensure this when using the uncached bitstream reader; with valid input
> one can still run into the assert and left-shift 1 by 31 which is
> undefined.
> 
> This commit changes this. All valid data is parsed correctly; invalid
> data does no longer lead to undefined behaviour or to asserts. Parsing
> all valid data correctly also necessitated changing the return value to
> unsigned; also an intermediate variable used for parsing signed
> exponential-golomb codes needed to be made unsigned, too, in order to
> parse long signed codes correctly.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> ---
> Moving the function to the beginning is done in preparation for another
> commit that I'll send soon.
> 
>  libavformat/avc.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/libavformat/avc.c b/libavformat/avc.c
> index b5e2921388..55494eb08a 100644
> --- a/libavformat/avc.c
> +++ b/libavformat/avc.c
> @@ -27,6 +27,21 @@
>  #include "avc.h"
>  #include "avio_internal.h"
>  
> +static inline unsigned get_ue_golomb(GetBitContext *gb)
> +{
> +    int i;
> +    for (i = 1; i <= 32; i++) {
> +        if (get_bits_left(gb) < i)
> +            return 0;
> +        if (show_bits1(gb))
> +            break;
> +        skip_bits1(gb);
> +    }
> +    if (i > 32)
> +        return 0;
> +    return get_bits_long(gb, i) - 1;
> +}
> +
>  static const uint8_t *avc_find_startcode_internal(const uint8_t *p, const uint8_t *end)
>  {
>      const uint8_t *a = p + 4 - ((intptr_t)p & 3);
> @@ -318,15 +333,8 @@ static const AVRational avc_sample_aspect_ratio[17] = {
>      {   2,  1 },
>  };
>  
> -static inline int get_ue_golomb(GetBitContext *gb) {
> -    int i;
> -    for (i = 0; i < 32 && !get_bits1(gb); i++)
> -        ;
> -    return get_bitsz(gb, i) + (1 << i) - 1;
> -}
> -
>  static inline int get_se_golomb(GetBitContext *gb) {
> -    int v = get_ue_golomb(gb) + 1;
> +    unsigned v = get_ue_golomb(gb) + 1;
>      int sign = -(v & 1);
>      return ((v >> 1) ^ sign) - sign;
>  }

I think it's best if we use the lavc golomb code instead. When i
suggested to re implement it here i wasn't aware it was shared with lavf
within the golomb_tab.c source file.