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

[Paolo Bonzini <pbonzini@redhat.com>]

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.