[FFmpeg-devel] [PATCH] avcodec/get_bits: consider bit_size of 0 an error

[Ronald S. Bultje]

Hi,

On Mon, Oct 28, 2013 at 3:04 PM, Paul B Mahol <onemda at gmail.com> wrote:

> On 10/28/13, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> > Hi,
> >
> > On Mon, Oct 28, 2013 at 2:34 PM, Paul B Mahol <onemda at gmail.com> wrote:
> >
> >> On 10/28/13, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> >> > Hi,
> >> >
> >> > On Mon, Oct 28, 2013 at 2:17 PM, Paul B Mahol <onemda at gmail.com>
> wrote:
> >> >
> >> >> On 10/28/13, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> >> >> > Hi,
> >> >> >
> >> >> > On Mon, Oct 28, 2013 at 1:35 PM, Paul B Mahol <onemda at gmail.com>
> >> wrote:
> >> >> >
> >> >> >> On 10/28/13, Reimar Doeffinger <Reimar.Doeffinger at gmx.de> wrote:
> >> >> >> > On Mon, Oct 28, 2013 at 05:04:38PM +0000, Paul B Mahol wrote:
> >> >> >> >> On 10/28/13, Reimar Doeffinger <Reimar.Doeffinger at gmx.de>
> wrote:
> >> >> >> >> > On Mon, Oct 28, 2013 at 04:52:27PM +0000, Paul B Mahol wrote:
> >> >> >> >> >> Signed-off-by: Paul B Mahol <onemda at gmail.com>
> >> >> >> >> >
> >> >> >> >> > Could you add the reason for this change to the commit
> >> >> >> >> > message?
> >> >> >> >> > In theory being able to use 0 might work as an optimization
> in
> >> >> >> >> > some cases, so I don't think supporting it is as nonsense as
> >> >> >> >> > it
> >> >> >> >> > might seem.
> >> >> >> >>
> >> >> >> >> What kind of optimization?
> >> >> >> >
> >> >> >> > Purely theoretical: You have a value A, depending on that
> >> >> >> > following
> >> >> >> > value B is either not encoded, encoded with 2, 5 or 8 bits.
> >> >> >> > If get_bits supports 0 bits then you can just do something like
> >> >> >> > get_bits(... (int){0, 2, 5, 8}[A]);
> >> >> >> > if 0 is not allowed, you have to explicitly skip the get_bits
> for
> >> >> >> > A == 0.
> >> >> >> >
> >> >> >> >> If return value is never checked and bit_size is negative
> number
> >> >> >> >> next
> >> >> >> >> get_bits will happily crash.
> >> >> >> >
> >> >> >> > But negative number is already checked, your patch is about
> >> checking
> >> >> >> > 0?!
> >> >> >>
> >> >> >> Yes, because here is what happens for 0 case:
> >> >> >>
> >> >> >> buffer is not set to NULL (for negative case this causes crash)
> >> >> >>
> >> >> >> get_bits may return 1 even if bit count is 0, and it will happily
> >> >> >> return it forever (until  whatever buffer points to is changed to
> >> >> >> 0)
> >> >> >>
> >> >> >> so something like
> >> >> >>
> >> >> >> while(get_bits1(gb)) {
> >> >> >> }
> >> >> >>
> >> >> >> loops forever.
> >> >> >>
> >> >> >
> >> >> > That is a bug in the caller.
> >> >>
> >> >> Are you saying I should use get_bits_left ?
> >> >
> >> >
> >> > In this particular case, no; If you buffer is padded with
> >> > FF_INPUT_BUFFER_PADDING_SIZE zero bytes, at EOF, get_bits should
> always
> >> > return 0.
> >>
> >> For sample from bug #3089 it does not return always 0, as bit size is
> set
> >> to 0,
> >> thus checked bit reader never reach padded bytes.
> >
> >
> > That sounds fixable.
>
> Certainly, this and another patch fixes it.


I think the patch is fine. In terms of caller, I'm still not sure it's a
good idea (what if for some obscure reason we decide to return 1 instead of
0 on EOF?), but I guess that's sort of academic so maybe not worth worrying
about.

Ronald