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

[Andreas Rheinhardt]

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.
> 

True. Seems like this was never a supported case. Objection lifted.

- Andreas