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

Ronald S. Bultje rsbultje at gmail.com
Thu Dec 15 15:02:52 EET 2016


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.

Ronald


More information about the ffmpeg-devel mailing list