From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id DCB9143828 for ; Thu, 30 Jun 2022 08:46:28 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id D6CB868B685; Thu, 30 Jun 2022 11:46:27 +0300 (EEST) Received: from mail-yw1-f182.google.com (mail-yw1-f182.google.com [209.85.128.182]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id C3D0068B4B8 for ; Thu, 30 Jun 2022 11:46:21 +0300 (EEST) Received: by mail-yw1-f182.google.com with SMTP id 00721157ae682-31bf3656517so81328507b3.12 for ; Thu, 30 Jun 2022 01:46:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=t/w9ZjX1d0Wpxp2/p++3JOy8OgetwlpPO8RpHtgun3M=; b=m+cUOCRiUtedADxki4xrQUTVeGToJ3QlqU0P8WKa5sFu2YhQ6G13++/5hSmuWwin4H MTUrTYZHnuFhR5oNFkHVGpmwL9eXV0C3nkQ6LOhc/PTw7MFrimnnTCqm3WrXFMjvrq3r nz3PqkKIR+Mp0t3iumdwcAOyA33uQ1nRQ/OqlHVWPPtFmuPQ/+cmjCVvWdNdNFj9Mbp0 8bMEMlfojCwqgPcHK7KJHFu/9KOntnFW0lis18R8xvReFqLuqzRmAA6t7g5HfzeoKRPO ydQ+lWEq8szc6J+HsNq3VhIwQtz3TBfBtSYSy6ZHpN9ZkhVThGXGuWWieY6hSjuej0kV TRFw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=t/w9ZjX1d0Wpxp2/p++3JOy8OgetwlpPO8RpHtgun3M=; b=CHwno0Unb7sLsJ9fkSetzczoI0easOtMqRVDz3ZLsV6hzfa5gzZVNzEwXOF1mtRyxK HzRA2e6cze9PhzIuH+yIKUYLejiJUOGdSAtB1u1qLsDPP2UQC52yiWi98/KOxKnoR6uY Xh0j2xt2f9a6w9H9HMhdkOoAhE3X+rnA9BYZCCf8PBXXN41QeQUJCdtvyyzvoEDp/3Ce Ty2416beklyglwzQS0x03dd1u0edBUskhDsgJWXXAdYCzdWLKUJAfuNHdeL2CWV+XUYJ FLlMwmxZe2/aQpK5qCTAUMKVz0ZejMQ0USiwe0w7FJfNw08VuEpYaTMUtPLSRbi0LVXj LqMg== X-Gm-Message-State: AJIora84CJOUz9r3F4CRXt4Ag5ugrKrg2n/EmbEfW8PgxzXjkeTSQPiM 6cD9BzvMGB8Npzey1W235KVztS/J/hUeulox5JKuJSequsI= X-Google-Smtp-Source: AGRyM1tpcbzKK1jblSVl+XtKz3ob8N1ElyOe7uWhjZT7Z48TNeAoK8/mak+qsNDzNbZdLk++2tb4CW00+GxSYWbdvr4= X-Received: by 2002:a0d:e841:0:b0:317:9fea:a18b with SMTP id r62-20020a0de841000000b003179feaa18bmr9214672ywe.302.1656578780526; Thu, 30 Jun 2022 01:46:20 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Paul B Mahol Date: Thu, 30 Jun 2022 10:49:06 +0200 Message-ID: To: FFmpeg development discussions and patches X-Content-Filtered-By: Mailman/MimeDel 2.1.29 Subject: Re: [FFmpeg-devel] [PATCH] avcodec/apedec: fix prediction X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: On Thu, Jun 30, 2022 at 10:31 AM Andreas Rheinhardt < andreas.rheinhardt@outlook.com> wrote: > Zhao Zhili: > > From: Zhao Zhili > > > > 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 > > --- > > 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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".