Re: [libav-devel] [PATCH 1/4] put_bits: bounds check buffer
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
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH 1/4] put_bits: bounds check buffer
On Tue, Feb 28, 2017 at 11:35 AM, John Stebbins 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
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.
--
Vittorio
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH 1/4] put_bits: bounds check buffer
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
>>> 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?
--
John GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01 83F0 49F1 D7B2 60D4 D0F7
signature.asc
Description: OpenPGP digital signature
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH 1/4] put_bits: bounds check buffer
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
>> 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 GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01 83F0 49F1 D7B2 60D4 D0F7
signature.asc
Description: OpenPGP digital signature
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH 1/4] put_bits: bounds check buffer
On 28/02/2017 16:27, Vittorio Giovara wrote:
> On Sun, Feb 26, 2017 at 12:58 PM, John Stebbins
> 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
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH 1/4] put_bits: bounds check buffer
On Sun, Feb 26, 2017 at 12:58 PM, John Stebbins 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
--
Vittorio
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
