[PATCH] target/arm: Free GDB command data

[PATCH] target/arm: Free GDB command data

[Akihiko Odaki]

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);
     }
 }
 

---
base-commit: f2cb4026fccfe073f84a4b440e41d3ed0c3134f6
change-id: 20240714-arm-045665f1c2ef

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>

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

[Peter Maydell]

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.

thanks
-- PMM

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

[Alex Bennée]

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