[PATCH] gdbserver: add client_state destructor

[PATCH] gdbserver: add client_state destructor

[Guinevere Larsen]

the struct client_state in gdbserver allocated memory dynamically in its
constructor, but it never deallocates it. This was pointed out by a
static analyzer that ran on the repo. While I think this was done
because client_states are never destructed, I think it would still be
good to have a destructor in case that ever changes, either on purpose
or by accident.

Tested with native-gdbserver on x86_64.
---
 gdbserver/server.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/gdbserver/server.h b/gdbserver/server.h
index 0074818c6df..256294b0052 100644
--- a/gdbserver/server.h
+++ b/gdbserver/server.h
@@ -133,6 +133,12 @@ struct client_state
     own_buf ((char *) xmalloc (PBUFSIZ + 1)) 
   {}
 
+  ~client_state ()
+  {
+    if (own_buf != nullptr)
+      xfree (own_buf);
+  }
+
   /* The thread set with an `Hc' packet.  `Hc' is deprecated in favor of
      `vCont'.  Note the multi-process extensions made `vCont' a
      requirement, so `Hc pPID.TID' is pretty much undefined.  So
-- 
2.44.0

Re: [PATCH] gdbserver: add client_state destructor

[Keith Seitz]

On 5/3/24 05:29, Guinevere Larsen wrote:
> ---
>   gdbserver/server.h | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/gdbserver/server.h b/gdbserver/server.h
> index 0074818c6df..256294b0052 100644
> --- a/gdbserver/server.h
> +++ b/gdbserver/server.h
> @@ -133,6 +133,12 @@ struct client_state
>       own_buf ((char *) xmalloc (PBUFSIZ + 1))
>     {}
>   
> +  ~client_state ()
> +  {
> +    if (own_buf != nullptr)
> +      xfree (own_buf);
> +  }
> +

You don't need to guard xfree -- it deals gracefully with nullptr.

Reviewed-by: Keith Seitz <keiths@redhat.com>

Keith

>     /* The thread set with an `Hc' packet.  `Hc' is deprecated in favor of
>        `vCont'.  Note the multi-process extensions made `vCont' a
>        requirement, so `Hc pPID.TID' is pretty much undefined.  So

Re: [PATCH] gdbserver: add client_state destructor

[Tom Tromey]

>>>>> "Guinevere" == Guinevere Larsen <blarsen@redhat.com> writes:

Guinevere> +  ~client_state ()
Guinevere> +  {
Guinevere> +    if (own_buf != nullptr)
Guinevere> +      xfree (own_buf);
Guinevere> +  }

A null check isn't needed when calling xfree.
At some point we went through the tree and removed all these IIRC.

Tom

Re: [PATCH] gdbserver: add client_state destructor

[Guinevere Larsen]

On 5/3/24 10:31, Tom Tromey wrote:
>>>>>> "Guinevere" == Guinevere Larsen <blarsen@redhat.com> writes:
> Guinevere> +  ~client_state ()
> Guinevere> +  {
> Guinevere> +    if (own_buf != nullptr)
> Guinevere> +      xfree (own_buf);
> Guinevere> +  }
>
> A null check isn't needed when calling xfree.
Thanks (and thanks Keith). I'll remove this locally.
> At some point we went through the tree and removed all these IIRC.
I think I have a case of friday brains. You mean removed all nullptr 
checks, or removed unnecessary xfrees?

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

>
> Tom
>

Re: [PATCH] gdbserver: add client_state destructor

[Tom Tromey]

>> At some point we went through the tree and removed all these IIRC.

Guinevere> I think I have a case of friday brains. You mean removed all nullptr
Guinevere> checks, or removed unnecessary xfrees?

Sorry.  All the checks of null before free were removed at some point.

Though I see that gdb-xfree.h still does the check.  Lol.
IIRC, the gnulib folks looked into this and concluded that no existing
system requires the check before a call to free.

Tom

Re: [PATCH] gdbserver: add client_state destructor

[Pedro Alves]

On 2024-05-03 16:17, Tom Tromey wrote:
>>> At some point we went through the tree and removed all these IIRC.
> 
> Guinevere> I think I have a case of friday brains. You mean removed all nullptr
> Guinevere> checks, or removed unnecessary xfrees?
> 
> Sorry.  All the checks of null before free were removed at some point.
> 
> Though I see that gdb-xfree.h still does the check.  Lol.
> IIRC, the gnulib folks looked into this and concluded that no existing
> system requires the check before a call to free.

That is the original point of xfree existing.  It was needed on some ancient
Solaris, IIRC.  It's free didn't work right with a null argument.

Note that bfd uses xmalloc, but does not use xfree, so all such hosts are surely
gone.

Nowadays, having xfree is still important for the POD checking, of course,
and it's nice to have xmalloc/xfree for balance IMHO, thought that's of course
debatable.

Re: [PATCH] gdbserver: add client_state destructor

[Simon Marchi]

On 2024-05-03 12:14, Pedro Alves wrote:
> On 2024-05-03 16:17, Tom Tromey wrote:
>>>> At some point we went through the tree and removed all these IIRC.
>>
>> Guinevere> I think I have a case of friday brains. You mean removed all nullptr
>> Guinevere> checks, or removed unnecessary xfrees?
>>
>> Sorry.  All the checks of null before free were removed at some point.
>>
>> Though I see that gdb-xfree.h still does the check.  Lol.
>> IIRC, the gnulib folks looked into this and concluded that no existing
>> system requires the check before a call to free.
> 
> That is the original point of xfree existing.  It was needed on some ancient
> Solaris, IIRC.  It's free didn't work right with a null argument.
> 
> Note that bfd uses xmalloc, but does not use xfree, so all such hosts are surely
> gone.
> 
> Nowadays, having xfree is still important for the POD checking, of course,
> and it's nice to have xmalloc/xfree for balance IMHO, thought that's of course
> debatable.

We delete free for !IsFreeable types:

template <typename T, typename = gdb::Requires<gdb::Not<IsFreeable<T>>>>
void free (T *ptr) = delete;

So I guess that xfree isn't technically necessary for that.

Anyway, our goal should be for free/xfree to never be called manually :)

Simon

Re: [PATCH] gdbserver: add client_state destructor

[Guinevere Larsen]

On 5/3/24 14:00, Simon Marchi wrote:
>
> On 2024-05-03 12:14, Pedro Alves wrote:
>> On 2024-05-03 16:17, Tom Tromey wrote:
>>>>> At some point we went through the tree and removed all these IIRC.
>>> Guinevere> I think I have a case of friday brains. You mean removed all nullptr
>>> Guinevere> checks, or removed unnecessary xfrees?
>>>
>>> Sorry.  All the checks of null before free were removed at some point.
>>>
>>> Though I see that gdb-xfree.h still does the check.  Lol.
>>> IIRC, the gnulib folks looked into this and concluded that no existing
>>> system requires the check before a call to free.
>> That is the original point of xfree existing.  It was needed on some ancient
>> Solaris, IIRC.  It's free didn't work right with a null argument.
>>
>> Note that bfd uses xmalloc, but does not use xfree, so all such hosts are surely
>> gone.
>>
>> Nowadays, having xfree is still important for the POD checking, of course,
>> and it's nice to have xmalloc/xfree for balance IMHO, thought that's of course
>> debatable.
> We delete free for !IsFreeable types:
>
> template <typename T, typename = gdb::Requires<gdb::Not<IsFreeable<T>>>>
> void free (T *ptr) = delete;
>
> So I guess that xfree isn't technically necessary for that.
>
> Anyway, our goal should be for free/xfree to never be called manually :)
I'm somewhat confused about this comment. Does this mean you don't want 
this patch in?
>
> Simon
>
-- 
Cheers,
Guinevere Larsen
She/Her/Hers