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

[Michael Niedermayer]

On Thu, Dec 15, 2016 at 08:02:52AM -0500, Ronald S. Bultje wrote:
> Hi,
> 
> 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.
> 
> - does this belong in 4xm.c? (Or in generic code? Or in the app?)
> - should it return an error? (Or just clip parameters? Or ignore the
> invalid value?)
> 
> > This isn't specifically intended at this patch, but rather at the sort of
> > > rabbit hole this change might lead to,
> >
> > I have a pretty good map of this rabbit hole (i.e. lots of samples
> > triggering
> > UBSan errors) and one day I might try to dig it up, but for now I'm
> > limiting
> > myself to the codec parameters.
> 
> 
> I'm not saying mplayer was great, but one of the principles I believe we
> always copied from them was to try to play files to the best of our
> abilities and not error out at the first convenience. This isn't just a
> theoretical thing, a lot of people credited mplayer with playing utterly
> broken AVI files that probably even ffmpeg rejects. What ffmpeg added on
> top of that is to make a project maintainable by not being an utter
> crapshoot.
> 
> This isn't 4xm.c-specific, this is a general philosophical question:
> - should we error out?
> - should this be in generic code if we're likely to repeat such checks all
> over the place?
> 
> > which would cause the code to be uber-full of such checks, none of which
> > > really have any significance. But maybe others disagree...
> >
> > Not relying on undefined behavior is a significant improvement. And doing
> > these checks consequently where the values are set makes it possible
> > for other code to rely on their validity without further checks.
> 
> 
> Unless "UB" leads to actual bad behaviour, I personally don't necessarily
> care. I'm very scared that when you go beyond codec parameters, and you
> want to check for overflows all over the place, we'll never see the end of
> it...
> 

> I'd appreciate if others could chime in here, I don't want to carry this
> argument all by myself if nobody cares.

as you are asking for others oppinion
valid C code must not trigger undefined behavior

undefined behavior means anything can happen and this is not guranteed
to be limited to the value a operation produces.
if you write a*b the compiler can assume that a and b are small
enough for this not to overflow, it can use this to simplify checks
before and after the operation.
A silly example would be a*b/b != a  (with signed a and b) which any
good compiler should replace by 0

We have seen bugs from undefined behavior like strict aliassing
violations were the compiler assumed that things wouldnt be accessed
as they had a different type ...

I think we should fix all instances of UB.
If we find cases that we cannot fix without slowdown or without
making the code hard to maintain by the people who activly work on it
then iam not against making exceptions. But in my oppinion in
general UB should be fixed

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20161215/0c92ad1a/attachment.sig>