| Message ID | 20250228212039.1768614-1-venture@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | util/keyval: fix msan findings | expand |
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;
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;
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.
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.
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;