On Fri, Jan 01, 2016 at 02:07:34PM +0100, Hendrik Leppkes wrote:
> On Fri, Jan 1, 2016 at 1:39 PM, Michael Niedermayer
> <mich...@niedermayer.cc> wrote:
> > On Fri, Jan 01, 2016 at 08:59:23AM +0000, Paul B Mahol wrote:
> >> On 1/1/16, Michael Niedermayer <michae...@gmx.at> wrote:
> >> > From: Michael Niedermayer <mich...@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 <mich...@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
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
