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

[Rodger Combs]

> On Dec 15, 2016, at 19:21, Andreas Cadhalpun <andreas.cadhalpun at googlemail.com> wrote:
> 
> 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.
> 

Fair enough.

>> - 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.
> 

My proposal was for something like:
#define BAIL_ON_OVERFLOW(x) if (x) {av_log(avctx, AV_LOG_ERROR, "Overflow check failed: " #x); return AVERROR_INVALIDDATA;}
Which basically reduces the code overhead down to a simple one-liner. It's hard to get detailed error prints out of this, but if we're saying these cases are so unlikely (fuzzer-only?) that we're comfortable outright failing on them, the level of precision in the message probably doesn't matter much?

>> - 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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel