[FFmpeg-devel] [PATCH] avcodec/put_bits: Always check buffer end before writing

[Hendrik Leppkes]

On Fri, Jan 1, 2016 at 1:39 PM, Michael Niedermayer
<michael at niedermayer.cc> wrote:
> On Fri, Jan 01, 2016 at 08:59:23AM +0000, Paul B Mahol wrote:
>> On 1/1/16, Michael Niedermayer <michaelni at gmx.at> wrote:
>> > From: Michael Niedermayer <michael at niedermayer.cc>
>> >
>> > This causes a overall slowdown of 0.1 % (tested with mpeg4 single thread
>> > encoding of matrixbench at QP=3)
>> >
>> > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>> > ---
>> >  libavcodec/put_bits.h |   16 ++++++++++------
>> >  1 file changed, 10 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/libavcodec/put_bits.h b/libavcodec/put_bits.h
>> > index 5b1bc8b..69a3049 100644
>> > --- a/libavcodec/put_bits.h
>> > +++ b/libavcodec/put_bits.h
>> > @@ -163,9 +163,11 @@ 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_assert2(s->buf_ptr+3<s->buf_end);
>> > -        AV_WL32(s->buf_ptr, bit_buf);
>> > -        s->buf_ptr += 4;
>> > +        if (3 < s->buf_end - s->buf_ptr) {
>> > +            AV_WL32(s->buf_ptr, bit_buf);
>> > +            s->buf_ptr += 4;
>> > +        } else
>> > +            av_assert0(0);
>> >          bit_buf     = value >> bit_left;
>> >          bit_left   += 32;
>> >      }
>> > @@ -177,9 +179,11 @@ static inline void put_bits(PutBitContext *s, int n,
>> > unsigned int value)
>> >      } else {
>> >          bit_buf   <<= bit_left;
>> >          bit_buf    |= value >> (n - bit_left);
>> > -        av_assert2(s->buf_ptr+3<s->buf_end);
>> > -        AV_WB32(s->buf_ptr, bit_buf);
>> > -        s->buf_ptr += 4;
>> > +        if (3 < s->buf_end - s->buf_ptr) {
>> > +            AV_WB32(s->buf_ptr, bit_buf);
>> > +            s->buf_ptr += 4;
>> > +        } else
>> > +            av_assert0(0);
>> >          bit_left   += 32 - n;
>> >          bit_buf     = value;
>> >      }
>>
>> If this can happen, using assert0 is bad idea.
>
> Its a internal bug if it happens, it should not happen ...
>
>
>> If this should not happen add if under assert2.
>
> I tried doing the assert0 without the if() yestarday before sending
> the patch but it doubbled the speedloss
>
> Using assert2 instead of assert0 could be done but then there is
> no indication of this fault with default configuration.
>
> so i ended up with this a bit funny looking variant as it was the
> fastest in my testing
>

asserts should generally track things that cannot happen.
If this is a legit error case, it should be checked as such -  and if
it cannot happen, it might as well use assert2 so there is no slowdown
for normal execution.

Anyone working on this particular code is then advised to build with
assert level 2 to avoid introducing bugs that might trigger this.

In general a "release" build should really not contain any asserts, as
they should be present only to double-check internal assumptions and
avoid breakage, not something for users (CLI or API alike) to concern
themself with.

- Hendrik