[FFmpeg-devel] [PATCH V3] lavc/golomb: Fix UE golomb overwrite issue.

[Michael Niedermayer]

On Thu, Jun 08, 2017 at 11:02:30AM +0100, Mark Thompson wrote:
> On 08/06/17 01:29, Jun Zhao wrote:
> > V3: Clean the code logic base on Michael's review.
> > V2: Add Add set_ue_golomb_long() to support 32bits UE golomb and update the unit test.
> > 
> > From 4de3e0c30af7bc1901562000eda018a6d6849292 Mon Sep 17 00:00:00 2001
> > From: Jun Zhao <jun.zhao at intel.com>
> > Date: Fri, 2 Jun 2017 15:05:49 +0800
> > Subject: [PATCH V3] lavc/golomb: Fix UE golomb overwrite issue.
> > 
> > put_bits just support write up to 31 bits, when write 32 bit in
> > put_bits, it's will overwrite the bit buffer, because the default
> > assert level is 0, the av_assert2(n <= 31 && value < (1U << n))
> > in put_bits can not be trigger runtime. Add set_ue_golomb_long()
> > to support 32bits UE golomb.
> > 
> > Signed-off-by: Jun Zhao <jun.zhao at intel.com>
[...]
> > +
> > +    if (i < 256)
> > +        put_bits(pb, ff_ue_golomb_len[i], i + 1);
> > +    else {
> > +        int e = av_log2(i + 1);
> > +        put_bits64(pb, 2 * e + 1, i + 1);
> > +    }
> > +}
> 
> Please don't add a new function, just extend the existing one.  set_ue_golomb() is not used in any place where minor ricing is of any value, and having another function will only be confusing over which to use (or, more likely, people will always use the long version in order to avoid thinking about it just like they do with get_ue_golomb() - cf. "grep get_ue_golomb libavcodec/hevc*").

No question its easier to do something random than to think about
the range allowed by the specification.
But when writing data, its neccessary to comply to the specification,
otherwise one creates invalid streams. So theres no way around thinking
about the range even with just one function.



> 
> > +
> > +/**
> >   * write truncated unsigned exp golomb code.
> >   */
> >  static inline void set_te_golomb(PutBitContext *pb, int i, int range)
> > diff --git a/libavcodec/put_bits.h b/libavcodec/put_bits.h
> > index 68ed391195..06f0ebbeba 100644
> > --- a/libavcodec/put_bits.h
> > +++ b/libavcodec/put_bits.h
> > @@ -221,6 +221,41 @@ static void av_unused put_bits32(PutBitContext *s, uint32_t value)
> >  }
> >  
> >  /**
> > + * Write up to 64 bits into a bitstream.
> > + */
> > +static inline void put_bits64(PutBitContext *s, int n, uint64_t value)
> 
> put_bits32() writes exactly 32 bits, so it sounds like put_bits64() will write exactly 64 bits.  I'm not sure what the function should be called, but not that.

If you name it different then please also rename get_bits64() in a
seperate patch before
[get_bits64 was originally called get_bits_longlong until it was
 renamed to get_bits64 in result of a merge i did. Looking back that
 renaming was a bad idea. longlong isnt a great name either though]


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

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- 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/20170608/53be7aa8/attachment.sig>