Imagelibrary/binutils-gdb
1
0
Fork 1
mirror of https://github.com/bminor/binutils-gdb.git synced 2025-12-26 17:18:55 +00:00
Code Issues Packages Projects Releases Wiki Activity

gdbserver: don't leak program name in handle_v_run

Browse Source 
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 7ab2607f97
  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>
This commit is contained in:
Andrew Burgess
2023-10-04 16:06:32 +01:00
parent f9b96f673e
commit fe7c8e26fc
1 changed files with 56 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files

70
gdbserver/server.cc
View File

@@ -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);