This uninitialized value violates the contract in the
documentation comment, and may lead to a SEGV during
translaton with -d in_asm.
Change the documentation to disallow hostp NULL.
Pass hostp to probe_access_internal directly.
Reported-by: Panda Jiang <3160104094@zju.edu.cn>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
accel/tcg/internal-common.h | 2 +-
accel/tcg/cputlb.c | 7 +++----
accel/tcg/user-exec.c | 4 +---
3 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/accel/tcg/internal-common.h b/accel/tcg/internal-common.h
index 0ca13750f9..9e7be2d78d 100644
--- a/accel/tcg/internal-common.h
+++ b/accel/tcg/internal-common.h
@@ -82,7 +82,7 @@ void tb_check_watchpoint(CPUState *cpu, uintptr_t retaddr);
* See get_page_addr_code() (full-system version) for documentation on the
* return value.
*
- * Sets *@hostp (when @hostp is non-NULL) as follows.
+ * Sets *@hostp as follows.
* If the return value is -1, sets *@hostp to NULL. Otherwise, sets *@hostp
* to the host address where @addr's content is kept.
*
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 6900a12682..f9d9697a5a 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1545,7 +1545,9 @@ tb_page_addr_t get_page_addr_code_hostp(CPUArchState *env, vaddr addr,
(void)probe_access_internal(env_cpu(env), addr, 1, MMU_INST_FETCH,
cpu_mmu_index(env_cpu(env), true), false,
- &p, &full, 0, false);
+ hostp, &full, 0, false);
+
+ p = *hostp;
if (p == NULL) {
return -1;
}
@@ -1554,9 +1556,6 @@ tb_page_addr_t get_page_addr_code_hostp(CPUArchState *env, vaddr addr,
return -1;
}
- if (hostp) {
- *hostp = p;
- }
return qemu_ram_addr_from_host_nofail(p);
}
diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index ddbdc0432d..f8b4a26711 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -822,9 +822,7 @@ tb_page_addr_t get_page_addr_code_hostp(CPUArchState *env, vaddr addr,
flags = probe_access_internal(env, addr, 1, MMU_INST_FETCH, false, 0);
g_assert(flags == 0);
- if (hostp) {
- *hostp = g2h_untagged(addr);
- }
+ *hostp = g2h_untagged(addr);
return addr;
}
--
2.43.0On 1/28/26 9:07 AM, Richard Henderson wrote:
> This uninitialized value violates the contract in the
> documentation comment, and may lead to a SEGV during
> translaton with -d in_asm.
>
> Change the documentation to disallow hostp NULL.
> Pass hostp to probe_access_internal directly.
>
> Reported-by: Panda Jiang <3160104094@zju.edu.cn>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Hi Richard,
Thanks a lot for the patch!
> ---
> accel/tcg/internal-common.h | 2 +-
> accel/tcg/cputlb.c | 7 +++----
> accel/tcg/user-exec.c | 4 +---
> 3 files changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/accel/tcg/internal-common.h b/accel/tcg/internal-common.h
> ...
> @@ -1545,7 +1545,9 @@ tb_page_addr_t get_page_addr_code_hostp(CPUArchState *env, vaddr addr,
>
> (void)probe_access_internal(env_cpu(env), addr, 1, MMU_INST_FETCH,
> cpu_mmu_index(env_cpu(env), true), false,
> - &p, &full, 0, false);
> + hostp, &full, 0, false);
> +
> + p = *hostp;
> if (p == NULL) {
> return -1;
> }
> ...
I have a small question regarding the implementation choice. My initial
thought was to simply initialize *hostp to NULL at the beginning of
get_page_addr_code_hostp to solve the uninitialized variable issue.
I've been comparing that with your approach of passing 'hostp' directly
into probe_access_internal(). As I see it, the main functional
difference is that in your implementation, if probe_access_internal()
succeeds but the subsequent check
'if (full->lg_page_size < TARGET_PAGE_BITS)' fails, the function
returns -1, but *hostp will hold a valid pointer set by
probe_access_internal(). With the simple initialization approach, *hostp
would always be NULL in any failure case.
Is this behavior intentional? I was wondering if it provides some kind
of performance benefit or gives the caller more context even on this
specific failure path.
Thanks again for the fix!
Best regards,
Panda Jiang
On 1/29/26 17:13, Panda Jiang wrote:
> On 1/28/26 9:07 AM, Richard Henderson wrote:
>> This uninitialized value violates the contract in the
>> documentation comment, and may lead to a SEGV during
>> translaton with -d in_asm.
>>
>> Change the documentation to disallow hostp NULL.
>> Pass hostp to probe_access_internal directly.
>>
>> Reported-by: Panda Jiang <3160104094@zju.edu.cn>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>
> Hi Richard,
>
> Thanks a lot for the patch!
>
>> ---
>> accel/tcg/internal-common.h | 2 +-
>> accel/tcg/cputlb.c | 7 +++----
>> accel/tcg/user-exec.c | 4 +---
>> 3 files changed, 5 insertions(+), 8 deletions(-)
>>
>> diff --git a/accel/tcg/internal-common.h b/accel/tcg/internal-common.h
>> ...
>> @@ -1545,7 +1545,9 @@ tb_page_addr_t get_page_addr_code_hostp(CPUArchState *env, vaddr
>> addr,
>> (void)probe_access_internal(env_cpu(env), addr, 1, MMU_INST_FETCH,
>> cpu_mmu_index(env_cpu(env), true), false,
>> - &p, &full, 0, false);
>> + hostp, &full, 0, false);
>> +
>> + p = *hostp;
>> if (p == NULL) {
>> return -1;
>> }
>> ...
>
> I have a small question regarding the implementation choice. My initial
> thought was to simply initialize *hostp to NULL at the beginning of
> get_page_addr_code_hostp to solve the uninitialized variable issue.
>
> I've been comparing that with your approach of passing 'hostp' directly
> into probe_access_internal(). As I see it, the main functional
> difference is that in your implementation, if probe_access_internal()
> succeeds but the subsequent check
> 'if (full->lg_page_size < TARGET_PAGE_BITS)' fails, the function
> returns -1, but *hostp will hold a valid pointer set by
> probe_access_internal(). With the simple initialization approach, *hostp
> would always be NULL in any failure case.
Good catch. I will assign *hostp = NULL along that one path.
r~
© 2016 - 2026 Red Hat, Inc.