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 <[email protected]> >>> 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 [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
