diff mbox series

accel/tcg: fix msan findings in translate-all

Message ID 20250228212329.1775669-1-venture@google.com (mailing list archive)
State New
Headers show
Series accel/tcg: fix msan findings in translate-all | expand

Commit Message

Patrick Venture Feb. 28, 2025, 9:23 p.m. UTC 
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: Patrick Venture <venture@google.com>
---
 accel/tcg/translate-all.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Richard Henderson Feb. 28, 2025, 9:38 p.m. UTC | #1 
On 2/28/25 13:23, Patrick Venture wrote:
> 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: Patrick Venture <venture@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 d4189c7386..f584055a15 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -298,7 +298,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>       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();

False positive, because the error return exits without using the uninitialized value.
But if we do want to "fix" this, do it at the beginning of get_page_addr_code_hostp.

r~
Patrick Venture Feb. 28, 2025, 10:25 p.m. UTC | #2 
On Fri, Feb 28, 2025 at 1:38 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 2/28/25 13:23, Patrick Venture wrote:
> > 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: Patrick Venture <venture@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 d4189c7386..f584055a15 100644
> > --- a/accel/tcg/translate-all.c
> > +++ b/accel/tcg/translate-all.c
> > @@ -298,7 +298,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
> >       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();
>
> False positive, because the error return exits without using the
> uninitialized value.
> But if we do want to "fix" this, do it at the beginning of
> get_page_addr_code_hostp.
>

Acknowledged.  Gotta hate false positives, although better aggressive than
not. Thanks!


>
> r~
>
Peter Foley March 3, 2025, 4:34 p.m. UTC | #3 
For reference, the full output from msan looks like:

==4872==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0xaaaac681ef98 in tb_gen_code
third_party/qemu/accel/tcg/translate-all.c:358:21
    #1 0xaaaac67f2520 in cpu_exec_loop
third_party/qemu/accel/tcg/cpu-exec.c:993:22
    #2 0xaaaac67f154c in cpu_exec_setjmp
third_party/qemu/accel/tcg/cpu-exec.c:1039:12
    #3 0xaaaac67f1240 in cpu_exec third_party/qemu/accel/tcg/cpu-exec.c:1065:11
    #4 0xaaaac681022c in tcg_cpu_exec
third_party/qemu/accel/tcg/tcg-accel-ops.c:79:11
    #5 0xaaaac680ede4 in mttcg_cpu_thread_fn
third_party/qemu/accel/tcg/tcg-accel-ops-mttcg.c:95:17
    #6 0xaaaacf096698 in qemu_thread_start
third_party/qemu/util/qemu-thread-posix.c:541:9
    #7 0xffffa9242cec in start_thread
(/usr/grte/v5/lib64/libpthread.so.0+0xbcec) (BuildId:
0bdac2117d4465a78d3de57b307368b4)
    #8 0xffffa912ec98 in thread_start
(/usr/grte/v5/lib64/libc.so.6+0x116c98) (BuildId:
613d20d3b812b4c87fe9ebf8c4caae83)

  Uninitialized value was stored to memory at
    #0 0xaaaac681ef94 in tb_gen_code
third_party/qemu/accel/tcg/translate-all.c:358:50
    #1 0xaaaac67f2520 in cpu_exec_loop
third_party/qemu/accel/tcg/cpu-exec.c:993:22
    #2 0xaaaac67f154c in cpu_exec_setjmp
third_party/qemu/accel/tcg/cpu-exec.c:1039:12
    #3 0xaaaac67f1240 in cpu_exec third_party/qemu/accel/tcg/cpu-exec.c:1065:11
    #4 0xaaaac681022c in tcg_cpu_exec
third_party/qemu/accel/tcg/tcg-accel-ops.c:79:11
    #5 0xaaaac680ede4 in mttcg_cpu_thread_fn
third_party/qemu/accel/tcg/tcg-accel-ops-mttcg.c:95:17
    #6 0xaaaacf096698 in qemu_thread_start
third_party/qemu/util/qemu-thread-posix.c:541:9
    #7 0xffffa9242cec in start_thread
(/usr/grte/v5/lib64/libpthread.so.0+0xbcec) (BuildId:
0bdac2117d4465a78d3de57b307368b4)
    #8 0xffffa912ec98 in thread_start
(/usr/grte/v5/lib64/libc.so.6+0x116c98) (BuildId:
613d20d3b812b4c87fe9ebf8c4caae83)

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

SUMMARY: MemorySanitizer: use-of-uninitialized-value
third_party/qemu/accel/tcg/translate-all.c:358:21 in tb_gen_code
Exiting


On Fri, Feb 28, 2025 at 5:26 PM Patrick Venture <venture@google.com> wrote:

>
>
> On Fri, Feb 28, 2025 at 1:38 PM Richard Henderson <
> richard.henderson@linaro.org> wrote:
>
>> On 2/28/25 13:23, Patrick Venture wrote:
>> > 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: Patrick Venture <venture@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 d4189c7386..f584055a15 100644
>> > --- a/accel/tcg/translate-all.c
>> > +++ b/accel/tcg/translate-all.c
>> > @@ -298,7 +298,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>> >       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();
>>
>> False positive, because the error return exits without using the
>> uninitialized value.
>> But if we do want to "fix" this, do it at the beginning of
>> get_page_addr_code_hostp.
>>
>
> Acknowledged.  Gotta hate false positives, although better aggressive than
> not. Thanks!
>
>
>>
>> r~
>>
>
Peter Foley March 3, 2025, 4:40 p.m. UTC | #4 
And interestingly enough, it appears that execution continues even with
early return from get_page_addr_code_hostp:
https://gitlab.com/qemu-project/qemu/-/blob/master/accel/tcg/translate-all.c?ref_type=heads#L308

Which implies that we could still have an uninitialized value here:
https://gitlab.com/qemu-project/qemu/-/blob/master/accel/tcg/translate-all.c?ref_type=heads#L362

On Mon, Mar 3, 2025 at 11:34 AM Peter Foley <pefoley@google.com> wrote:

> For reference, the full output from msan looks like:
>
> ==4872==WARNING: MemorySanitizer: use-of-uninitialized-value
>     #0 0xaaaac681ef98 in tb_gen_code third_party/qemu/accel/tcg/translate-all.c:358:21
>     #1 0xaaaac67f2520 in cpu_exec_loop third_party/qemu/accel/tcg/cpu-exec.c:993:22
>     #2 0xaaaac67f154c in cpu_exec_setjmp third_party/qemu/accel/tcg/cpu-exec.c:1039:12
>     #3 0xaaaac67f1240 in cpu_exec third_party/qemu/accel/tcg/cpu-exec.c:1065:11
>     #4 0xaaaac681022c in tcg_cpu_exec third_party/qemu/accel/tcg/tcg-accel-ops.c:79:11
>     #5 0xaaaac680ede4 in mttcg_cpu_thread_fn third_party/qemu/accel/tcg/tcg-accel-ops-mttcg.c:95:17
>     #6 0xaaaacf096698 in qemu_thread_start third_party/qemu/util/qemu-thread-posix.c:541:9
>     #7 0xffffa9242cec in start_thread (/usr/grte/v5/lib64/libpthread.so.0+0xbcec) (BuildId: 0bdac2117d4465a78d3de57b307368b4)
>     #8 0xffffa912ec98 in thread_start (/usr/grte/v5/lib64/libc.so.6+0x116c98) (BuildId: 613d20d3b812b4c87fe9ebf8c4caae83)
>
>   Uninitialized value was stored to memory at
>     #0 0xaaaac681ef94 in tb_gen_code third_party/qemu/accel/tcg/translate-all.c:358:50
>     #1 0xaaaac67f2520 in cpu_exec_loop third_party/qemu/accel/tcg/cpu-exec.c:993:22
>     #2 0xaaaac67f154c in cpu_exec_setjmp third_party/qemu/accel/tcg/cpu-exec.c:1039:12
>     #3 0xaaaac67f1240 in cpu_exec third_party/qemu/accel/tcg/cpu-exec.c:1065:11
>     #4 0xaaaac681022c in tcg_cpu_exec third_party/qemu/accel/tcg/tcg-accel-ops.c:79:11
>     #5 0xaaaac680ede4 in mttcg_cpu_thread_fn third_party/qemu/accel/tcg/tcg-accel-ops-mttcg.c:95:17
>     #6 0xaaaacf096698 in qemu_thread_start third_party/qemu/util/qemu-thread-posix.c:541:9
>     #7 0xffffa9242cec in start_thread (/usr/grte/v5/lib64/libpthread.so.0+0xbcec) (BuildId: 0bdac2117d4465a78d3de57b307368b4)
>     #8 0xffffa912ec98 in thread_start (/usr/grte/v5/lib64/libc.so.6+0x116c98) (BuildId: 613d20d3b812b4c87fe9ebf8c4caae83)
>
>   Uninitialized value was created by an allocation of 'host_pc' in the stack frame
>     #0 0xaaaac681d8ac in tb_gen_code third_party/qemu/accel/tcg/translate-all.c:297:5
>
> SUMMARY: MemorySanitizer: use-of-uninitialized-value third_party/qemu/accel/tcg/translate-all.c:358:21 in tb_gen_code
> Exiting
>
>
> On Fri, Feb 28, 2025 at 5:26 PM Patrick Venture <venture@google.com>
> wrote:
>
>>
>>
>> On Fri, Feb 28, 2025 at 1:38 PM Richard Henderson <
>> richard.henderson@linaro.org> wrote:
>>
>>> On 2/28/25 13:23, Patrick Venture wrote:
>>> > 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: Patrick Venture <venture@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 d4189c7386..f584055a15 100644
>>> > --- a/accel/tcg/translate-all.c
>>> > +++ b/accel/tcg/translate-all.c
>>> > @@ -298,7 +298,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>>> >       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();
>>>
>>> False positive, because the error return exits without using the
>>> uninitialized value.
>>> But if we do want to "fix" this, do it at the beginning of
>>> get_page_addr_code_hostp.
>>>
>>
>> Acknowledged.  Gotta hate false positives, although better aggressive
>> than not. Thanks!
>>
>>
>>>
>>> r~
>>>
>>
diff mbox series

Patch

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index d4189c7386..f584055a15 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -298,7 +298,7 @@  TranslationBlock *tb_gen_code(CPUState *cpu,
     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();