diff options
author | Andrew Burgess <aburgess@redhat.com> | 2023-10-04 16:06:32 +0100 |
---|---|---|
committer | Andrew Burgess <aburgess@redhat.com> | 2023-10-25 16:51:19 +0100 |
commit | fe7c8e26fcadc377e1b76435f767d665a0b69f94 (patch) | |
tree | 6d37fbbde9cf57e26f8d3e75bbf56498d08fb3de /gdbserver/server.cc | |
parent | f9b96f673e6c442fe81cd1b5c27cb83ae50ae63b (diff) | |
download | gdb-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.cc | 70 |
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); |