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

[Marton Balint]

On Mon, 27 Mar 2023, Andreas Rheinhardt wrote:

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

Ok, will apply.

Regards,
Marton