[FFmpeg-devel] [PATCH] avcodec/apedec: fix prediction

[Paul B Mahol]

On Thu, Jun 30, 2022 at 10:31 AM Andreas Rheinhardt <
andreas.rheinhardt at outlook.com> wrote:

> Zhao Zhili:
> > From: Zhao Zhili <zhilizhao at tencent.com>
> >
> > Fixes ticket #9816.
> > Regression since ed0001482a74b60f3d5bc5.
> >
> > Prediction value larger than INT32_MAX should be treated as
> > negative. The code already depends on undefined right shift
> > behavior before the patch, which doesn't get fixed by the patch.
> >
> > Signed-off-by: Zhao Zhili <zhilizhao at tencent.com>
> > ---
> >  libavcodec/apedec.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c
> > index a7c38bce1b..5f2af2e147 100644
> > --- a/libavcodec/apedec.c
> > +++ b/libavcodec/apedec.c
> > @@ -1198,7 +1198,7 @@ static av_always_inline int
> predictor_update_filter(APEPredictor64 *p,
> >                    p->buf[delayB - 3] * p->coeffsB[filter][3] +
> >                    p->buf[delayB - 4] * p->coeffsB[filter][4];
> >
> > -    p->lastA[filter] = decoded + ((int64_t)((uint64_t)predictionA +
> (predictionB >> 1)) >> 10);
> > +    p->lastA[filter] = decoded + ((int32_t)((uint64_t)predictionA +
> (predictionB >> 1)) >> 10);
> >      p->filterA[filter] = p->lastA[filter] +
> ((int64_t)(p->filterA[filter] * 31ULL) >> 5);
> >
> >      sign = APESIGN(decoded);
>
> 1. Right shifts of negative numbers are implementation-defined, not
> undefined. And we rely on the implementation to do the sane thing for
> two's complement, so it is defined for us.
> 2. You are not necessarily treating them as negative, you simply discard
> the upper 32 bits and the result could be positive. And you discard the
> upper bits before you discard the lowest 10 bits, so the result has
> effectively only 22 bits. Is this really intended? If it is, you could
> perform the addition predictionA + (predictionB >> 1) in 32 bits.
> 3. I see one possibility for UB in this: The outermost addition on the
> right side is a signed addition, so it could overflow.
>

Also make sure that you do not break insane ape files, its on one of trac
issues.


>
> - Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>