From fe7c8e26fcadc377e1b76435f767d665a0b69f94 Mon Sep 17 00:00:00 2001 From: Andrew Burgess Date: Wed, 4 Oct 2023 16:06:32 +0100 Subject: [PATCH] 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 (or nullptr on error). Back in handle_v_run I have converted new_program_name to also be a gdb::unique_xmalloc_ptr. 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. 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 --- 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 +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 (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 new_argv; - char *new_program_name = NULL; + gdb::unique_xmalloc_ptr 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 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); -- 2.30.2