Re: [FFmpeg-devel] [PATCH 1/5] avcodec/lpc: copy levenson coeffs only when they have been computed

From: Michael Niedermayer <michael@niedermayer.cc>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH 1/5] avcodec/lpc: copy levenson coeffs only when they have been computed
Date: Mon, 13 May 2024 03:25:05 +0200
Message-ID: <20240513012505.GX6420@pb2> (raw)
In-Reply-To: <d7d0aa3d-f3c4-4da2-8f64-60871cd8104a@lynne.ee>

[-- Attachment #1.1: Type: text/plain, Size: 2541 bytes --]

On Sun, May 12, 2024 at 02:29:57AM +0200, Lynne via ffmpeg-devel wrote:
> On 12/05/2024 02:18, Michael Niedermayer wrote:
> > On Sun, May 12, 2024 at 02:13:06AM +0200, Lynne via ffmpeg-devel wrote:
> > > On 12/05/2024 02:03, Michael Niedermayer wrote:
> > > > Fixes: CID1473514 Uninitialized scalar variable
> > > > 
> > > > Sponsored-by: Sovereign Tech Fund
> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > ---
> > > >    libavcodec/lpc.c | 5 +++--
> > > >    1 file changed, 3 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/libavcodec/lpc.c b/libavcodec/lpc.c
> > > > index 8305cc0596a..981dacce8a5 100644
> > > > --- a/libavcodec/lpc.c
> > > > +++ b/libavcodec/lpc.c
> > > > @@ -282,8 +282,9 @@ int ff_lpc_calc_coefs(LPCContext *s,
> > > >            double av_uninit(weight);
> > > >            memset(var, 0, FFALIGN(MAX_LPC_ORDER+1,4)*sizeof(*var));
> > > > -        for(j=0; j<max_order; j++)
> > > > -            m[0].coeff[max_order-1][j] = -lpc[max_order-1][j];
> > > > +        if (lpc_passes > 1)
> > > > +            for(j=0; j<max_order; j++)
> > > > +                m[0].coeff[max_order-1][j] = -lpc[max_order-1][j];
> > > >            for(; pass<lpc_passes; pass++){
> > > >                avpriv_init_lls(&m[pass&1], max_order);
> > > 
> > > max_order is a function argument, I don't think that's the right place to
> > > fix this.
> > 
> > max_orders is fine
> > 
> > what the problem is, is that CHOLESKY with lpc_passes = 1
> > skips the first pass LEVINSON but this line copies the output
> > from LEVINSON so it copies Uninitialized data.
> > a few lines later thats cleared with avpriv_init_lls()
> > but that access to uninitialized data i think is undefined behavior
> > 
> > if my analysis is not wrong then i think my fix is correct
> > 
> > thx
> > 
> > [...]
> > 
> > 
> > _______________________________________________
> > 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".
> 
> Ah, I see. Could you put a small comment above, like:
> /* Avoids initializing with an unused value when max_order == 1 */?
> Other than that looks fine.

ok, will apply with such note

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

There will always be a question for which you do not know the correct answer.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".