Re: [PATCH 3/3] meson: Enable -Wvla

[Thomas Huth]

On 21/02/2024 17.59, Thomas Huth wrote:
> On 21/02/2024 17.26, Thomas Huth wrote:
> > From: Peter Maydell <peter.maydell@linaro.org>
> >
> > QEMU has historically used variable length arrays only very rarely.
> > Variable length arrays are a potential security issue where an
> > on-stack dynamic allocation isn't correctly size-checked, especially
> > when the size comes from the guest.  (An example problem of this kind
> > from the past is CVE-2021-3527).  Forbidding them entirely is a
> > defensive measure against further bugs of this kind.
> >
> > Enable -Wvla to prevent any new uses from sneaking into the codebase.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > Message-ID: <20240125173211.1786196-3-peter.maydell@linaro.org>
> > [thuth: rebased to current master branch]
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >   meson.build | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/meson.build b/meson.build
> > index c1dc83e4c0..0ef1654e86 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -592,6 +592,7 @@ warn_flags = [
> >     '-Wstrict-prototypes',
> >     '-Wtype-limits',
> >     '-Wundef',
> > +  '-Wvla',
> >     '-Wwrite-strings',
> >     # Then disable some undesirable warnings
>
> Sigh, there's a new warning in the latest master branch:
>
>   https://gitlab.com/thuth/qemu/-/jobs/6225992174
>
> Caused by commit d65aba828 ("hw/sparc/leon3: implement multiprocessor")...
> Clément, Philippe, could this maybe be written in a different way that does 
> not trigger a -Wvla warning?

I think the DO_UPCAST is wrong here - if I got that right, DO_UPCAST is 
supposed to check that the second parameter is at the same location as the 
first type later points to. This is not the case here. I think we rather 
want container_of() here, so this patch is likely a simple fix:

diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -150,7 +150,7 @@ static void leon3_cpu_reset(void *opaque)
 {
     struct CPUResetData *info = (struct CPUResetData *) opaque;
     int id = info->id;
-    ResetData *s = (ResetData *)DO_UPCAST(ResetData, info[id], info);
+    ResetData *s = container_of(info, ResetData, info[id]);
     CPUState *cpu = CPU(s->info[id].cpu);
     CPUSPARCState *env = cpu_env(cpu);

I can send it as a proper patch, too.

 Thomas