[FFmpeg-devel] [PATCH 1/3] 4xm: prevent overflow during bit rate calculation

Andreas Cadhalpun andreas.cadhalpun at googlemail.com
Fri Dec 16 03:21:17 EET 2016


On 15.12.2016 14:02, Ronald S. Bultje wrote:
> On Wed, Dec 14, 2016 at 7:11 PM, Andreas Cadhalpun <
> andreas.cadhalpun at googlemail.com> wrote:
>> On 14.12.2016 02:46, Ronald S. Bultje wrote:
>>> Not wanting to discourage you, but I wonder if there's really a point to
>>> this...?
>>
>> These patches are prerequisites for enforcing the validity of codec
>> parameters [1].
>>
>>> I don't see how the user experience changes.
>>
>> Users won't see ffmpeg claiming nonsense bit rates like -1184293205235990
>> kb/s
>> anymore.
> 
> 
> I don't think you understand my question.

Maybe you just didn't understand my answer.

> - does this belong in 4xm.c? (Or in generic code? Or in the app?)

I think it belongs both in 4xm.c and generic code, as I have explained
in detail [1] in the discussion back then.

> - should it return an error? (Or just clip parameters? Or ignore the
> invalid value?)

This was also already discussed [2].

On 15.12.2016 21:57, Ronald S. Bultje wrote:
> So, I asked on IRC, here's 3 suggestions from Roger Combs:
> 
> - in case we want to be pedantic (I personally don't care, but I understand
> other people are), it might make sense to just make these members
> (channels, block_align, bitrate) unsigned. That removes the UB of the
> overflow, and it fixes the negative number reporting in client apps for
> bitrate, but you can still have positive crazy numbers and it doesn't
> return an error.

These are public fields, so changing them is not easy and more importantly
changing them from signed to unsigned is a very bad idea, as it will most
probably cause all kinds of subtle bugs for API users, e.g. when comparing,
subtracting, etc. and not expecting unsigned behavior.

> - if for whatever reason some things cannot be done in generic code or by
> changing the type (this should really cover most cases), and we want
> specific overflow checks, then maybe we want to have some generic helper
> macros that make them one-liners in decoders. This would return an error
> along with fixing the UB.

I don't think the number of overflow checks added justifies the additional
complexity of factoring things out. These checks are also subtly different,
so it's not easy to write a generic helper for that.
However, I plan to do this for the actually common cases when validating
codec parameters, like checking that a parameter is not negative.

> - have overflow-safe multiplication functions instead of checking each
> argument before doing a multiply, like __builtin_mul_overflow, and then
> return INT64_MAX if it would overflow inside that function. This fixes
> display of numbers in client applications and the UB, but without returning
> an error.

I think that would be over-engineered.

> What I want most - and this comment applies to all patches of this sort -
> is to have something that we can all agree is OK for pretty much any
> decoder out there without significant overhead in code (source - not
> binary) size. This way, you have a template to work from and fix specific
> instances without us having to argue over every single time you do a next
> run with ubsan.

UBSan is not only about overflows, undefined shifts are also quite common.
But I haven't really looked into these cases yet, so I don't know what kind
of template, if any, would make sense for them.

Best regards,
Andreas


1: https://ffmpeg.org/pipermail/ffmpeg-devel/2016-October/201742.html
2: https://ffmpeg.org/pipermail/ffmpeg-devel/2016-November/203257.html


More information about the ffmpeg-devel mailing list