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

Ronald S. Bultje rsbultje at gmail.com
Wed Dec 14 03:46:49 EET 2016


Hi,

On Tue, Dec 13, 2016 at 8:21 PM, Andreas Cadhalpun <
andreas.cadhalpun at googlemail.com> wrote:

> On 14.12.2016 02:01, Ronald S. Bultje wrote:
> > Hi,
> >
> > On Tue, Dec 13, 2016 at 7:57 PM, Andreas Cadhalpun <
> > andreas.cadhalpun at googlemail.com> wrote:
> >
> >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> >> ---
> >>  libavformat/4xm.c | 8 +++++++-
> >>  1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/libavformat/4xm.c b/libavformat/4xm.c
> >> index 8a50778..2758b69 100644
> >> --- a/libavformat/4xm.c
> >> +++ b/libavformat/4xm.c
> >> @@ -163,6 +163,12 @@ static int parse_strk(AVFormatContext *s,
> >>          return AVERROR_INVALIDDATA;
> >>      }
> >>
> >> +    if (fourxm->tracks[track].sample_rate > INT64_MAX /
> >> fourxm->tracks[track].bits / fourxm->tracks[track].channels) {
> >> +        av_log(s, AV_LOG_ERROR, "Overflow during bit rate calculation
> %d
> >> * %d * %d\n",
> >> +               fourxm->tracks[track].sample_rate,
> >> fourxm->tracks[track].bits, fourxm->tracks[track].channels);
> >> +        return AVERROR_INVALIDDATA;
> >> +    }
> >
> >
> > What is the functional effect of the overflow?
>
> It is undefined behavior.
>
> > Does it crash? Or is there some other security issue?
>
> The most likely behavior is that bit_rate will contain a negative value,
> which might cause problems later on, but I'm not aware of specific
> security issues caused by this.


Not wanting to discourage you, but I wonder if there's really a point to
this...? I don't see how the user experience changes.

This isn't specifically intended at this patch, but rather at the sort of
rabbit hole this change might lead to, which would cause the code to be
uber-full of such checks, none of which really have any significance. But
maybe others disagree...

Ronald


More information about the ffmpeg-devel mailing list