qemu: tpm: Initialize variable with NULL

[PATCH] qemu: tpm: Initialize variable with NULL

Posted by Stefan Berger 2 years, 5 months ago

Initialize an autofree'd variable with NULL that causes crashes
if a TPM 1.2 is used and the variable doesn't get a value assigned.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 src/qemu/qemu_tpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index b305313ad2..1992b596cb 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -610,7 +610,7 @@ qemuTPMEmulatorReconfigure(const char *storagepath,
 {
     g_autoptr(virCommand) cmd = NULL;
     int exitstatus;
-    g_autofree char *activePcrBanksStr;
+    g_autofree char *activePcrBanksStr = NULL;
     g_autofree char *swtpm_setup = virTPMGetSwtpmSetup();
     VIR_AUTOCLOSE pwdfile_fd = -1;
 
-- 
2.31.1

Re: [PATCH] qemu: tpm: Initialize variable with NULL

Posted by Marc-André Lureau 2 years, 5 months ago

On Mon, Nov 8, 2021 at 5:53 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
> Initialize an autofree'd variable with NULL that causes crashes
> if a TPM 1.2 is used and the variable doesn't get a value assigned.
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

certainly, I wished there would be a gcc error there..

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  src/qemu/qemu_tpm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
> index b305313ad2..1992b596cb 100644
> --- a/src/qemu/qemu_tpm.c
> +++ b/src/qemu/qemu_tpm.c
> @@ -610,7 +610,7 @@ qemuTPMEmulatorReconfigure(const char *storagepath,
>  {
>      g_autoptr(virCommand) cmd = NULL;
>      int exitstatus;
> -    g_autofree char *activePcrBanksStr;
> +    g_autofree char *activePcrBanksStr = NULL;
>      g_autofree char *swtpm_setup = virTPMGetSwtpmSetup();
>      VIR_AUTOCLOSE pwdfile_fd = -1;
>
> --
> 2.31.1
>

Re: [PATCH] qemu: tpm: Initialize variable with NULL

Posted by Daniel P. Berrangé 2 years, 5 months ago

On Mon, Nov 08, 2021 at 06:06:50PM +0400, Marc-André Lureau wrote:
> On Mon, Nov 8, 2021 at 5:53 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> >
> > Initialize an autofree'd variable with NULL that causes crashes
> > if a TPM 1.2 is used and the variable doesn't get a value assigned.
> >
> > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> 
> certainly, I wished there would be a gcc error there..

I'd support a coding rule for libvirt that *every* single stack variable
must *always* be initialized at time of declaration, even if there's an
initialization happening later in the method, even if it is on the very
next line.

We've had way too many bugs from leaving variables uninitialized over
the years, that mandatory explicit init is worthwhile on balance IMHO.

It isn't that easy to detect this in a coding style rule though, as
parsing C with regexes is flakey at best. I wonder if libclang could
be used to write a better style check.

Meanwhile, I'm looking forward to GCC 12 which introduces a flag

   $CC -ftrivial-auto-var-init=zero

to tell the compiler to initialize everything to zero. Annoyingly
that LLVM maintainers have had this in clang for a while, but don't
want to officially support this nice enhancement to the security
and reliability of C code, so hide this feature behind a crazy
flag [1]

> 
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> > ---
> >  src/qemu/qemu_tpm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
> > index b305313ad2..1992b596cb 100644
> > --- a/src/qemu/qemu_tpm.c
> > +++ b/src/qemu/qemu_tpm.c
> > @@ -610,7 +610,7 @@ qemuTPMEmulatorReconfigure(const char *storagepath,
> >  {
> >      g_autoptr(virCommand) cmd = NULL;
> >      int exitstatus;
> > -    g_autofree char *activePcrBanksStr;
> > +    g_autofree char *activePcrBanksStr = NULL;
> >      g_autofree char *swtpm_setup = virTPMGetSwtpmSetup();
> >      VIR_AUTOCLOSE pwdfile_fd = -1;
> >
> > --
> > 2.31.1
> >
> 

Regards,
Daniel

[1]  $  clang -ftrivial-auto-var-init=zero demo.c
     clang-12: error: -ftrivial-auto-var-init=zero hasn't been enabled. Enable it at your own peril for benchmarking purpose only with -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang

-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] qemu: tpm: Initialize variable with NULL

Posted by Ján Tomko 2 years, 5 months ago

On a Monday in 2021, Marc-André Lureau wrote:
>On Mon, Nov 8, 2021 at 5:53 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>> Initialize an autofree'd variable with NULL that causes crashes
>> if a TPM 1.2 is used and the variable doesn't get a value assigned.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>
>certainly, I wished there would be a gcc error there..
>

There was
https://gitlab.com/libvirt/libvirt/-/pipelines/402739013/failures

And this was already patched by Peter:
commit c43f22d5c1ea08c5516d791010ed3c230c47cb53
     qemuTPMEmulatorReconfigure: Fix two build issues

Jano

>Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
>> ---
>>  src/qemu/qemu_tpm.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)

Re: [PATCH] qemu: tpm: Initialize variable with NULL

Posted by Stefan Berger 2 years, 5 months ago

On 11/8/21 09:40, Ján Tomko wrote:
> On a Monday in 2021, Marc-André Lureau wrote:
>> On Mon, Nov 8, 2021 at 5:53 PM Stefan Berger <stefanb@linux.ibm.com> 
>> wrote:
>>>
>>> Initialize an autofree'd variable with NULL that causes crashes
>>> if a TPM 1.2 is used and the variable doesn't get a value assigned.
>>>
>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>
>> certainly, I wished there would be a gcc error there..
>>
>
> There was
> https://gitlab.com/libvirt/libvirt/-/pipelines/402739013/failures
>
> And this was already patched by Peter:
> commit c43f22d5c1ea08c5516d791010ed3c230c47cb53
>     qemuTPMEmulatorReconfigure: Fix two build issues

Sorry for the noise. F34 gcc hadn't complained.


    Stefan

Re: [PATCH] qemu: tpm: Initialize variable with NULL

Posted by Michal Prívozník 2 years, 5 months ago

On 11/8/21 3:50 PM, Stefan Berger wrote:
> 
> On 11/8/21 09:40, Ján Tomko wrote:
>> On a Monday in 2021, Marc-André Lureau wrote:
>>> On Mon, Nov 8, 2021 at 5:53 PM Stefan Berger <stefanb@linux.ibm.com>
>>> wrote:
>>>>
>>>> Initialize an autofree'd variable with NULL that causes crashes
>>>> if a TPM 1.2 is used and the variable doesn't get a value assigned.
>>>>
>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>>
>>> certainly, I wished there would be a gcc error there..
>>>
>>
>> There was
>> https://gitlab.com/libvirt/libvirt/-/pipelines/402739013/failures
>>
>> And this was already patched by Peter:
>> commit c43f22d5c1ea08c5516d791010ed3c230c47cb53
>>     qemuTPMEmulatorReconfigure: Fix two build issues
> 
> Sorry for the noise. F34 gcc hadn't complained.
> 

I've noticed that some of these warnings fire only when compiling with
-O2. Since I'm debugging a lot I'm in habit of setting -O0 -ggdb. And
that's when gcc did not emit the warning.

Michal