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

Paul B Mahol onemda at gmail.com
Mon Oct 28 20:04:59 CET 2013


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.

>
> Ronald
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>


More information about the ffmpeg-devel mailing list