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

[Michael Niedermayer]

On Fri, Jan 01, 2016 at 02:07:34PM +0100, Hendrik Leppkes wrote:
> 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.

The assert0 here is used to turn a out of array WRITE into an ABORT
thus preventing the possibility of an arbitrary code execution exploit
it is not a legit error condition if this happens nor does it
achive its purpose if its disabled in user builds

That is what av_assert0() is for, something that is always checked
because the consequences of a bug and failure to check
are worse than an abort()

It would be possible to skip the write and continue, silently
generating a corrupted output, this seems strictly worse to me than
loudly aborting and pointing to a internal bug.
Bugs in the code should be pointed at with some noise so they are
seen, reported and corrected quickly.

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160101/095ca4fb/attachment.sig>