[PATCH] util/keyval: fix msan findings

[PATCH] util/keyval: fix msan findings

[Patrick Venture]

From: Peter Foley <pefoley@google.com>

e.g.
qemu: Uninitialized value was created by an allocation of 'key_in_cur.i' in the stack frame
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: Patrick Venture <venture@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.48.1.711.g2feabab25a-goog

Re: [PATCH] util/keyval: fix msan findings

[Paolo Bonzini]

On 2/28/25 22:20, Patrick Venture wrote:
> From: Peter Foley <pefoley@google.com>
> 
> e.g.
> qemu: Uninitialized value was created by an allocation of 'key_in_cur.i' in the stack frame
> 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: Patrick Venture <venture@google.com>

This is not a fix, since there's no bug to fix.  It's just the tool 
complaining about something it can't reason on.

Paolo

> ---
>   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;

Re: [PATCH] util/keyval: fix msan findings

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 2/28/25 22:20, Patrick Venture wrote:
>> From: Peter Foley <pefoley@google.com>
>> e.g.
>> qemu: Uninitialized value was created by an allocation of 'key_in_cur.i' in the stack frame
>> 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: Patrick Venture <venture@google.com>
>
> This is not a fix, since there's no bug to fix.  It's just the tool complaining about something it can't reason on.
>
> Paolo

The code is designed to read @keyval_in_cur only in non-first iterations
of the loop.  The previous iteration assigned to it then.

The two lines you quoted don't make sense to me.  Is this the full
report you got?  If not, show us the full report, please.  Ideally with
a reproducer.

>> ---
>>  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] = {};

Suspect overkill.  Would "" do?

>>      QDict *cur;
>>      int ret;
>>      QObject *next;

Re: [PATCH] util/keyval: fix msan findings

[Peter Foley]

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

On Sat, Mar 1, 2025 at 2:14 AM Markus Armbruster <armbru@redhat.com> wrote:

> Paolo Bonzini <pbonzini@redhat.com> writes:
>
> > On 2/28/25 22:20, Patrick Venture wrote:
> >> From: Peter Foley <pefoley@google.com>
> >> e.g.
> >> qemu: Uninitialized value was created by an allocation of
> 'key_in_cur.i' in the stack frame
> >> 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: Patrick Venture <venture@google.com>
> >
> > This is not a fix, since there's no bug to fix.  It's just the tool
> complaining about something it can't reason on.
> >
> > Paolo
>
> The code is designed to read @keyval_in_cur only in non-first iterations
> of the loop.  The previous iteration assigned to it then.
>
> The two lines you quoted don't make sense to me.  Is this the full
> report you got?  If not, show us the full report, please.  Ideally with
> a reproducer.
>

The full output looks like:

Uninitialized bytes in strlen at offset 0 inside [0xffffd1958110, 5)
==9780==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0xaaaac1c4b170 in tdb_hash third_party/qemu/qobject/qdict.c:46:31
    #1 0xaaaac1c4b4a4 in qdict_get third_party/qemu/qobject/qdict.c:164:36
    #2 0xaaaac1c78468 in keyval_parse_put third_party/qemu/util/keyval.c:152:11
    #3 0xaaaac1c77740 in keyval_parse_one third_party/qemu/util/keyval.c:295:10
    #4 0xaaaac1c77740 in keyval_parse_into third_party/qemu/util/keyval.c:530:13
    #5 0xaaaaba2f9524 in qemu_init third_party/qemu/system/vl.c:3322:21
    #6 0xaaaab9641c2c in main third_party/qemu/system/main.c:54:5
    #7 0xffffba320000 in __libc_start_main
(/usr/grte/v5/lib64/libc.so.6+0x61000) (BuildId:
613d20d3b812b4c87fe9ebf8c4caae83)
    #8 0xaaaab934bd10 in _start
/usr/grte/v5/debug-src/src/csu/../sysdeps/aarch64/start.S:92

  Uninitialized value was created by an allocation of 'key_in_cur.i'
in the stack frame
    #0 0xaaaac1c7708c in keyval_parse_one third_party/qemu/util/keyval.c:190:5
    #1 0xaaaac1c7708c in keyval_parse_into third_party/qemu/util/keyval.c:530:13

SUMMARY: MemorySanitizer: use-of-uninitialized-value
third_party/qemu/qobject/qdict.c:46:31 in tdb_hash
Exiting


I don't have an easily shareable reproducer, but it's probably possible to
whip one up.



>
> >> ---
> >>  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] = {};
>
> Suspect overkill.  Would "" do?
>

It appears to resolve the complaint, yes.

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

Re: [PATCH] util/keyval: fix msan findings

[Paolo Bonzini]

On 3/3/25 17:32, Peter Foley wrote:
> The full output looks like:
> 
> Uninitialized bytes in strlen at offset 0 inside [0xffffd1958110, 5)
> ==9780==WARNING: MemorySanitizer: use-of-uninitialized-value
>      #0 0xaaaac1c4b170 in tdb_hash third_party/qemu/qobject/qdict.c:46:31
>      #1 0xaaaac1c4b4a4 in qdict_get third_party/qemu/qobject/qdict.c:164:36
>      #2 0xaaaac1c78468 in keyval_parse_put third_party/qemu/util/keyval.c:152:11
>      #3 0xaaaac1c77740 in keyval_parse_one third_party/qemu/util/keyval.c:295:10

In order to get to

     if (!keyval_parse_put(cur, key_in_cur, qstring_from_gstring(val),
                           key, key_end, errp)) {

you must have gone through the only exit path of the earlier
"for(;;)" loop:

         memcpy(key_in_cur, s, len);
         key_in_cur[len] = 0;
         s += len;

         if (*s != '.') {
             break;
         }

meaning that key_in_cur is NULL-terminated and initialized---unless s is
also uninitialized, but then adding an initializer would not do anything.

Paolo

>      #4 0xaaaac1c77740 in keyval_parse_into third_party/qemu/util/keyval.c:530:13
>      #5 0xaaaaba2f9524 in qemu_init third_party/qemu/system/vl.c:3322:21
>      #6 0xaaaab9641c2c in main third_party/qemu/system/main.c:54:5
>      #7 0xffffba320000 in __libc_start_main (/usr/grte/v5/lib64/libc.so.6+0x61000) (BuildId: 613d20d3b812b4c87fe9ebf8c4caae83)
>      #8 0xaaaab934bd10 in _start /usr/grte/v5/debug-src/src/csu/../sysdeps/aarch64/start.S:92
> 
>    Uninitialized value was created by an allocation of 'key_in_cur.i' in the stack frame
>      #0 0xaaaac1c7708c in keyval_parse_one third_party/qemu/util/keyval.c:190:5
>      #1 0xaaaac1c7708c in keyval_parse_into third_party/qemu/util/keyval.c:530:13
> 
> SUMMARY: MemorySanitizer: use-of-uninitialized-value third_party/qemu/qobject/qdict.c:46:31 in tdb_hash
> Exiting
> 
> 
> I don't have an easily shareable reproducer, but it's probably possible 
> to whip one up.
> 
> 
>      >> ---
>      >>  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] = {};
> 
>     Suspect overkill.  Would "" do?
> 
> 
> It appears to resolve the complaint, yes.