Re: [PULL 39/40] gdbstub: Add support for MTE in user mode

[Peter Maydell]

On Fri, 5 Jul 2024 at 16:37, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> From: Gustavo Romero <gustavo.romero@linaro.org>
>
> This commit implements the stubs to handle the qIsAddressTagged,
> qMemTag, and QMemTag GDB packets, allowing all GDB 'memory-tag'
> subcommands to work with QEMU gdbstub on aarch64 user mode. It also
> implements the get/set functions for the special GDB MTE register
> 'tag_ctl', used to control the MTE fault type at runtime.
>
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> Message-Id: <20240628050850.536447-11-gustavo.romero@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20240705084047.857176-40-alex.bennee@linaro.org>

Hi; Coverity thinks there's a memory leak in this function
(CID 1549757):

> +void arm_cpu_register_gdb_commands(ARMCPU *cpu)
> +{
> +    GArray *query_table =
> +        g_array_new(FALSE, FALSE, sizeof(GdbCmdParseEntry));
> +    GArray *set_table =
> +        g_array_new(FALSE, FALSE, sizeof(GdbCmdParseEntry));
> +    GString *qsupported_features = g_string_new(NULL);

We allocate memory for the qsupported_features GString here, but
it looks like nowhere in the function either passes it to
some other function that takes ownership of it, and we don't
free it either.

> +    if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
> +    #ifdef TARGET_AARCH64
> +        aarch64_cpu_register_gdb_commands(cpu, qsupported_features, 
> query_table,
> +                                          set_table);
> +    #endif

(PS: this ifdef/endif ought to be on the left margin, not indented.)

> +    }
> +
> +    /* Set arch-specific handlers for 'q' commands. */
> +    if (query_table->len) {
> +        gdb_extend_query_table(&g_array_index(query_table,
> +                                              GdbCmdParseEntry, 0),
> +                                              query_table->len);
> +    }
> +
> +    /* Set arch-specific handlers for 'Q' commands. */
> +    if (set_table->len) {
> +        gdb_extend_set_table(&g_array_index(set_table,
> +                             GdbCmdParseEntry, 0),
> +                             set_table->len);
> +    }
> +
> +    /* Set arch-specific qSupported feature. */
> +    if (qsupported_features->len) {
> +        gdb_extend_qsupported_features(qsupported_features->str);
> +    }
> +}

Something odd also seems to be happening with the query_table
and set_table -- we allocate them in this function, and we
rely on the memory not going away because we pass pointers
into the actual array data into gdb_extend_query_table()
and gdb_extend_set_table() which those functions retain copies
of, but we don't keep hold of the pointer to the struct GArray
itself.

The functions gdb_extend_set_table() and gdb_extend_query_table()
both seem to not quite do what their documentation says they
do. The docs say they extend the set table and the query table,
but the implementation appears to be setting an extended set
table/query table (e.g. you can't extend the table twice).

Perhaps a more natural API would be for these functions to
either:
 * be passed in the entire GArray, and take ownership of
   it and be responsible for disposing of it
 * take a copy of the entries they're passed in, so that
   the calling function can free the GArray and its
   contents after the call
?

thanks
-- PMM