aboutsummaryrefslogtreecommitdiff
path: root/gdbserver/server.cc
diff options
context:
space:
mode:
authorAndrew Burgess <aburgess@redhat.com>2023-10-04 16:06:32 +0100
committerAndrew Burgess <aburgess@redhat.com>2023-10-25 16:51:19 +0100
commitfe7c8e26fcadc377e1b76435f767d665a0b69f94 (patch)
tree6d37fbbde9cf57e26f8d3e75bbf56498d08fb3de /gdbserver/server.cc
parentf9b96f673e6c442fe81cd1b5c27cb83ae50ae63b (diff)
downloadgdb-fe7c8e26fcadc377e1b76435f767d665a0b69f94.zip
gdb-fe7c8e26fcadc377e1b76435f767d665a0b69f94.tar.gz
gdb-fe7c8e26fcadc377e1b76435f767d665a0b69f94.tar.bz2
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>
Diffstat (limited to 'gdbserver/server.cc')
-rw-r--r--gdbserver/server.cc70
1 files changed, 56 insertions, 14 deletions
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index e02cdb8..5f2032c 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);