[1/4] put_bits: bounds check buffer

Message ID 20170226175813.20428-2-stebbins@jetheaddev.com
State New
Headers show

Commit Message

John Stebbins Feb. 26, 2017, 5:58 p.m.
This prevents invalid writes outside put_bits' buffer.

It also has the side effect of allowing measurement of the required
size of a buffer without the need to pre-allocate an over-sized buffer.

This fixes a crash in aacenc.c where it could write past the end of the
allocated packet, which is allocated to be the max size allowed by the
aac spec.  aacenc.c uses the above feature to check the size
of encoded data and try again when the size is too large.
---
 libavcodec/put_bits.h | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Comments

Vittorio Giovara Feb. 28, 2017, 3:27 p.m. | #1
On Sun, Feb 26, 2017 at 12:58 PM, John Stebbins <stebbins@jetheaddev.com> wrote:
> This prevents invalid writes outside put_bits' buffer.
>
> It also has the side effect of allowing measurement of the required
> size of a buffer without the need to pre-allocate an over-sized buffer.
>
> This fixes a crash in aacenc.c where it could write past the end of the
> allocated packet, which is allocated to be the max size allowed by the
> aac spec.  aacenc.c uses the above feature to check the size
> of encoded data and try again when the size is too large.
> ---
>  libavcodec/put_bits.h | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/libavcodec/put_bits.h b/libavcodec/put_bits.h
> index 17666fa..30b1dd2 100644
> --- a/libavcodec/put_bits.h
> +++ b/libavcodec/put_bits.h
> @@ -89,10 +89,14 @@ static inline void flush_put_bits(PutBitContext *s)
>      while (s->bit_left < 32) {
>          /* XXX: should test end of buffer */
>  #ifdef BITSTREAM_WRITER_LE
> -        *s->buf_ptr++ = s->bit_buf;
> +        if (s->buf_ptr < s->buf_end)
> +            *s->buf_ptr = s->bit_buf;
> +        s->buf_ptr++;
>          s->bit_buf  >>= 8;
>  #else
> -        *s->buf_ptr++ = s->bit_buf >> 24;
> +        if (s->buf_ptr < s->buf_end)
> +            *s->buf_ptr = s->bit_buf >> 24;
> +        s->buf_ptr++;
>          s->bit_buf  <<= 8;
>  #endif
>          s->bit_left  += 8;

shouldn't you move the buffer pointer only if it's within bounds?
namely, do s->buf_ptr++; only when s->buf_ptr < s->buf_end
same in the other chunk
Luca Barbato Feb. 28, 2017, 3:40 p.m. | #2
On 28/02/2017 16:27, Vittorio Giovara wrote:
> On Sun, Feb 26, 2017 at 12:58 PM, John Stebbins <stebbins@jetheaddev.com> wrote:
>> This prevents invalid writes outside put_bits' buffer.
>>
>> It also has the side effect of allowing measurement of the required
>> size of a buffer without the need to pre-allocate an over-sized buffer.
>>
>> This fixes a crash in aacenc.c where it could write past the end of the
>> allocated packet, which is allocated to be the max size allowed by the
>> aac spec.  aacenc.c uses the above feature to check the size
>> of encoded data and try again when the size is too large.
>> ---
>>  libavcodec/put_bits.h | 14 ++++++++++----
>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/libavcodec/put_bits.h b/libavcodec/put_bits.h
>> index 17666fa..30b1dd2 100644
>> --- a/libavcodec/put_bits.h
>> +++ b/libavcodec/put_bits.h
>> @@ -89,10 +89,14 @@ static inline void flush_put_bits(PutBitContext *s)
>>      while (s->bit_left < 32) {
>>          /* XXX: should test end of buffer */
>>  #ifdef BITSTREAM_WRITER_LE
>> -        *s->buf_ptr++ = s->bit_buf;
>> +        if (s->buf_ptr < s->buf_end)
>> +            *s->buf_ptr = s->bit_buf;
>> +        s->buf_ptr++;
>>          s->bit_buf  >>= 8;
>>  #else
>> -        *s->buf_ptr++ = s->bit_buf >> 24;
>> +        if (s->buf_ptr < s->buf_end)
>> +            *s->buf_ptr = s->bit_buf >> 24;
>> +        s->buf_ptr++;
>>          s->bit_buf  <<= 8;
>>  #endif
>>          s->bit_left  += 8;
> 
> shouldn't you move the buffer pointer only if it's within bounds?
> namely, do s->buf_ptr++; only when s->buf_ptr < s->buf_end
> same in the other chunk
> 

We'd have to change the functions that report the nominal size written then.

lu
John Stebbins Feb. 28, 2017, 4:16 p.m. | #3
On 02/28/2017 08:40 AM, Luca Barbato wrote:
> On 28/02/2017 16:27, Vittorio Giovara wrote:
>> On Sun, Feb 26, 2017 at 12:58 PM, John Stebbins <stebbins@jetheaddev.com> wrote:
>>> This prevents invalid writes outside put_bits' buffer.
>>>
>>> It also has the side effect of allowing measurement of the required
>>> size of a buffer without the need to pre-allocate an over-sized buffer.
>>>
>>> This fixes a crash in aacenc.c where it could write past the end of the
>>> allocated packet, which is allocated to be the max size allowed by the
>>> aac spec.  aacenc.c uses the above feature to check the size
>>> of encoded data and try again when the size is too large.
>>> ---
>>>  libavcodec/put_bits.h | 14 ++++++++++----
>>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/libavcodec/put_bits.h b/libavcodec/put_bits.h
>>> index 17666fa..30b1dd2 100644
>>> --- a/libavcodec/put_bits.h
>>> +++ b/libavcodec/put_bits.h
>>> @@ -89,10 +89,14 @@ static inline void flush_put_bits(PutBitContext *s)
>>>      while (s->bit_left < 32) {
>>>          /* XXX: should test end of buffer */
>>>  #ifdef BITSTREAM_WRITER_LE
>>> -        *s->buf_ptr++ = s->bit_buf;
>>> +        if (s->buf_ptr < s->buf_end)
>>> +            *s->buf_ptr = s->bit_buf;
>>> +        s->buf_ptr++;
>>>          s->bit_buf  >>= 8;
>>>  #else
>>> -        *s->buf_ptr++ = s->bit_buf >> 24;
>>> +        if (s->buf_ptr < s->buf_end)
>>> +            *s->buf_ptr = s->bit_buf >> 24;
>>> +        s->buf_ptr++;
>>>          s->bit_buf  <<= 8;
>>>  #endif
>>>          s->bit_left  += 8;
>> shouldn't you move the buffer pointer only if it's within bounds?
>> namely, do s->buf_ptr++; only when s->buf_ptr < s->buf_end
>> same in the other chunk
>>
> We'd have to change the functions that report the nominal size written then.
>
>

Correct, the idea is that you can still call put_bits_count() to discover how much would have been written, even when
the buffer is too small.  So you can do things like put_bits_init((s, NULL, 0), then call execute some code that
"writes" using put_bits and measure what size buffer you need with put_bits_count.  aacenc.c does something like this. 
It doesn't set a zero size buffer, but it sets a buffer that may be too small, and when it has written too much it
decreases lambda and tries again.
John Stebbins Feb. 28, 2017, 4:35 p.m. | #4
On 02/28/2017 09:16 AM, John Stebbins wrote:
> On 02/28/2017 08:40 AM, Luca Barbato wrote:
>> On 28/02/2017 16:27, Vittorio Giovara wrote:
>>> On Sun, Feb 26, 2017 at 12:58 PM, John Stebbins <stebbins@jetheaddev.com> wrote:
>>>> This prevents invalid writes outside put_bits' buffer.
>>>>
>>>> It also has the side effect of allowing measurement of the required
>>>> size of a buffer without the need to pre-allocate an over-sized buffer.
>>>>
>>>> This fixes a crash in aacenc.c where it could write past the end of the
>>>> allocated packet, which is allocated to be the max size allowed by the
>>>> aac spec.  aacenc.c uses the above feature to check the size
>>>> of encoded data and try again when the size is too large.
>>>> ---
>>>>  libavcodec/put_bits.h | 14 ++++++++++----
>>>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/libavcodec/put_bits.h b/libavcodec/put_bits.h
>>>> index 17666fa..30b1dd2 100644
>>>> --- a/libavcodec/put_bits.h
>>>> +++ b/libavcodec/put_bits.h
>>>> @@ -89,10 +89,14 @@ static inline void flush_put_bits(PutBitContext *s)
>>>>      while (s->bit_left < 32) {
>>>>          /* XXX: should test end of buffer */
>>>>  #ifdef BITSTREAM_WRITER_LE
>>>> -        *s->buf_ptr++ = s->bit_buf;
>>>> +        if (s->buf_ptr < s->buf_end)
>>>> +            *s->buf_ptr = s->bit_buf;
>>>> +        s->buf_ptr++;
>>>>          s->bit_buf  >>= 8;
>>>>  #else
>>>> -        *s->buf_ptr++ = s->bit_buf >> 24;
>>>> +        if (s->buf_ptr < s->buf_end)
>>>> +            *s->buf_ptr = s->bit_buf >> 24;
>>>> +        s->buf_ptr++;
>>>>          s->bit_buf  <<= 8;
>>>>  #endif
>>>>          s->bit_left  += 8;
>>> shouldn't you move the buffer pointer only if it's within bounds?
>>> namely, do s->buf_ptr++; only when s->buf_ptr < s->buf_end
>>> same in the other chunk
>>>
>> We'd have to change the functions that report the nominal size written then.
>>
>>
> Correct, the idea is that you can still call put_bits_count() to discover how much would have been written, even when
> the buffer is too small.  So you can do things like put_bits_init((s, NULL, 0), then call execute some code that
> "writes" using put_bits and measure what size buffer you need with put_bits_count.  aacenc.c does something like this. 
> It doesn't set a zero size buffer, but it sets a buffer that may be too small, and when it has written too much it
> decreases lambda and tries again.
>

This conversation gives me a thought on how to improve this patch some.  Perhaps we should modify put_bits_count to
return FFMIN(s->size_in_bits, (s->buf_ptr - s->buf) * 8 + 32 - s->bit_left));  And then create a new function that
returns the number of bits that *would* have been written if the buffer was not too small.  This gives an extra layer of
safety that we never had before.  Code that doesn't intend to overwrite the buffer, but does, would get the actual size
of the buffer when calling put_bits_count and therefore wouldn't accidentally access outside the buffer (e.g.
avio_write(io, buf, put_bits_count(s) / 8)).  Code that knows it may overflow the buffer (aacenc.c) would use the new
function to check for overflow.

Would this address your concern Vittorio?
Vittorio Giovara Feb. 28, 2017, 4:52 p.m. | #5
On Tue, Feb 28, 2017 at 11:35 AM, John Stebbins <stebbins@jetheaddev.com> wrote:
> On 02/28/2017 09:16 AM, John Stebbins wrote:
>> On 02/28/2017 08:40 AM, Luca Barbato wrote:
>>> On 28/02/2017 16:27, Vittorio Giovara wrote:
>>>> On Sun, Feb 26, 2017 at 12:58 PM, John Stebbins <stebbins@jetheaddev.com> wrote:
>>>>> This prevents invalid writes outside put_bits' buffer.
>>>>>
>>>>> It also has the side effect of allowing measurement of the required
>>>>> size of a buffer without the need to pre-allocate an over-sized buffer.
>>>>>
>>>>> This fixes a crash in aacenc.c where it could write past the end of the
>>>>> allocated packet, which is allocated to be the max size allowed by the
>>>>> aac spec.  aacenc.c uses the above feature to check the size
>>>>> of encoded data and try again when the size is too large.
>>>>> ---
>>>>>  libavcodec/put_bits.h | 14 ++++++++++----
>>>>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/libavcodec/put_bits.h b/libavcodec/put_bits.h
>>>>> index 17666fa..30b1dd2 100644
>>>>> --- a/libavcodec/put_bits.h
>>>>> +++ b/libavcodec/put_bits.h
>>>>> @@ -89,10 +89,14 @@ static inline void flush_put_bits(PutBitContext *s)
>>>>>      while (s->bit_left < 32) {
>>>>>          /* XXX: should test end of buffer */
>>>>>  #ifdef BITSTREAM_WRITER_LE
>>>>> -        *s->buf_ptr++ = s->bit_buf;
>>>>> +        if (s->buf_ptr < s->buf_end)
>>>>> +            *s->buf_ptr = s->bit_buf;
>>>>> +        s->buf_ptr++;
>>>>>          s->bit_buf  >>= 8;
>>>>>  #else
>>>>> -        *s->buf_ptr++ = s->bit_buf >> 24;
>>>>> +        if (s->buf_ptr < s->buf_end)
>>>>> +            *s->buf_ptr = s->bit_buf >> 24;
>>>>> +        s->buf_ptr++;
>>>>>          s->bit_buf  <<= 8;
>>>>>  #endif
>>>>>          s->bit_left  += 8;
>>>> shouldn't you move the buffer pointer only if it's within bounds?
>>>> namely, do s->buf_ptr++; only when s->buf_ptr < s->buf_end
>>>> same in the other chunk
>>>>
>>> We'd have to change the functions that report the nominal size written then.
>>>
>>>
>> Correct, the idea is that you can still call put_bits_count() to discover how much would have been written, even when
>> the buffer is too small.  So you can do things like put_bits_init((s, NULL, 0), then call execute some code that
>> "writes" using put_bits and measure what size buffer you need with put_bits_count.  aacenc.c does something like this.
>> It doesn't set a zero size buffer, but it sets a buffer that may be too small, and when it has written too much it
>> decreases lambda and tries again.
>>
>
> This conversation gives me a thought on how to improve this patch some.  Perhaps we should modify put_bits_count to
> return FFMIN(s->size_in_bits, (s->buf_ptr - s->buf) * 8 + 32 - s->bit_left));  And then create a new function that
> returns the number of bits that *would* have been written if the buffer was not too small.  This gives an extra layer of
> safety that we never had before.  Code that doesn't intend to overwrite the buffer, but does, would get the actual size
> of the buffer when calling put_bits_count and therefore wouldn't accidentally access outside the buffer (e.g.
> avio_write(io, buf, put_bits_count(s) / 8)).  Code that knows it may overflow the buffer (aacenc.c) would use the new
> function to check for overflow.
>
> Would this address your concern Vittorio?

I think so, and thanks for the explanation. I just hope that this
doesn't end up being more complicated than it should.
Luca Barbato Feb. 28, 2017, 5:15 p.m. | #6
On 28/02/2017 17:52, Vittorio Giovara wrote:
> I think so, and thanks for the explanation. I just hope that this
> doesn't end up being more complicated than it should.

I think it should be one more variable to account for the overflow.

So you can do something like this to get the expected size:

if (overflow)
    return size_in_bits + overflow;

And when you write do something along the lines of:

if (at the end of the buffer) {
    overflow += size;
    return early;
}

lu

Patch

diff --git a/libavcodec/put_bits.h b/libavcodec/put_bits.h
index 17666fa..30b1dd2 100644
--- a/libavcodec/put_bits.h
+++ b/libavcodec/put_bits.h
@@ -89,10 +89,14 @@  static inline void flush_put_bits(PutBitContext *s)
     while (s->bit_left < 32) {
         /* XXX: should test end of buffer */
 #ifdef BITSTREAM_WRITER_LE
-        *s->buf_ptr++ = s->bit_buf;
+        if (s->buf_ptr < s->buf_end)
+            *s->buf_ptr = s->bit_buf;
+        s->buf_ptr++;
         s->bit_buf  >>= 8;
 #else
-        *s->buf_ptr++ = s->bit_buf >> 24;
+        if (s->buf_ptr < s->buf_end)
+            *s->buf_ptr = s->bit_buf >> 24;
+        s->buf_ptr++;
         s->bit_buf  <<= 8;
 #endif
         s->bit_left  += 8;
@@ -145,7 +149,8 @@  static inline void put_bits(PutBitContext *s, int n, unsigned int value)
 #ifdef BITSTREAM_WRITER_LE
     bit_buf |= value << (32 - bit_left);
     if (n >= bit_left) {
-        AV_WL32(s->buf_ptr, bit_buf);
+        if (s->buf_ptr < s->buf_end)
+            AV_WL32(s->buf_ptr, bit_buf);
         s->buf_ptr += 4;
         bit_buf     = (bit_left == 32) ? 0 : value >> bit_left;
         bit_left   += 32;
@@ -158,7 +163,8 @@  static inline void put_bits(PutBitContext *s, int n, unsigned int value)
     } else {
         bit_buf   <<= bit_left;
         bit_buf    |= value >> (n - bit_left);
-        AV_WB32(s->buf_ptr, bit_buf);
+        if (s->buf_ptr < s->buf_end)
+            AV_WB32(s->buf_ptr, bit_buf);
         s->buf_ptr += 4;
         bit_left   += 32 - n;
         bit_buf     = value;