> On Dec 15, 2016, at 19:21, Andreas Cadhalpun > <andreas.cadhal...@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.cadhal...@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@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel