| Message ID | 20230317150232.17804-1-dheitmueller@ltnglobal.com |
|---|---|
| State | Accepted |
| Commit | 85c62b48e9189d84746009f26a4cff41ad5d4603 |
| Headers | show |
| Series | [FFmpeg-devel,v2,1/2] avcodec: Fix warnings with signed/unsigned compare in bitstream.h | expand |
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@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. - Andreas
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@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
On Sat, Mar 25, 2023 at 1:10 PM Marton Balint <cus@passwd.hu> wrote: > 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. This was the conclusion I came to as well. I couldn't imagine a case where it would ever actually work, since prior to my patch in every case it results in a call to memcpy() with a negative length argument. Devin
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@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
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@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
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;
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@ltnglobal.com> --- libavcodec/bytestream.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)