[PATCH 0/2] Miscellaneous MSan finding

[PATCH 0/2] Miscellaneous MSan finding

[Nabih Estefan]

Fixes for miscellaneous msan finding in QEMU

Peter Foley (2):
  util: fix msan findings in keyval
  accel/tcg: fix msan findings in translate-all

 accel/tcg/translate-all.c | 2 +-
 util/keyval.c             | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.49.0.1015.ga840276032-goog

[PATCH 1/2] util: fix msan findings in keyval

[Nabih Estefan]

From: Peter Foley <pefoley@google.com>

e.g.
I	2025-02-28 09:51:05.240071-0800		624	stream.go:47	qemu: Uninitialized value was created by an allocation of 'key_in_cur.i' in the stack frame
I	2025-02-28 09:51:05.240187-0800		624	stream.go:47	qemu: #0 0xaaaac49f489c in keyval_parse_one third_party/qemu/util/keyval.c:190:5

Signed-off-by: Peter Foley <pefoley@google.com>
Signed-off-by: Nabih Estefan <nabihestefan@google.com>
---
 util/keyval.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/keyval.c b/util/keyval.c
index a70629a481..f33c64079d 100644
--- a/util/keyval.c
+++ b/util/keyval.c
@@ -187,7 +187,7 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
 {
     const char *key, *key_end, *val_end, *s, *end;
     size_t len;
-    char key_in_cur[128];
+    char key_in_cur[128] = {};
     QDict *cur;
     int ret;
     QObject *next;
-- 
2.49.0.1015.ga840276032-goog

[PATCH 2/2] accel/tcg: fix msan findings in translate-all

[Nabih Estefan]

From: Peter Foley <pefoley@google.com>

e.g.
  Uninitialized value was created by an allocation of 'host_pc' in the stack frame
  #0 0xaaaac07df87c in tb_gen_code third_party/qemu/accel/tcg/translate-all.c:297:5

Signed-off-by: Peter Foley <pefoley@google.com>
Signed-off-by: Nabih Estefan <nabihestefan@google.com>
---
 accel/tcg/translate-all.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 451b383aa8..03af677281 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -267,7 +267,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu, TCGTBCPUState s)
     tcg_insn_unit *gen_code_buf;
     int gen_code_size, search_size, max_insns;
     int64_t ti;
-    void *host_pc;
+    void *host_pc = NULL;
 
     assert_memory_lock();
     qemu_thread_jit_write();
-- 
2.49.0.1015.ga840276032-goog

Re: [PATCH 1/2] util: fix msan findings in keyval

[Markus Armbruster]

Nabih Estefan <nabihestefan@google.com> writes:

> From: Peter Foley <pefoley@google.com>
>
> e.g.
> I	2025-02-28 09:51:05.240071-0800		624	stream.go:47	qemu: Uninitialized value was created by an allocation of 'key_in_cur.i' in the stack frame
> I	2025-02-28 09:51:05.240187-0800		624	stream.go:47	qemu: #0 0xaaaac49f489c in keyval_parse_one third_party/qemu/util/keyval.c:190:5
>
> Signed-off-by: Peter Foley <pefoley@google.com>
> Signed-off-by: Nabih Estefan <nabihestefan@google.com>
> ---
>  util/keyval.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/util/keyval.c b/util/keyval.c
> index a70629a481..f33c64079d 100644
> --- a/util/keyval.c
> +++ b/util/keyval.c
> @@ -187,7 +187,7 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
>  {
>      const char *key, *key_end, *val_end, *s, *end;
>      size_t len;
> -    char key_in_cur[128];
> +    char key_in_cur[128] = {};
>      QDict *cur;
>      int ret;
>      QObject *next;

Prior review of Peter's patch concluded this must be false positive:
https://lore.kernel.org/qemu-devel/14168384-ecdb-4c05-8267-ac5ef1c46fe9@redhat.com/

Re: [PATCH 1/2] util: fix msan findings in keyval

[Daniel P. Berrangé]

On Fri, May 09, 2025 at 07:48:57AM +0200, Markus Armbruster wrote:
> Nabih Estefan <nabihestefan@google.com> writes:
> 
> > From: Peter Foley <pefoley@google.com>
> >
> > e.g.
> > I	2025-02-28 09:51:05.240071-0800		624	stream.go:47	qemu: Uninitialized value was created by an allocation of 'key_in_cur.i' in the stack frame
> > I	2025-02-28 09:51:05.240187-0800		624	stream.go:47	qemu: #0 0xaaaac49f489c in keyval_parse_one third_party/qemu/util/keyval.c:190:5
> >
> > Signed-off-by: Peter Foley <pefoley@google.com>
> > Signed-off-by: Nabih Estefan <nabihestefan@google.com>
> > ---
> >  util/keyval.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/util/keyval.c b/util/keyval.c
> > index a70629a481..f33c64079d 100644
> > --- a/util/keyval.c
> > +++ b/util/keyval.c
> > @@ -187,7 +187,7 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
> >  {
> >      const char *key, *key_end, *val_end, *s, *end;
> >      size_t len;
> > -    char key_in_cur[128];
> > +    char key_in_cur[128] = {};
> >      QDict *cur;
> >      int ret;
> >      QObject *next;
> 
> Prior review of Peter's patch concluded this must be false positive:
> https://lore.kernel.org/qemu-devel/14168384-ecdb-4c05-8267-ac5ef1c46fe9@redhat.com/

While I agree with Paolo's reasoning, I think it is still worth adding an
explicit initializer, because it makes it easier for both humans and machines
to reason about correctless.

To reinforce that we don't have an actual bug though, also note that qemu
unconditionally builds with -ftrivial-auto-var-init=zero. So if we happen
to forget any, it won't cause a bug in the common case of a zero-initializer.

With regards,
Daniel
-- 
|: 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 1/2] util: fix msan findings in keyval

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, May 09, 2025 at 07:48:57AM +0200, Markus Armbruster wrote:
>> Nabih Estefan <nabihestefan@google.com> writes:
>> 
>> > From: Peter Foley <pefoley@google.com>
>> >
>> > e.g.
>> > I	2025-02-28 09:51:05.240071-0800		624	stream.go:47	qemu: Uninitialized value was created by an allocation of 'key_in_cur.i' in the stack frame
>> > I	2025-02-28 09:51:05.240187-0800		624	stream.go:47	qemu: #0 0xaaaac49f489c in keyval_parse_one third_party/qemu/util/keyval.c:190:5
>> >
>> > Signed-off-by: Peter Foley <pefoley@google.com>
>> > Signed-off-by: Nabih Estefan <nabihestefan@google.com>
>> > ---
>> >  util/keyval.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/util/keyval.c b/util/keyval.c
>> > index a70629a481..f33c64079d 100644
>> > --- a/util/keyval.c
>> > +++ b/util/keyval.c
>> > @@ -187,7 +187,7 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
>> >  {
>> >      const char *key, *key_end, *val_end, *s, *end;
>> >      size_t len;
>> > -    char key_in_cur[128];
>> > +    char key_in_cur[128] = {};
>> >      QDict *cur;
>> >      int ret;
>> >      QObject *next;
>> 
>> Prior review of Peter's patch concluded this must be false positive:
>> https://lore.kernel.org/qemu-devel/14168384-ecdb-4c05-8267-ac5ef1c46fe9@redhat.com/
>
> While I agree with Paolo's reasoning, I think it is still worth adding an
> explicit initializer, because it makes it easier for both humans and machines
> to reason about correctless.

I think the code is just fine as is.  But if we decide we want an
initializer to make things easier for casual readers / imperfect tools,
then let's use "", not {}, as I suggested in my review.

> To reinforce that we don't have an actual bug though, also note that qemu
> unconditionally builds with -ftrivial-auto-var-init=zero. So if we happen
> to forget any, it won't cause a bug in the common case of a zero-initializer.
>
> With regards,
> Daniel

Re: [PATCH 1/2] util: fix msan findings in keyval

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 2469 bytes --]

Il ven 9 mag 2025, 09:58 Daniel P. Berrangé <berrange@redhat.com> ha
scritto:

> On Fri, May 09, 2025 at 07:48:57AM +0200, Markus Armbruster wrote:
> > Nabih Estefan <nabihestefan@google.com> writes:
> >
> > > From: Peter Foley <pefoley@google.com>
> > >
> > > e.g.
> > > I   2025-02-28 09:51:05.240071-0800         624     stream.go:47
> qemu: Uninitialized value was created by an allocation of 'key_in_cur.i' in
> the stack frame
> > > I   2025-02-28 09:51:05.240187-0800         624     stream.go:47
> qemu: #0 0xaaaac49f489c in keyval_parse_one
> third_party/qemu/util/keyval.c:190:5
> > >
> > > Signed-off-by: Peter Foley <pefoley@google.com>
> > > Signed-off-by: Nabih Estefan <nabihestefan@google.com>
> > > ---
> > >  util/keyval.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/util/keyval.c b/util/keyval.c
> > > index a70629a481..f33c64079d 100644
> > > --- a/util/keyval.c
> > > +++ b/util/keyval.c
> > > @@ -187,7 +187,7 @@ static const char *keyval_parse_one(QDict *qdict,
> const char *params,
> > >  {
> > >      const char *key, *key_end, *val_end, *s, *end;
> > >      size_t len;
> > > -    char key_in_cur[128];
> > > +    char key_in_cur[128] = {};
> > >      QDict *cur;
> > >      int ret;
> > >      QObject *next;
> >
> > Prior review of Peter's patch concluded this must be false positive:
> >
> https://lore.kernel.org/qemu-devel/14168384-ecdb-4c05-8267-ac5ef1c46fe9@redhat.com/
>
> While I agree with Paolo's reasoning, I think it is still worth adding an
> explicit initializer, because it makes it easier for both humans and
> machines
> to reason about correctless.
>

The problem is that, in the exact same (impossible) case there would have
to be another uninitialized variable, s. So the patch ends up making it
harder to understand what are the invariants of the function.

One should fix the compiler instead.

Paolo


> To reinforce that we don't have an actual bug though, also note that qemu
> unconditionally builds with -ftrivial-auto-var-init=zero. So if we happen
> to forget any, it won't cause a bug in the common case of a
> zero-initializer.
>
> With regards,
> Daniel
> --
> |: 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 :|
>
>

[-- Attachment #2: Type: text/html, Size: 4589 bytes --]