[FFmpeg-devel] [PATCH] avcodec/wavpack: check for overflow

[Michael Niedermayer]

On Wed, Jul 03, 2013 at 10:57:17AM +0000, Paul B Mahol wrote:
> On 7/3/13, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Wed, Jul 03, 2013 at 12:13:45AM +0000, Paul B Mahol wrote:
> >> On 7/3/13, Michael Niedermayer <michaelni at gmx.at> wrote:
> >> > On Tue, Jul 02, 2013 at 11:05:09PM +0000, Paul B Mahol wrote:
> >> >> On 7/2/13, Michael Niedermayer <michaelni at gmx.at> wrote:
> >> >> > On Mon, Jun 17, 2013 at 05:49:47PM +0200, Michael Niedermayer wrote:
> >> >> >> On Mon, Jun 17, 2013 at 10:54:28AM +0000, Paul B Mahol wrote:
> >> >> >> > On 6/15/13, Michael Niedermayer <michaelni at gmx.at> wrote:
> >> >> >> > > Fix integer overflow in fate
> >> >> >> > >
> >> >> >> > > Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> >> >> >> > > ---
> >> >> >> > >  libavcodec/wavpack.c |   10 ++++++++--
> >> >> >> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> >> >> >> > >
> >> >> >> > > diff --git a/libavcodec/wavpack.c b/libavcodec/wavpack.c
> >> >> >> > > index 47f598a..dd273f7 100644
> >> >> >> > > --- a/libavcodec/wavpack.c
> >> >> >> > > +++ b/libavcodec/wavpack.c
> >> >> >> > > @@ -581,8 +581,14 @@ static inline int
> >> >> >> > > wv_unpack_stereo(WavpackFrameContext
> >> >> >> > > *s, GetBitContext *gb,
> >> >> >> > >                      L2 = L + ((s->decorr[i].weightA *
> >> >> >> > > (int64_t)A
> >> >> >> > > +
> >> >> >> > > 512) >>
> >> >> >> > > 10);
> >> >> >> > >                      R2 = R + ((s->decorr[i].weightB *
> >> >> >> > > (int64_t)B
> >> >> >> > > +
> >> >> >> > > 512) >>
> >> >> >> > > 10);
> >> >> >> > >                  } else {
> >> >> >> > > -                    L2 = L + ((s->decorr[i].weightA * A + 512)
> >> >> >> > > >>
> >> >> >> > > 10);
> >> >> >> > > -                    R2 = R + ((s->decorr[i].weightB * B + 512)
> >> >> >> > > >>
> >> >> >> > > 10);
> >> >> >> > > +                    int64_t Lt = s->decorr[i].weightA *
> >> >> >> > > (int64_t)A
> >> >> >> > > +
> >> >> >> > > 512;
> >> >> >> > > +                    int64_t Rt = s->decorr[i].weightB *
> >> >> >> > > (int64_t)B
> >> >> >> > > +
> >> >> >> > > 512;
> >> >> >> > > +                    if ((int32_t)Lt != Lt || (int32_t)Rt != Rt)
> >> >> >> > > {
> >> >> >> > > +                        av_log(s->avctx, AV_LOG_ERROR, "sample
> >> >> >> > > overflow %d
> >> >> >> >
> >> >> >> > This looks extremly ugly.
> >> >> >>
> >> >> >> Iam quite aware of that, which is part of the reason why i posted
> >> >> >> this
> >> >> >> (aka do you know a less ugly solution?)
> >> >> >
> >> >> > ping
> >> >> > anyone has a better solution ?
> >> >> > do any objections to the original patch remain ?
> >> >>
> >> >> Yes, i see no point to apply this patch as is.
> >> >
> >> > what alternative solution do you suggest?
> >>
> >> There is no alternative solution yet, as you did not provided any real
> >> solution to this problem.
> >
> > If i had a real solution i wouldnt ask for one.
> > Repeating my question with different wording isnt going to solve this
> > i suspect but then again yeah trying cant hurt. sadly no i still
> > have only that ugly solution an alternativ or real or whatever you
> > call it solution is still highly welcome
> >
> >
> >>
> >> Why only this overflow in decoder need checking?
> >
> >
> >>
> >> Why check is needed at all?
> >
> > It crashes with the right compiler flags, Its undefined behavior
> 
> Yes, its undefined behavior when decoding invalid files. Thing is, its
> clear I'm against applying this kind of band aid.

I understand you quite well, the change is far from something i like.
still i think this issue needs to be fixed, a lib crashing on an
invalid file is a bad thing, it normally kills the application using
that file, and that might be a web browser with other windows open
or even just ffmpeg itself that crashes at 95% transcoding.
Now we are lucky that it doesnt crash with default compile flags but
still ...

also if integer overflows arent fixed, serious ones that have
security implications are hard to spot amongth all the not so serious
ones.

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130703/c7097979/attachment.asc>