* [PATCH] gdbserver: fix memory leak when handling qsupported packet
@ 2020-07-09 1:53 Simon Marchi
2020-07-09 22:30 ` Christian Biesinger
0 siblings, 1 reply; 5+ messages in thread
From: Simon Marchi @ 2020-07-09 1:53 UTC (permalink / raw)
To: gdb-patches
When building gdbserver with AddressSanitizer, I get this annoying
little leak when gdbserver exits:
==307817==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 14 byte(s) in 1 object(s) allocated from:
#0 0x7f7fd4256459 in __interceptor_malloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:145
#1 0x563bef981b80 in xmalloc /home/simark/src/binutils-gdb/gdbserver/../gdb/alloc.c:60
#2 0x563befb53301 in xstrdup /home/simark/src/binutils-gdb/libiberty/xstrdup.c:34
#3 0x563bef9d742b in handle_query /home/simark/src/binutils-gdb/gdbserver/server.cc:2286
#4 0x563bef9ed0b7 in process_serial_event /home/simark/src/binutils-gdb/gdbserver/server.cc:4061
#5 0x563bef9f1d9e in handle_serial_event(int, void*) /home/simark/src/binutils-gdb/gdbserver/server.cc:4402
#6 0x563befb0ec65 in handle_file_event /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:548
#7 0x563befb0f49f in gdb_wait_for_event /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:673
#8 0x563befb0d4a1 in gdb_do_one_event() /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:215
#9 0x563bef9e721a in start_event_loop /home/simark/src/binutils-gdb/gdbserver/server.cc:3484
#10 0x563bef9eb90a in captured_main /home/simark/src/binutils-gdb/gdbserver/server.cc:3875
#11 0x563bef9ec2c7 in main /home/simark/src/binutils-gdb/gdbserver/server.cc:3961
#12 0x7f7fd3330001 in __libc_start_main (/usr/lib/libc.so.6+0x27001)
SUMMARY: AddressSanitizer: 14 byte(s) leaked in 1 allocation(s).
This is due to the handling of unknown qsupported features in
handle_query. The `qsupported` vector is built, containing all the
feature names received from GDB. As we iterate on them, when we
encounter unknown ones, we move them at the beginning of the vector, in
preparation of passing this vector of unknown features down to the
target (which may know about them).
When moving these unknown features to other slots in the vector, we
overwrite other pointers without freeing them, which therefore leak.
An easy fix would be to add a `free` when doing the move. However, I
think this is a good opportunity to sprinkle a bit of automatic memory
management in this code.
So, use a vector of unique pointers which owns all the entries. And use
a separate vector (that doesn't own the entries) for the unknown ones,
which is then passed to target_process_qsupported.
gdbserver/ChangeLog:
* server.cc (handle_query): Use std::vector of
gdb::unique_xmalloc_ptr for `qsupported` vector. Use separate
vector for unknowns.
Change-Id: I97f133825faa6d7abbf83a58504eb0ba77462812
---
gdbserver/server.cc | 27 ++++++++-------------------
1 file changed, 8 insertions(+), 19 deletions(-)
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 0313d18bb247..c909dcdd4d00 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -2269,10 +2269,8 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
';'. */
if (*p == ':')
{
- char **qsupported = NULL;
- int count = 0;
- int unknown = 0;
- int i;
+ std::vector<gdb::unique_xmalloc_ptr<char>> qsupported;
+ std::vector<char *> unknowns;
/* Two passes, to avoid nested strtok calls in
target_process_qsupported. */
@@ -2280,15 +2278,12 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
for (p = strtok_r (p + 1, ";", &saveptr);
p != NULL;
p = strtok_r (NULL, ";", &saveptr))
- {
- count++;
- qsupported = XRESIZEVEC (char *, qsupported, count);
- qsupported[count - 1] = xstrdup (p);
- }
+ qsupported.emplace_back (make_unique_xstrdup (p));
- for (i = 0; i < count; i++)
+ for (const gdb::unique_xmalloc_ptr<char> &up : qsupported)
{
- p = qsupported[i];
+ p = up.get ();
+
if (strcmp (p, "multiprocess+") == 0)
{
/* GDB supports and wants multi-process support if
@@ -2347,19 +2342,13 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
else
{
/* Move the unknown features all together. */
- qsupported[i] = NULL;
- qsupported[unknown] = p;
- unknown++;
+ unknowns.push_back (p);
}
}
/* Give the target backend a chance to process the unknown
features. */
- target_process_qsupported (qsupported, unknown);
-
- for (i = 0; i < count; i++)
- free (qsupported[i]);
- free (qsupported);
+ target_process_qsupported (unknowns.data (), unknowns.size ());
}
sprintf (own_buf,
--
2.27.0
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gdbserver: fix memory leak when handling qsupported packet
2020-07-09 1:53 [PATCH] gdbserver: fix memory leak when handling qsupported packet Simon Marchi
@ 2020-07-09 22:30 ` Christian Biesinger
2020-07-10 2:42 ` Simon Marchi
0 siblings, 1 reply; 5+ messages in thread
From: Christian Biesinger @ 2020-07-09 22:30 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches
On Wed, Jul 8, 2020 at 8:53 PM Simon Marchi via Gdb-patches
<gdb-patches@sourceware.org> wrote:
> + qsupported.emplace_back (make_unique_xstrdup (p));
Why not make it a vector of std::string? You'd avoid an extra alloc
for short features.
Christian
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gdbserver: fix memory leak when handling qsupported packet
2020-07-09 22:30 ` Christian Biesinger
@ 2020-07-10 2:42 ` Simon Marchi
2020-07-13 18:41 ` Tom Tromey
0 siblings, 1 reply; 5+ messages in thread
From: Simon Marchi @ 2020-07-10 2:42 UTC (permalink / raw)
To: Christian Biesinger; +Cc: gdb-patches
On 2020-07-09 6:30 p.m., Christian Biesinger wrote:
> On Wed, Jul 8, 2020 at 8:53 PM Simon Marchi via Gdb-patches
> <gdb-patches@sourceware.org> wrote:
>> + qsupported.emplace_back (make_unique_xstrdup (p));
>
> Why not make it a vector of std::string? You'd avoid an extra alloc
> for short features.
Yeah I can do that. Although to the build the `unknowns` vector, I
have to use `.c_str ()`, which returns a `const char *`. And
currently we pass an array of non-const `char *` to the low target,
so I couldn't pass an array of `const char *`.
It's not as daunting as I thought to change the target.h interface,
since only x86 implements it.
I thought about using `gdb::array_view<gdb::string_view>` as the
parameter type for the `process_qsupported` method, but that
introduced too much unnecessary changes to the code... so I settled
on `gdb::array_view<const char * const>`.
Here's the updated version.
From db634f2e59bf6d4c68f6dfc3d025bbc8909144ba Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Wed, 8 Jul 2020 19:38:48 -0400
Subject: [PATCH] gdbserver: fix memory leak when handling qsupported packet
When building gdbserver with AddressSanitizer, I get this annoying
little leak when gdbserver exits:
==307817==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 14 byte(s) in 1 object(s) allocated from:
#0 0x7f7fd4256459 in __interceptor_malloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:145
#1 0x563bef981b80 in xmalloc /home/simark/src/binutils-gdb/gdbserver/../gdb/alloc.c:60
#2 0x563befb53301 in xstrdup /home/simark/src/binutils-gdb/libiberty/xstrdup.c:34
#3 0x563bef9d742b in handle_query /home/simark/src/binutils-gdb/gdbserver/server.cc:2286
#4 0x563bef9ed0b7 in process_serial_event /home/simark/src/binutils-gdb/gdbserver/server.cc:4061
#5 0x563bef9f1d9e in handle_serial_event(int, void*) /home/simark/src/binutils-gdb/gdbserver/server.cc:4402
#6 0x563befb0ec65 in handle_file_event /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:548
#7 0x563befb0f49f in gdb_wait_for_event /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:673
#8 0x563befb0d4a1 in gdb_do_one_event() /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:215
#9 0x563bef9e721a in start_event_loop /home/simark/src/binutils-gdb/gdbserver/server.cc:3484
#10 0x563bef9eb90a in captured_main /home/simark/src/binutils-gdb/gdbserver/server.cc:3875
#11 0x563bef9ec2c7 in main /home/simark/src/binutils-gdb/gdbserver/server.cc:3961
#12 0x7f7fd3330001 in __libc_start_main (/usr/lib/libc.so.6+0x27001)
SUMMARY: AddressSanitizer: 14 byte(s) leaked in 1 allocation(s).
This is due to the handling of unknown qsupported features in
handle_query. The `qsupported` vector is built, containing all the
feature names received from GDB. As we iterate on them, when we
encounter unknown ones, we move them at the beginning of the vector, in
preparation of passing this vector of unknown features down to the
target (which may know about them).
When moving these unknown features to other slots in the vector, we
overwrite other pointers without freeing them, which therefore leak.
An easy fix would be to add a `free` when doing the move. However, I
think this is a good opportunity to sprinkle a bit of automatic memory
management in this code.
So, use a vector of std::string which owns all the entries. And use a
separate vector (that doesn't own the entries) for the unknown ones,
which is then passed to target_process_qsupported.
Given that the `c_str` method of std::string returns a `const char *`,
it follows that process_stratum_target::process_qsupported must accept a
`const char **` instead of a `char **`. And while at it, change the
pointer + size paramters to use an array_view instead.
gdbserver/ChangeLog:
* server.cc (handle_query): Use std::vector of
std::string for `qsupported` vector. Use separate
vector for unknowns.
* target.h (class process_stratum_target) <process_qsupported>:
Change parameters to array_view of const char *.
(target_process_qsupported): Remove `count` parameter.
* target.cc (process_stratum_target::process_qsupported): Change
parameters to array_view of const char *.
* linux-x86-low.cc (class x86_target) <process_qsupported>:
Likewise.
Change-Id: I97f133825faa6d7abbf83a58504eb0ba77462812
---
gdbserver/linux-x86-low.cc | 12 +++++-----
gdbserver/server.cc | 45 ++++++++++++++------------------------
gdbserver/target.cc | 3 ++-
gdbserver/target.h | 10 +++++----
4 files changed, 29 insertions(+), 41 deletions(-)
diff --git a/gdbserver/linux-x86-low.cc b/gdbserver/linux-x86-low.cc
index 7a65c1d079f8..37d3626e1f73 100644
--- a/gdbserver/linux-x86-low.cc
+++ b/gdbserver/linux-x86-low.cc
@@ -106,7 +106,7 @@ public:
bool supports_z_point_type (char z_type) override;
- void process_qsupported (char **features, int count) override;
+ void process_qsupported (gdb::array_view<const char * const> features) override;
bool supports_tracepoints () override;
@@ -1003,18 +1003,15 @@ x86_target::update_xmltarget ()
PTRACE_GETREGSET. */
void
-x86_target::process_qsupported (char **features, int count)
+x86_target::process_qsupported (gdb::array_view<const char * const> features)
{
- int i;
-
/* Return if gdb doesn't support XML. If gdb sends "xmlRegisters="
with "i386" in qSupported query, it supports x86 XML target
descriptions. */
use_xml = 0;
- for (i = 0; i < count; i++)
- {
- const char *feature = features[i];
+ for (const char *feature : features)
+ {
if (startswith (feature, "xmlRegisters="))
{
char *copy = xstrdup (feature + 13);
@@ -1034,6 +1031,7 @@ x86_target::process_qsupported (char **features, int count)
free (copy);
}
}
+
update_xmltarget ();
}
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 0313d18bb247..ab5363eb0328 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -2269,10 +2269,8 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
';'. */
if (*p == ':')
{
- char **qsupported = NULL;
- int count = 0;
- int unknown = 0;
- int i;
+ std::vector<std::string> qsupported;
+ std::vector<const char *> unknowns;
/* Two passes, to avoid nested strtok calls in
target_process_qsupported. */
@@ -2280,28 +2278,23 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
for (p = strtok_r (p + 1, ";", &saveptr);
p != NULL;
p = strtok_r (NULL, ";", &saveptr))
- {
- count++;
- qsupported = XRESIZEVEC (char *, qsupported, count);
- qsupported[count - 1] = xstrdup (p);
- }
+ qsupported.emplace_back (p);
- for (i = 0; i < count; i++)
+ for (const std::string &feature : qsupported)
{
- p = qsupported[i];
- if (strcmp (p, "multiprocess+") == 0)
+ if (feature == "multiprocess+")
{
/* GDB supports and wants multi-process support if
possible. */
if (target_supports_multi_process ())
cs.multi_process = 1;
}
- else if (strcmp (p, "qRelocInsn+") == 0)
+ else if (feature == "qRelocInsn+")
{
/* GDB supports relocate instruction requests. */
gdb_supports_qRelocInsn = 1;
}
- else if (strcmp (p, "swbreak+") == 0)
+ else if (feature == "swbreak+")
{
/* GDB wants us to report whether a trap is caused
by a software breakpoint and for us to handle PC
@@ -2309,36 +2302,36 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
if (target_supports_stopped_by_sw_breakpoint ())
cs.swbreak_feature = 1;
}
- else if (strcmp (p, "hwbreak+") == 0)
+ else if (feature == "hwbreak+")
{
/* GDB wants us to report whether a trap is caused
by a hardware breakpoint. */
if (target_supports_stopped_by_hw_breakpoint ())
cs.hwbreak_feature = 1;
}
- else if (strcmp (p, "fork-events+") == 0)
+ else if (feature == "fork-events+")
{
/* GDB supports and wants fork events if possible. */
if (target_supports_fork_events ())
cs.report_fork_events = 1;
}
- else if (strcmp (p, "vfork-events+") == 0)
+ else if (feature == "vfork-events+")
{
/* GDB supports and wants vfork events if possible. */
if (target_supports_vfork_events ())
cs.report_vfork_events = 1;
}
- else if (strcmp (p, "exec-events+") == 0)
+ else if (feature == "exec-events+")
{
/* GDB supports and wants exec events if possible. */
if (target_supports_exec_events ())
cs.report_exec_events = 1;
}
- else if (strcmp (p, "vContSupported+") == 0)
+ else if (feature == "vContSupported+")
cs.vCont_supported = 1;
- else if (strcmp (p, "QThreadEvents+") == 0)
+ else if (feature == "QThreadEvents+")
;
- else if (strcmp (p, "no-resumed+") == 0)
+ else if (feature == "no-resumed+")
{
/* GDB supports and wants TARGET_WAITKIND_NO_RESUMED
events. */
@@ -2347,19 +2340,13 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
else
{
/* Move the unknown features all together. */
- qsupported[i] = NULL;
- qsupported[unknown] = p;
- unknown++;
+ unknowns.push_back (feature.c_str ());
}
}
/* Give the target backend a chance to process the unknown
features. */
- target_process_qsupported (qsupported, unknown);
-
- for (i = 0; i < count; i++)
- free (qsupported[i]);
- free (qsupported);
+ target_process_qsupported (unknowns);
}
sprintf (own_buf,
diff --git a/gdbserver/target.cc b/gdbserver/target.cc
index b5190e1f52a2..87f62a0b5559 100644
--- a/gdbserver/target.cc
+++ b/gdbserver/target.cc
@@ -599,7 +599,8 @@ process_stratum_target::read_loadmap (const char *annex,
}
void
-process_stratum_target::process_qsupported (char **features, int count)
+process_stratum_target::process_qsupported
+ (gdb::array_view<const char * const> features)
{
/* Nop. */
}
diff --git a/gdbserver/target.h b/gdbserver/target.h
index 701c8ef87675..13f069f7729f 100644
--- a/gdbserver/target.h
+++ b/gdbserver/target.h
@@ -27,6 +27,7 @@
#include "target/wait.h"
#include "target/waitstatus.h"
#include "mem-break.h"
+#include "gdbsupport/array-view.h"
#include "gdbsupport/btrace-common.h"
#include <vector>
@@ -315,8 +316,9 @@ class process_stratum_target
unsigned char *myaddr, unsigned int len);
/* Target specific qSupported support. FEATURES is an array of
- features with COUNT elements. */
- virtual void process_qsupported (char **features, int count);
+ features unsupported by the core of GDBserver. */
+ virtual void process_qsupported
+ (gdb::array_view<const char * const> features);
/* Return true if the target supports tracepoints, false otherwise. */
virtual bool supports_tracepoints ();
@@ -547,8 +549,8 @@ int kill_inferior (process_info *proc);
#define target_async(enable) \
the_target->async (enable)
-#define target_process_qsupported(features, count) \
- the_target->process_qsupported (features, count)
+#define target_process_qsupported(features) \
+ the_target->process_qsupported (features)
#define target_supports_catch_syscall() \
the_target->supports_catch_syscall ()
--
2.27.0
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gdbserver: fix memory leak when handling qsupported packet
2020-07-10 2:42 ` Simon Marchi
@ 2020-07-13 18:41 ` Tom Tromey
2020-07-14 2:28 ` Simon Marchi
0 siblings, 1 reply; 5+ messages in thread
From: Tom Tromey @ 2020-07-13 18:41 UTC (permalink / raw)
To: Simon Marchi via Gdb-patches
>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
Simon> gdbserver/ChangeLog:
Simon> * server.cc (handle_query): Use std::vector of
Simon> std::string for `qsupported` vector. Use separate
Simon> vector for unknowns.
Simon> * target.h (class process_stratum_target) <process_qsupported>:
Simon> Change parameters to array_view of const char *.
Simon> (target_process_qsupported): Remove `count` parameter.
Simon> * target.cc (process_stratum_target::process_qsupported): Change
Simon> parameters to array_view of const char *.
Simon> * linux-x86-low.cc (class x86_target) <process_qsupported>:
Simon> Likewise.
FWIW this seems fine to me. Thanks for cleaning this up.
Tom
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gdbserver: fix memory leak when handling qsupported packet
2020-07-13 18:41 ` Tom Tromey
@ 2020-07-14 2:28 ` Simon Marchi
0 siblings, 0 replies; 5+ messages in thread
From: Simon Marchi @ 2020-07-14 2:28 UTC (permalink / raw)
To: Tom Tromey, Simon Marchi via Gdb-patches
On 2020-07-13 2:41 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Simon> gdbserver/ChangeLog:
>
> Simon> * server.cc (handle_query): Use std::vector of
> Simon> std::string for `qsupported` vector. Use separate
> Simon> vector for unknowns.
> Simon> * target.h (class process_stratum_target) <process_qsupported>:
> Simon> Change parameters to array_view of const char *.
> Simon> (target_process_qsupported): Remove `count` parameter.
> Simon> * target.cc (process_stratum_target::process_qsupported): Change
> Simon> parameters to array_view of const char *.
> Simon> * linux-x86-low.cc (class x86_target) <process_qsupported>:
> Simon> Likewise.
>
> FWIW this seems fine to me. Thanks for cleaning this up.
>
> Tom
>
Thanks, I pushed it.
Simon
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-07-14 2:28 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-09 1:53 [PATCH] gdbserver: fix memory leak when handling qsupported packet Simon Marchi
2020-07-09 22:30 ` Christian Biesinger
2020-07-10 2:42 ` Simon Marchi
2020-07-13 18:41 ` Tom Tromey
2020-07-14 2:28 ` Simon Marchi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).