Re: [PATCH 13/20] disas/nanomips: Add free() calls

[Thomas Huth <thuth@redhat.com>]

On 15/08/2022 09.26, Milica Lazarevic wrote:
> The free() function is called for every string allocated using the
> strdup() function to prevent memory leaking.
> 
> The implementation of the several functions working with dynamically
> allocated strings is slightly changed so we can free those strings.
> 
> Almost every disassembly_function returns the result of the img_format()
> function, which returns a dynamically allocated string. To be able to
> free that string for every disassembly_function, a strdup() call is
> added for returning value of some disassembly functions like TLBGINV,
> TLBGINVF, TLBGP, etc.
> 
> Signed-off-by: Milica Lazarevic <milica.lazarevic@syrmia.com>
> ---
>   disas/nanomips.cpp | 1117 +++++++++++++++++++++++++++++++++-----------
>   1 file changed, 841 insertions(+), 276 deletions(-)
> 
> diff --git a/disas/nanomips.cpp b/disas/nanomips.cpp
> index 561e4ff095..551bcb3164 100644
> --- a/disas/nanomips.cpp
> +++ b/disas/nanomips.cpp
> @@ -526,7 +526,9 @@ static const char *save_restore_list(uint64 rt, uint64 count, uint64 gp)
>       for (uint64 counter = 0; counter != count; counter++) {
>           bool use_gp = gp && (counter == count - 1);
>           uint64 this_rt = use_gp ? 28 : ((rt & 0x10) | (rt + counter)) & 0x1f;
> -        strcat(str, img_format(",%s", GPR(this_rt)));
> +        const char *dis_str = img_format(",%s", GPR(this_rt));
> +        strcat(str, dis_str);
> +        free((char *)dis_str);

When using glib, you could get rid of the free() here by declaring dis_str 
with g_autofree.

>       }
>   
>       return strdup(str);
> @@ -663,7 +665,9 @@ static int Disassemble(const uint16 *data, char *dis,
>                                   return -6;
>                               }
>                               type = table[i].type;
> -                            strcpy(dis, dis_fn(op_code));
> +                            const char *dis_str = dis_fn(op_code);
> +                            strcpy(dis, dis_str);
> +                            free((char *)dis_str);

dito

>                               return table[i].instructions_size;
>                           } else {
>                               strcpy(dis, "reserved instruction");
> @@ -1737,7 +1741,10 @@ static const char *ACLR(uint64 instruction)
>       const char *s = IMMEDIATE(copy(s_value));
>       const char *rs = GPR(copy(rs_value));
>   
> -    return img_format("ACLR %s, %s(%s)", bit, s, rs);
> +    const char *ret = img_format("ACLR %s, %s(%s)", bit, s, rs);
> +    free((char *)bit);
> +    free((char *)s);
> +    return ret;
>   }
>   
>   
> @@ -1833,7 +1840,9 @@ static const char *ADDIU_32_(uint64 instruction)
>       const char *rs = GPR(copy(rs_value));
>       const char *u = IMMEDIATE(copy(u_value));
>   
> -    return img_format("ADDIU %s, %s, %s", rt, rs, u);
> +    const char *ret = img_format("ADDIU %s, %s, %s", rt, rs, u);
> +    free((char *)u);

I really dislike the need of having these "(char *)" casts in the calls to 
free() everywhere.
IMHO, if a function allocated memory and the caller is required to free it, 
the memory belongs to the caller after the function has finished, so it 
would be better to return "char *" instead of "const char *" in these cases. 
"const char *" should be used in the cases where the return value points to 
static strings that the caller must not free.
(Apart from that, please consider to use g_autofree for the variables 
declaration everywhere you added a free() here ... that would make this 
patch way easier).

  Thomas