On Sun, 14 Jul 2024 at 11:43, Akihiko Odaki <akihiko.od...@daynix.com> wrote:
>
> Signed-off-by: Akihiko Odaki <akihiko.od...@daynix.com>
> ---
> target/arm/gdbstub.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
> index c3a9b5eb1ed2..03f77362efc1 100644
> --- a/target/arm/gdbstub.c
> +++ b/target/arm/gdbstub.c
> @@ -477,11 +477,11 @@ static GDBFeature
> *arm_gen_dynamic_m_secextreg_feature(CPUState *cs,
>
> void arm_cpu_register_gdb_commands(ARMCPU *cpu)
> {
> - GArray *query_table =
> + g_autoptr(GArray) query_table =
> g_array_new(FALSE, FALSE, sizeof(GdbCmdParseEntry));
> - GArray *set_table =
> + g_autoptr(GArray) set_table =
> g_array_new(FALSE, FALSE, sizeof(GdbCmdParseEntry));
> - GString *qsupported_features = g_string_new(NULL);
> + g_autoptr(GString) qsupported_features = g_string_new(NULL);
>
> if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
> #ifdef TARGET_AARCH64
> @@ -495,6 +495,7 @@ void arm_cpu_register_gdb_commands(ARMCPU *cpu)
> gdb_extend_query_table(&g_array_index(query_table,
> GdbCmdParseEntry, 0),
> query_table->len);
> + g_array_free(g_steal_pointer(&query_table), FALSE);
> }
>
> /* Set arch-specific handlers for 'Q' commands. */
> @@ -502,11 +503,13 @@ void arm_cpu_register_gdb_commands(ARMCPU *cpu)
> gdb_extend_set_table(&g_array_index(set_table,
> GdbCmdParseEntry, 0),
> set_table->len);
> + g_array_free(g_steal_pointer(&set_table), FALSE);
> }
>
> /* Set arch-specific qSupported feature. */
> if (qsupported_features->len) {
> gdb_extend_qsupported_features(qsupported_features->str);
> + g_string_free(g_steal_pointer(&qsupported_features), FALSE);
> }
> }
I don't think this is the right approach to this leak (which
Coverity also complained about):
https://lore.kernel.org/qemu-devel/CAFEAcA8YJwWtQxdOe2wmH7i0jvjU=uv92oeb6vuzt1grqhr...@mail.gmail.com/
I think the underlying problem is that the gdb_extend_query_table
and gdb_extend_set_table functions have a weird API where they
retain pointers to a chunk of the contents of the GArray but
they don't get passed the actual GArray. My take is that it
would be better to make the API to those functions more natural
(either "take the whole GArray and take ownership of it" or
else "copy the info they need and the caller retains ownership
of both the GArray and its contents").
Also, there is a second leak here if you have more than one
CPU -- when the second CPU calls gdb_extend_query_table() etc,
the function will leak the first CPU's data. Having the function
API be clearly either "always takes ownership" or "never takes
ownership" would make it easier to fix this leak too.
thanks
-- PMM
