On Tue, Feb 28, 2017 at 11:35 AM, John Stebbins <stebb...@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 <stebb...@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.
--
Vittorio
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
