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