[FFmpeg-devel] [PATCH v2 1/2] avcodec: Fix warnings with signed/unsigned compare in bitstream.h

[Marton Balint]

On Sat, 25 Mar 2023, Andreas Rheinhardt wrote:

> Devin Heitmueller:
>> When including the header in decklink_enc.cpp it would be fed
>> through the C++ compiler rather than the C compiler, which has
>> more strict warnings when comparing signed/unsigned values.
>>
>> Make the local variables unsigned to match the arguments they are
>> being passed for those functions.
>>
>> Signed-off-by: Devin Heitmueller <dheitmueller at ltnglobal.com>
>> ---
>>  libavcodec/bytestream.h | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/libavcodec/bytestream.h b/libavcodec/bytestream.h
>> index d0033f14f3..67080604b9 100644
>> --- a/libavcodec/bytestream.h
>> +++ b/libavcodec/bytestream.h
>> @@ -180,7 +180,7 @@ static av_always_inline void bytestream2_skipu(GetByteContext *g,
>>  static av_always_inline void bytestream2_skip_p(PutByteContext *p,
>>                                                  unsigned int size)
>>  {
>> -    int size2;
>> +    unsigned int size2;
>>      if (p->eof)
>>          return;
>>      size2 = FFMIN(p->buffer_end - p->buffer, size);
>> @@ -268,7 +268,7 @@ static av_always_inline unsigned int bytestream2_get_buffer(GetByteContext *g,
>>                                                              uint8_t *dst,
>>                                                              unsigned int size)
>>  {
>> -    int size2 = FFMIN(g->buffer_end - g->buffer, size);
>> +    unsigned int size2 = FFMIN(g->buffer_end - g->buffer, size);
>>      memcpy(dst, g->buffer, size2);
>>      g->buffer += size2;
>>      return size2;
>> @@ -287,7 +287,7 @@ static av_always_inline unsigned int bytestream2_put_buffer(PutByteContext *p,
>>                                                              const uint8_t *src,
>>                                                              unsigned int size)
>>  {
>> -    int size2;
>> +    unsigned int size2;
>>      if (p->eof)
>>          return 0;
>>      size2 = FFMIN(p->buffer_end - p->buffer, size);
>> @@ -311,7 +311,7 @@ static av_always_inline void bytestream2_set_buffer(PutByteContext *p,
>>                                                      const uint8_t c,
>>                                                      unsigned int size)
>>  {
>> -    int size2;
>> +    unsigned int size2;
>>      if (p->eof)
>>          return;
>>      size2 = FFMIN(p->buffer_end - p->buffer, size);
>> @@ -348,7 +348,7 @@ static av_always_inline unsigned int bytestream2_copy_buffer(PutByteContext *p,
>>                                                               GetByteContext *g,
>>                                                               unsigned int size)
>>  {
>> -    int size2;
>> +    unsigned int size2;
>>
>>      if (p->eof)
>>          return 0;
>
> The bytestream APIs are allowed to overread if the buffer is padded and
> the user manages this himself. So you are not allowed to presume that
> g->buffer_end - g->buffer is positive.

I am not sure if overread/overwrote is a supported state for these 
functions. As far as I see bytestream2_get_buffer, bytestream2_put_buffer, 
bytestream2_copy_buffer and bytestream2_set_buffer just crashes if 
buffer_end < buffer because sooner or later memcpy/memset gets a negative 
value. There are no special checks to handle it.

Regards,
Marton