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

[Ronald S. Bultje]

Hi,

On Thu, Dec 15, 2016 at 9:28 AM, Michael Niedermayer <michael at niedermayer.cc
> wrote:

> 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


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

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. I personally like suggestion (1), unsigned is a pretty good
type for things that shouldn't have negative values. WDYT?

Ronald