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.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) (limited to 'gdb/cli/cli-script.c') diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c index 2101d6f..88afd1a 100644 --- a/gdb/cli/cli-script.c +++ b/gdb/cli/cli-script.c @@ -44,7 +44,7 @@ static enum command_control_type recurse_read_control_structure - (gdb::function_view read_next_line_func, + (read_next_line_ftype read_next_line_func, struct command_line *current_cmd, gdb::function_view validator); @@ -54,7 +54,7 @@ static void do_define_command (const char *comname, int from_tty, static void do_document_command (const char *comname, int from_tty, const counted_command_line *commands); -static const char *read_next_line (); +static const char *read_next_line (std::string &buffer); /* Level of control structure when reading. */ static int control_level; @@ -894,7 +894,7 @@ user_args::insert_args (const char *line) const from stdin. */ static const char * -read_next_line () +read_next_line (std::string &buffer) { struct ui *ui = current_ui; char *prompt_ptr, control_prompt[256]; @@ -917,7 +917,7 @@ read_next_line () else prompt_ptr = NULL; - return command_line_input (prompt_ptr, "commands"); + return command_line_input (buffer, prompt_ptr, "commands"); } /* Given an input line P, skip the command and return a pointer to the @@ -1064,7 +1064,7 @@ process_next_line (const char *p, command_line_up *command, obtain lines of the command. */ static enum command_control_type -recurse_read_control_structure (gdb::function_view read_next_line_func, +recurse_read_control_structure (read_next_line_ftype read_next_line_func, struct command_line *current_cmd, gdb::function_view validator) { @@ -1085,8 +1085,9 @@ recurse_read_control_structure (gdb::function_view read_next_li { dont_repeat (); + std::string buffer; next = nullptr; - val = process_next_line (read_next_line_func (), &next, + val = process_next_line (read_next_line_func (buffer), &next, current_cmd->control_type != python_control && current_cmd->control_type != guile_control && current_cmd->control_type != compile_control, @@ -1215,7 +1216,7 @@ read_command_lines (const char *prompt_arg, int from_tty, int parse_commands, obtained using READ_NEXT_LINE_FUNC. */ counted_command_line -read_command_lines_1 (gdb::function_view read_next_line_func, +read_command_lines_1 (read_next_line_ftype read_next_line_func, int parse_commands, gdb::function_view validator) { @@ -1231,7 +1232,9 @@ read_command_lines_1 (gdb::function_view read_next_line_func, while (1) { dont_repeat (); - val = process_next_line (read_next_line_func (), &next, parse_commands, + + std::string buffer; + val = process_next_line (read_next_line_func (buffer), &next, parse_commands, validator); /* Ignore blank lines or comments. */ -- cgit v1.1