From f8631e5e04dbef678323e9be6b7329f39049d2c4 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Thu, 15 Dec 2022 14:06:25 -0500 Subject: gdb: remove static buffer in command_line_input [I sent this earlier today, but I don't see it in the archives. Resending it through a different computer / SMTP.] The use of the static buffer in command_line_input is becoming problematic, as explained here [1]. In short, with this patch [2] that attempt to fix a post-hook bug, when running gdb.base/commands.exp, we hit a case where we read a "define" command line from a script file using command_command_line_input. The command line is stored in command_line_input's static buffer. Inside the define command's execution, we read the lines inside the define using command_line_input, which overwrites the define command, in command_line_input's static buffer. After the execution of the define command, execute_command does a command look up to see if a post-hook is registered. For that, it uses a now stale pointer that used to point to the define command, in the static buffer, causing a use-after-free. Note that the pointer in execute_command points to the dynamically-allocated buffer help by the static buffer in command_line_input, not to the static object itself, hence why we see a use-after-free. Fix that by removing the static buffer. I initially changed command_line_input and other related functions to return an std::string, which is the obvious but naive solution. The thing is that some callees don't need to return an allocated string, so this this an unnecessary pessimization. I changed it to passing in a reference to an std::string buffer, which the callee can use if it needs to return dynamically-allocated content. It fills the buffer and returns a pointers to the C string inside. The callees that don't need to return dynamically-allocated content simply don't use it. So, it started with modifying command_line_input as described above, all the other changes derive directly from that. One slightly shady thing is in handle_line_of_input, where we now pass a pointer to an std::string's internal buffer to readline's history_value function, which takes a `char *`. I'm pretty sure that this function does not modify the input string, because I was able to change it (with enough massaging) to take a `const char *`. A subtle change is that we now clear a UI's line buffer using a SCOPE_EXIT in command_line_handler, after executing the command. This was previously done by this line in handle_line_of_input: /* We have a complete command line now. Prepare for the next command, but leave ownership of memory to the buffer . */ cmd_line_buffer->used_size = 0; I think the new way is clearer. [1] https://inbox.sourceware.org/gdb-patches/becb8438-81ef-8ad8-cc42-fcbfaea8cddd@simark.ca/ [2] https://inbox.sourceware.org/gdb-patches/20221213112241.621889-1-jan.vrany@labware.com/ Change-Id: I8fc89b1c69870c7fc7ad9c1705724bd493596300 Reviewed-By: Tom Tromey --- gdb/cli/cli-script.h | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) (limited to 'gdb/cli/cli-script.h') diff --git a/gdb/cli/cli-script.h b/gdb/cli/cli-script.h index 2b9483f..d9ca7de 100644 --- a/gdb/cli/cli-script.h +++ b/gdb/cli/cli-script.h @@ -112,11 +112,18 @@ private: } }; +/* Prototype for a function to call to get one more input line. + + If the function needs to return a dynamically allocated string, it can place + in the passed-in buffer, and return a pointer to it. Otherwise, it can + simply ignore it. */ + +using read_next_line_ftype = gdb::function_view; + extern counted_command_line read_command_lines (const char *, int, int, gdb::function_view); extern counted_command_line read_command_lines_1 - (gdb::function_view, int, - gdb::function_view); + (read_next_line_ftype, int, gdb::function_view); /* Exported to cli/cli-cmds.c */ -- cgit v1.1