diff mbox

[FFmpeg-devel,1/2] avcodec/rangecoder: Do not increase the pointer beyond the buffer

Message ID 20170813221555.17253-1-michael@niedermayer.cc
State Accepted
Commit c359c51947c9ac925cc4a5d1893ef20ea1d3b4c8
Headers show

Commit Message

Michael Niedermayer Aug. 13, 2017, 10:15 p.m. UTC 
Fixes: undefined behavior

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/rangecoder.c | 1 +
 libavcodec/rangecoder.h | 8 ++++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

Comments

James Almer Aug. 13, 2017, 10:18 p.m. UTC | #1 
On 8/13/2017 7:15 PM, Michael Niedermayer wrote:
> Fixes: undefined behavior
> 
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/rangecoder.c | 1 +
>  libavcodec/rangecoder.h | 8 ++++++--
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/rangecoder.c b/libavcodec/rangecoder.c
> index 0bb79c880e..0d53bef076 100644
> --- a/libavcodec/rangecoder.c
> +++ b/libavcodec/rangecoder.c
> @@ -58,6 +58,7 @@ av_cold void ff_init_range_decoder(RangeCoder *c, const uint8_t *buf,
>  
>      c->low         = AV_RB16(c->bytestream);
>      c->bytestream += 2;
> +    c->overread    = 0;
>      if (c->low >= 0xFF00) {
>          c->low = 0xFF00;
>          c->bytestream_end = c->bytestream;
> diff --git a/libavcodec/rangecoder.h b/libavcodec/rangecoder.h
> index c3e81d0dcb..44af88b8f5 100644
> --- a/libavcodec/rangecoder.h
> +++ b/libavcodec/rangecoder.h
> @@ -42,6 +42,8 @@ typedef struct RangeCoder {
>      uint8_t *bytestream_start;
>      uint8_t *bytestream;
>      uint8_t *bytestream_end;
> +    int overread;
> +#define MAX_OVERREAD 2
>  } RangeCoder;
>  
>  void ff_init_range_encoder(RangeCoder *c, uint8_t *buf, int buf_size);
> @@ -106,9 +108,11 @@ static inline void refill(RangeCoder *c)
>      if (c->range < 0x100) {
>          c->range <<= 8;
>          c->low   <<= 8;
> -        if (c->bytestream < c->bytestream_end)
> +        if (c->bytestream < c->bytestream_end) {
>              c->low += c->bytestream[0];
> -        c->bytestream++;
> +            c->bytestream++;
> +        } else
> +            c->overread ++;
>      }
>  }

Wouldn't it be better to port this to the bytestream2 reading api?
Michael Niedermayer Aug. 14, 2017, 11:07 p.m. UTC | #2 
On Sun, Aug 13, 2017 at 07:18:11PM -0300, James Almer wrote:
> On 8/13/2017 7:15 PM, Michael Niedermayer wrote:
> > Fixes: undefined behavior
> > 
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/rangecoder.c | 1 +
> >  libavcodec/rangecoder.h | 8 ++++++--
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libavcodec/rangecoder.c b/libavcodec/rangecoder.c
> > index 0bb79c880e..0d53bef076 100644
> > --- a/libavcodec/rangecoder.c
> > +++ b/libavcodec/rangecoder.c
> > @@ -58,6 +58,7 @@ av_cold void ff_init_range_decoder(RangeCoder *c, const uint8_t *buf,
> >  
> >      c->low         = AV_RB16(c->bytestream);
> >      c->bytestream += 2;
> > +    c->overread    = 0;
> >      if (c->low >= 0xFF00) {
> >          c->low = 0xFF00;
> >          c->bytestream_end = c->bytestream;
> > diff --git a/libavcodec/rangecoder.h b/libavcodec/rangecoder.h
> > index c3e81d0dcb..44af88b8f5 100644
> > --- a/libavcodec/rangecoder.h
> > +++ b/libavcodec/rangecoder.h
> > @@ -42,6 +42,8 @@ typedef struct RangeCoder {
> >      uint8_t *bytestream_start;
> >      uint8_t *bytestream;
> >      uint8_t *bytestream_end;
> > +    int overread;
> > +#define MAX_OVERREAD 2
> >  } RangeCoder;
> >  
> >  void ff_init_range_encoder(RangeCoder *c, uint8_t *buf, int buf_size);
> > @@ -106,9 +108,11 @@ static inline void refill(RangeCoder *c)
> >      if (c->range < 0x100) {
> >          c->range <<= 8;
> >          c->low   <<= 8;
> > -        if (c->bytestream < c->bytestream_end)
> > +        if (c->bytestream < c->bytestream_end) {
> >              c->low += c->bytestream[0];
> > -        c->bytestream++;
> > +            c->bytestream++;
> > +        } else
> > +            c->overread ++;
> >      }
> >  }
> 
> Wouldn't it be better to port this to the bytestream2 reading api?

this is speed relevant code, i am not sure bytestream2 wouldnt add
overhead. In fact i wasnt entirely sure keeping track of the overread
bytes is a great idea. But i didnt see an easy way to avoid that.


[...]
James Almer Aug. 15, 2017, 2:30 a.m. UTC | #3 
On 8/14/2017 8:07 PM, Michael Niedermayer wrote:
> On Sun, Aug 13, 2017 at 07:18:11PM -0300, James Almer wrote:
>> On 8/13/2017 7:15 PM, Michael Niedermayer wrote:
>>> Fixes: undefined behavior
>>>
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>  libavcodec/rangecoder.c | 1 +
>>>  libavcodec/rangecoder.h | 8 ++++++--
>>>  2 files changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libavcodec/rangecoder.c b/libavcodec/rangecoder.c
>>> index 0bb79c880e..0d53bef076 100644
>>> --- a/libavcodec/rangecoder.c
>>> +++ b/libavcodec/rangecoder.c
>>> @@ -58,6 +58,7 @@ av_cold void ff_init_range_decoder(RangeCoder *c, const uint8_t *buf,
>>>  
>>>      c->low         = AV_RB16(c->bytestream);
>>>      c->bytestream += 2;
>>> +    c->overread    = 0;
>>>      if (c->low >= 0xFF00) {
>>>          c->low = 0xFF00;
>>>          c->bytestream_end = c->bytestream;
>>> diff --git a/libavcodec/rangecoder.h b/libavcodec/rangecoder.h
>>> index c3e81d0dcb..44af88b8f5 100644
>>> --- a/libavcodec/rangecoder.h
>>> +++ b/libavcodec/rangecoder.h
>>> @@ -42,6 +42,8 @@ typedef struct RangeCoder {
>>>      uint8_t *bytestream_start;
>>>      uint8_t *bytestream;
>>>      uint8_t *bytestream_end;
>>> +    int overread;
>>> +#define MAX_OVERREAD 2
>>>  } RangeCoder;
>>>  
>>>  void ff_init_range_encoder(RangeCoder *c, uint8_t *buf, int buf_size);
>>> @@ -106,9 +108,11 @@ static inline void refill(RangeCoder *c)
>>>      if (c->range < 0x100) {
>>>          c->range <<= 8;
>>>          c->low   <<= 8;
>>> -        if (c->bytestream < c->bytestream_end)
>>> +        if (c->bytestream < c->bytestream_end) {
>>>              c->low += c->bytestream[0];
>>> -        c->bytestream++;
>>> +            c->bytestream++;
>>> +        } else
>>> +            c->overread ++;
>>>      }
>>>  }
>>
>> Wouldn't it be better to port this to the bytestream2 reading api?
> 
> this is speed relevant code, i am not sure bytestream2 wouldnt add
> overhead. In fact i wasnt entirely sure keeping track of the overread
> bytes is a great idea. But i didnt see an easy way to avoid that.

Probably no overhead, since renorm_encoder() can use the unchecked
reader much like it's doing it now.

In any case, the buffer may be both read and written to, which means
you'd have to split things in two to use both the put and get
bytestream2 APIs, so it may not be worth doing (and probably the reason
why it's not already been done in the first place).
Michael Niedermayer Aug. 16, 2017, 4:27 p.m. UTC | #4 
On Mon, Aug 14, 2017 at 11:30:06PM -0300, James Almer wrote:
> On 8/14/2017 8:07 PM, Michael Niedermayer wrote:
> > On Sun, Aug 13, 2017 at 07:18:11PM -0300, James Almer wrote:
> >> On 8/13/2017 7:15 PM, Michael Niedermayer wrote:
> >>> Fixes: undefined behavior
> >>>
> >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>> ---
> >>>  libavcodec/rangecoder.c | 1 +
> >>>  libavcodec/rangecoder.h | 8 ++++++--
> >>>  2 files changed, 7 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/libavcodec/rangecoder.c b/libavcodec/rangecoder.c
> >>> index 0bb79c880e..0d53bef076 100644
> >>> --- a/libavcodec/rangecoder.c
> >>> +++ b/libavcodec/rangecoder.c
> >>> @@ -58,6 +58,7 @@ av_cold void ff_init_range_decoder(RangeCoder *c, const uint8_t *buf,
> >>>  
> >>>      c->low         = AV_RB16(c->bytestream);
> >>>      c->bytestream += 2;
> >>> +    c->overread    = 0;
> >>>      if (c->low >= 0xFF00) {
> >>>          c->low = 0xFF00;
> >>>          c->bytestream_end = c->bytestream;
> >>> diff --git a/libavcodec/rangecoder.h b/libavcodec/rangecoder.h
> >>> index c3e81d0dcb..44af88b8f5 100644
> >>> --- a/libavcodec/rangecoder.h
> >>> +++ b/libavcodec/rangecoder.h
> >>> @@ -42,6 +42,8 @@ typedef struct RangeCoder {
> >>>      uint8_t *bytestream_start;
> >>>      uint8_t *bytestream;
> >>>      uint8_t *bytestream_end;
> >>> +    int overread;
> >>> +#define MAX_OVERREAD 2
> >>>  } RangeCoder;
> >>>  
> >>>  void ff_init_range_encoder(RangeCoder *c, uint8_t *buf, int buf_size);
> >>> @@ -106,9 +108,11 @@ static inline void refill(RangeCoder *c)
> >>>      if (c->range < 0x100) {
> >>>          c->range <<= 8;
> >>>          c->low   <<= 8;
> >>> -        if (c->bytestream < c->bytestream_end)
> >>> +        if (c->bytestream < c->bytestream_end) {
> >>>              c->low += c->bytestream[0];
> >>> -        c->bytestream++;
> >>> +            c->bytestream++;
> >>> +        } else
> >>> +            c->overread ++;
> >>>      }
> >>>  }
> >>
> >> Wouldn't it be better to port this to the bytestream2 reading api?
> > 
> > this is speed relevant code, i am not sure bytestream2 wouldnt add
> > overhead. In fact i wasnt entirely sure keeping track of the overread
> > bytes is a great idea. But i didnt see an easy way to avoid that.
> 
> Probably no overhead, since renorm_encoder() can use the unchecked
> reader much like it's doing it now.
> 
> In any case, the buffer may be both read and written to, which means
> you'd have to split things in two to use both the put and get
> bytestream2 APIs, so it may not be worth doing (and probably the reason
> why it's not already been done in the first place).

yes, so unless there are objections i will apply this in a day or 2

thx

[...]
Michael Niedermayer Aug. 18, 2017, 9:33 a.m. UTC | #5 
On Wed, Aug 16, 2017 at 06:27:19PM +0200, Michael Niedermayer wrote:
> On Mon, Aug 14, 2017 at 11:30:06PM -0300, James Almer wrote:
> > On 8/14/2017 8:07 PM, Michael Niedermayer wrote:
> > > On Sun, Aug 13, 2017 at 07:18:11PM -0300, James Almer wrote:
> > >> On 8/13/2017 7:15 PM, Michael Niedermayer wrote:
> > >>> Fixes: undefined behavior
> > >>>
> > >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > >>> ---
> > >>>  libavcodec/rangecoder.c | 1 +
> > >>>  libavcodec/rangecoder.h | 8 ++++++--
> > >>>  2 files changed, 7 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/libavcodec/rangecoder.c b/libavcodec/rangecoder.c
> > >>> index 0bb79c880e..0d53bef076 100644
> > >>> --- a/libavcodec/rangecoder.c
> > >>> +++ b/libavcodec/rangecoder.c
> > >>> @@ -58,6 +58,7 @@ av_cold void ff_init_range_decoder(RangeCoder *c, const uint8_t *buf,
> > >>>  
> > >>>      c->low         = AV_RB16(c->bytestream);
> > >>>      c->bytestream += 2;
> > >>> +    c->overread    = 0;
> > >>>      if (c->low >= 0xFF00) {
> > >>>          c->low = 0xFF00;
> > >>>          c->bytestream_end = c->bytestream;
> > >>> diff --git a/libavcodec/rangecoder.h b/libavcodec/rangecoder.h
> > >>> index c3e81d0dcb..44af88b8f5 100644
> > >>> --- a/libavcodec/rangecoder.h
> > >>> +++ b/libavcodec/rangecoder.h
> > >>> @@ -42,6 +42,8 @@ typedef struct RangeCoder {
> > >>>      uint8_t *bytestream_start;
> > >>>      uint8_t *bytestream;
> > >>>      uint8_t *bytestream_end;
> > >>> +    int overread;
> > >>> +#define MAX_OVERREAD 2
> > >>>  } RangeCoder;
> > >>>  
> > >>>  void ff_init_range_encoder(RangeCoder *c, uint8_t *buf, int buf_size);
> > >>> @@ -106,9 +108,11 @@ static inline void refill(RangeCoder *c)
> > >>>      if (c->range < 0x100) {
> > >>>          c->range <<= 8;
> > >>>          c->low   <<= 8;
> > >>> -        if (c->bytestream < c->bytestream_end)
> > >>> +        if (c->bytestream < c->bytestream_end) {
> > >>>              c->low += c->bytestream[0];
> > >>> -        c->bytestream++;
> > >>> +            c->bytestream++;
> > >>> +        } else
> > >>> +            c->overread ++;
> > >>>      }
> > >>>  }
> > >>
> > >> Wouldn't it be better to port this to the bytestream2 reading api?
> > > 
> > > this is speed relevant code, i am not sure bytestream2 wouldnt add
> > > overhead. In fact i wasnt entirely sure keeping track of the overread
> > > bytes is a great idea. But i didnt see an easy way to avoid that.
> > 
> > Probably no overhead, since renorm_encoder() can use the unchecked
> > reader much like it's doing it now.
> > 
> > In any case, the buffer may be both read and written to, which means
> > you'd have to split things in two to use both the put and get
> > bytestream2 APIs, so it may not be worth doing (and probably the reason
> > why it's not already been done in the first place).
> 
> yes, so unless there are objections i will apply this in a day or 2

applied

[...]
diff mbox

Patch

diff --git a/libavcodec/rangecoder.c b/libavcodec/rangecoder.c
index 0bb79c880e..0d53bef076 100644
--- a/libavcodec/rangecoder.c
+++ b/libavcodec/rangecoder.c
@@ -58,6 +58,7 @@  av_cold void ff_init_range_decoder(RangeCoder *c, const uint8_t *buf,
 
     c->low         = AV_RB16(c->bytestream);
     c->bytestream += 2;
+    c->overread    = 0;
     if (c->low >= 0xFF00) {
         c->low = 0xFF00;
         c->bytestream_end = c->bytestream;
diff --git a/libavcodec/rangecoder.h b/libavcodec/rangecoder.h
index c3e81d0dcb..44af88b8f5 100644
--- a/libavcodec/rangecoder.h
+++ b/libavcodec/rangecoder.h
@@ -42,6 +42,8 @@  typedef struct RangeCoder {
     uint8_t *bytestream_start;
     uint8_t *bytestream;
     uint8_t *bytestream_end;
+    int overread;
+#define MAX_OVERREAD 2
 } RangeCoder;
 
 void ff_init_range_encoder(RangeCoder *c, uint8_t *buf, int buf_size);
@@ -106,9 +108,11 @@  static inline void refill(RangeCoder *c)
     if (c->range < 0x100) {
         c->range <<= 8;
         c->low   <<= 8;
-        if (c->bytestream < c->bytestream_end)
+        if (c->bytestream < c->bytestream_end) {
             c->low += c->bytestream[0];
-        c->bytestream++;
+            c->bytestream++;
+        } else
+            c->overread ++;
     }
 }