Re: [PATCH] target/arm: Free GDB command data

["Alex Bennée" <alex.bennee@linaro.org>]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Sun, 14 Jul 2024 at 11:43, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@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=UV92oeB6vUzT1GrQhRsTQ@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.

I'm working on cleaning this API up to make it easier to use. I'll send
a patch once its tested.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro