[binutils-gdb] gdbserver: don't leak program name in handle_v_run
Andrew Burgess
aburgess@sourceware.org
Wed Oct 25 15:51:23 GMT 2023
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=fe7c8e26fcadc377e1b76435f767d665a0b69f94
commit fe7c8e26fcadc377e1b76435f767d665a0b69f94
Author: Andrew Burgess <aburgess@redhat.com>
Date: Wed Oct 4 16:06:32 2023 +0100
gdbserver: don't leak program name in handle_v_run
I noticed that in handle_v_run (gdbserver/server.cc) we leak
new_program_name (a string) each time GDB starts an inferior, in the
case where GDB passes a program name to gdbserver.
This bug was introduced with this commit:
commit 7ab2607f97e5deaeae91018edf3ef5b4255a842c
Date: Wed Apr 13 17:31:02 2022 -0400
gdbsupport: make gdb_abspath return an std::string
When gdbserver receives a program name from GDB, this is first placed
into a malloc'd buffer within handle_v_run, and this buffer is then
used in this call:
program_path.set (new_program_name);
Prior to the above commit this call took ownership of the buffer
passed to it, but now this call uses the buffer to initialise a
std::string, which copies the buffer contents, leaving ownership with
the caller. So now, after this call (in handle_v_run)
new_program_name still owns a buffer.
At no point in handle_v_run do we free new_program_name, as a result
we are leaking the program name each time GDB starts a remote
inferior.
I could solve this by adding a 'free' call into handle_v_run, but I'd
rather automate the memory management.
So, to this end, I have added a new function in gdbserver/server.cc,
decode_v_run_arg. This function takes care of allocating the memory
buffer and decoding the vRun packet into the buffer, but returns a
gdb::unique_xmalloc_ptr<char> (or nullptr on error).
Back in handle_v_run I have converted new_program_name to also be a
gdb::unique_xmalloc_ptr<char>.
Now, after we call program_path.set(), the allocated buffer will be
automatically released when it is no longer needed.
It is worth highlighting that within the new decode_v_run_arg
function, I have wrapped the call to hex2bin in a try/catch block.
The hex2bin function can throw an exception if it encounters an
invalid (non-hex) character. Back in handle_v_run, we have a local
argument new_argv, which is of type std::vector<char *>. Each
'char *' in this vector is a malloc'd buffer. If we allow
hex2bin to throw an exception and don't catch it in either
decode_v_run_arg or handle_v_run then we are going to leak memory from
new_argv.
I chose to catch the exception in decode_v_run_arg, this seemed
cleanest, but I'm not sure it really matters, so long as the exception
is caught before we leave handle_v_run. I am working on a patch that
changes new_argv to automatically manage its memory, but that isn't
ready for posting yet. I think what I have here would be fine if my
follow on patch never arrives.
Additionally, within the handle_v_run loop I have changed an
assignment of nullptr to new_program_name into an assert. Previously,
the assignment could only trigger on the first iteration of the loop,
if we had no new program name to assign. However, new_program_name
always starts as nullptr, so, on the first loop iteration, if we have
nothing to assign to new_program_name, its value must already be
nullptr.
There should be no user visible changes after this commit.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Diff:
---
gdbserver/server.cc | 70 ++++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 56 insertions(+), 14 deletions(-)
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index e02cdb83b51..5f2032c37c1 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -2959,6 +2959,46 @@ handle_v_attach (char *own_buf)
write_enn (own_buf);
}
+/* Decode an argument from the vRun packet buffer. PTR points to the
+ first hex-encoded character in the buffer, and LEN is the number of
+ characters to read from the packet buffer.
+
+ If the argument decoding is successful, return a buffer containing the
+ decoded argument, including a null terminator at the end.
+
+ If the argument decoding fails for any reason, return nullptr. */
+
+static gdb::unique_xmalloc_ptr<char>
+decode_v_run_arg (const char *ptr, size_t len)
+{
+ /* Two hex characters are required for each decoded byte. */
+ if (len % 2 != 0)
+ return nullptr;
+
+ /* The length in bytes needed for the decoded argument. */
+ len /= 2;
+
+ /* Buffer to decode the argument into. The '+ 1' is for the null
+ terminator we will add. */
+ char *arg = (char *) xmalloc (len + 1);
+
+ /* Decode the argument from the packet and add a null terminator. We do
+ this within a try block as invalid characters within the PTR buffer
+ will cause hex2bin to throw an exception. Our caller relies on us
+ returning nullptr in order to clean up some memory allocations. */
+ try
+ {
+ hex2bin (ptr, (gdb_byte *) arg, len);
+ arg[len] = '\0';
+ }
+ catch (const gdb_exception_error &exception)
+ {
+ return nullptr;
+ }
+
+ return gdb::unique_xmalloc_ptr<char> (arg);
+}
+
/* Run a new program. */
static void
handle_v_run (char *own_buf)
@@ -2966,7 +3006,7 @@ handle_v_run (char *own_buf)
client_state &cs = get_client_state ();
char *p, *next_p;
std::vector<char *> new_argv;
- char *new_program_name = NULL;
+ gdb::unique_xmalloc_ptr<char> new_program_name;
int i;
for (i = 0, p = own_buf + strlen ("vRun;");
@@ -2980,7 +3020,7 @@ handle_v_run (char *own_buf)
if (i == 0 && p == next_p)
{
/* No program specified. */
- new_program_name = NULL;
+ gdb_assert (new_program_name == nullptr);
}
else if (p == next_p)
{
@@ -2989,29 +3029,31 @@ handle_v_run (char *own_buf)
}
else
{
- /* The length of the decoded argument. */
- size_t len = (next_p - p) / 2;
+ /* The length of the argument string in the packet. */
+ size_t len = next_p - p;
- /* Buffer to decode the argument into. */
- char *arg = (char *) xmalloc (len + 1);
-
- hex2bin (p, (gdb_byte *) arg, len);
- arg[len] = '\0';
+ gdb::unique_xmalloc_ptr<char> arg = decode_v_run_arg (p, len);
+ if (arg == nullptr)
+ {
+ write_enn (own_buf);
+ free_vector_argv (new_argv);
+ return;
+ }
if (i == 0)
- new_program_name = arg;
+ new_program_name = std::move (arg);
else
- new_argv.push_back (arg);
+ new_argv.push_back (arg.release ());
}
if (*next_p == '\0')
break;
}
- if (new_program_name == NULL)
+ if (new_program_name == nullptr)
{
/* GDB didn't specify a program to run. Use the program from the
last run with the new argument list. */
- if (program_path.get () == NULL)
+ if (program_path.get () == nullptr)
{
write_enn (own_buf);
free_vector_argv (new_argv);
@@ -3019,7 +3061,7 @@ handle_v_run (char *own_buf)
}
}
else
- program_path.set (new_program_name);
+ program_path.set (new_program_name.get ());
/* Free the old argv and install the new one. */
free_vector_argv (program_args);
More information about the Gdb-cvs
mailing list