| Message ID | 20240714-arm-v1-1-c045bad45e48@daynix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/arm: Free GDB command data | expand |
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
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.
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); } }
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- target/arm/gdbstub.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) --- base-commit: f2cb4026fccfe073f84a4b440e41d3ed0c3134f6 change-id: 20240714-arm-045665f1c2ef Best regards,