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>

0 files changed, 0 insertions, 0 deletions