From: Simon Marchi Date: Thu, 15 Dec 2022 19:06:25 +0000 (-0500) Subject: gdb: remove static buffer in command_line_input X-Git-Url: https://git.libre-soc.org/?a=commitdiff_plain;h=f8631e5e04dbef678323e9be6b7329f39049d2c4;p=binutils-gdb.git 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 --- diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c index 5054bdb2957..d59a5ae576e 100644 --- a/gdb/ada-lang.c +++ b/gdb/ada-lang.c @@ -3561,7 +3561,8 @@ get_selections (int *choices, int n_choices, int max_results, if (prompt == NULL) prompt = "> "; - args = command_line_input (prompt, annotation_suffix); + std::string buffer; + args = command_line_input (buffer, prompt, annotation_suffix); if (args == NULL) error_no_arg (_("one or more choice numbers")); diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index f0276a963c0..7a245b6fb5f 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -13782,8 +13782,8 @@ strace_command (const char *arg, int from_tty) static struct uploaded_tp *this_utp; static int next_cmd; -static char * -read_uploaded_action (void) +static const char * +read_uploaded_action (std::string &buffer) { char *rslt = nullptr; diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c index 2101d6fface..88afd1a82aa 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. */ diff --git a/gdb/cli/cli-script.h b/gdb/cli/cli-script.h index 2b9483f115c..d9ca7de5128 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 */ diff --git a/gdb/defs.h b/gdb/defs.h index f51ab9e5c0c..4771d02a92a 100644 --- a/gdb/defs.h +++ b/gdb/defs.h @@ -316,7 +316,8 @@ typedef void initialize_file_ftype (void); extern char *gdb_readline_wrapper (const char *); -extern const char *command_line_input (const char *, const char *); +extern const char *command_line_input (std::string &cmd_line_buffer, + const char *, const char *); extern void print_prompt (void); diff --git a/gdb/event-top.c b/gdb/event-top.c index bcf80bbd7d0..fa86a89e4a1 100644 --- a/gdb/event-top.c +++ b/gdb/event-top.c @@ -483,13 +483,13 @@ struct ui *main_ui; struct ui *current_ui; struct ui *ui_list; -/* Get a pointer to the current UI's line buffer. This is used to +/* Get a reference to the current UI's line buffer. This is used to construct a whole line of input from partial input. */ -static struct buffer * +static std::string & get_command_line_buffer (void) { - return ¤t_ui->line_buffer; + return current_ui->line_buffer; } /* When there is an event ready on the stdin file descriptor, instead @@ -620,46 +620,41 @@ command_handler (const char *command) } } -/* Append RL, an input line returned by readline or one of its - emulations, to CMD_LINE_BUFFER. Returns the command line if we - have a whole command line ready to be processed by the command - interpreter or NULL if the command line isn't complete yet (input - line ends in a backslash). */ +/* Append RL, an input line returned by readline or one of its emulations, to + CMD_LINE_BUFFER. Return true if we have a whole command line ready to be + processed by the command interpreter or false if the command line isn't + complete yet (input line ends in a backslash). */ -static char * -command_line_append_input_line (struct buffer *cmd_line_buffer, const char *rl) +static bool +command_line_append_input_line (std::string &cmd_line_buffer, const char *rl) { - char *cmd; - size_t len; - - len = strlen (rl); + size_t len = strlen (rl); if (len > 0 && rl[len - 1] == '\\') { /* Don't copy the backslash and wait for more. */ - buffer_grow (cmd_line_buffer, rl, len - 1); - cmd = NULL; + cmd_line_buffer.append (rl, len - 1); + return false; } else { /* Copy whole line including terminating null, and we're done. */ - buffer_grow (cmd_line_buffer, rl, len + 1); - cmd = cmd_line_buffer->buffer; + cmd_line_buffer.append (rl, len + 1); + return true; } - - return cmd; } /* Handle a line of input coming from readline. - If the read line ends with a continuation character (backslash), - save the partial input in CMD_LINE_BUFFER (except the backslash), - and return NULL. Otherwise, save the partial input and return a - pointer to CMD_LINE_BUFFER's buffer (null terminated), indicating a - whole command line is ready to be executed. + If the read line ends with a continuation character (backslash), return + nullptr. Otherwise, return a pointer to the command line, indicating a whole + command line is ready to be executed. - Returns EOF on end of file. + The returned pointer may or may not point to CMD_LINE_BUFFER's internal + buffer. + + Return EOF on end of file. If REPEAT, handle command repetitions: @@ -670,38 +665,32 @@ command_line_append_input_line (struct buffer *cmd_line_buffer, const char *rl) command instead of the empty input line. */ -char * -handle_line_of_input (struct buffer *cmd_line_buffer, +const char * +handle_line_of_input (std::string &cmd_line_buffer, const char *rl, int repeat, const char *annotation_suffix) { struct ui *ui = current_ui; int from_tty = ui->instream == ui->stdin_stream; - char *p1; - char *cmd; if (rl == NULL) return (char *) EOF; - cmd = command_line_append_input_line (cmd_line_buffer, rl); - if (cmd == NULL) + bool complete = command_line_append_input_line (cmd_line_buffer, rl); + if (!complete) return NULL; - /* 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; - if (from_tty && annotation_level > 1) printf_unfiltered (("\n\032\032post-%s\n"), annotation_suffix); #define SERVER_COMMAND_PREFIX "server " - server_command = startswith (cmd, SERVER_COMMAND_PREFIX); + server_command = startswith (cmd_line_buffer.c_str (), SERVER_COMMAND_PREFIX); if (server_command) { /* Note that we don't call `save_command_line'. Between this and the check in dont_repeat, this insures that repeating will still do the right thing. */ - return cmd + strlen (SERVER_COMMAND_PREFIX); + return cmd_line_buffer.c_str () + strlen (SERVER_COMMAND_PREFIX); } /* Do history expansion if that is wished. */ @@ -710,32 +699,29 @@ handle_line_of_input (struct buffer *cmd_line_buffer, char *cmd_expansion; int expanded; - expanded = history_expand (cmd, &cmd_expansion); + /* Note: here, we pass a pointer to the std::string's internal buffer as + a `char *`. At the time of writing, readline's history_expand does + not modify the passed-in string. Ideally, readline should be modified + to make that parameter `const char *`. */ + expanded = history_expand (&cmd_line_buffer[0], &cmd_expansion); gdb::unique_xmalloc_ptr history_value (cmd_expansion); if (expanded) { - size_t len; - /* Print the changes. */ printf_unfiltered ("%s\n", history_value.get ()); /* If there was an error, call this function again. */ if (expanded < 0) - return cmd; - - /* history_expand returns an allocated string. Just replace - our buffer with it. */ - len = strlen (history_value.get ()); - xfree (buffer_finish (cmd_line_buffer)); - cmd_line_buffer->buffer = history_value.get (); - cmd_line_buffer->buffer_size = len + 1; - cmd = history_value.release (); + return cmd_line_buffer.c_str (); + + cmd_line_buffer = history_value.get (); } } /* If we just got an empty line, and that is supposed to repeat the previous command, return the previously saved command. */ - for (p1 = cmd; *p1 == ' ' || *p1 == '\t'; p1++) + const char *p1; + for (p1 = cmd_line_buffer.c_str (); *p1 == ' ' || *p1 == '\t'; p1++) ; if (repeat && *p1 == '\0') return get_saved_command_line (); @@ -747,17 +733,21 @@ handle_line_of_input (struct buffer *cmd_line_buffer, and then later fetch it from the value history and remove the '#'. The kill ring is probably better, but some people are in the habit of commenting things out. */ - if (*cmd != '\0' && from_tty && current_ui->input_interactive_p ()) - gdb_add_history (cmd); + if (cmd_line_buffer[0] != '\0' && from_tty && current_ui->input_interactive_p ()) + gdb_add_history (cmd_line_buffer.c_str ()); /* Save into global buffer if appropriate. */ if (repeat) { - save_command_line (cmd); + save_command_line (cmd_line_buffer.c_str ()); + + /* It is important that we return a pointer to the saved command line + here, for the `cmd_start == saved_command_line` check in + execute_command to work. */ return get_saved_command_line (); } - else - return cmd; + + return cmd_line_buffer.c_str (); } /* See event-top.h. */ @@ -790,11 +780,10 @@ gdb_rl_deprep_term_function (void) void command_line_handler (gdb::unique_xmalloc_ptr &&rl) { - struct buffer *line_buffer = get_command_line_buffer (); + std::string &line_buffer = get_command_line_buffer (); struct ui *ui = current_ui; - char *cmd; - cmd = handle_line_of_input (line_buffer, rl.get (), 1, "prompt"); + const char *cmd = handle_line_of_input (line_buffer, rl.get (), 1, "prompt"); if (cmd == (char *) EOF) { /* stdin closed. The connection with the terminal is gone. @@ -857,6 +846,9 @@ command_line_handler (gdb::unique_xmalloc_ptr &&rl) { ui->prompt_state = PROMPT_NEEDED; + /* Ensure the UI's line buffer is empty for the next command. */ + SCOPE_EXIT { line_buffer.clear (); }; + command_handler (cmd); if (ui->prompt_state != PROMPTED) diff --git a/gdb/linespec.c b/gdb/linespec.c index 3db35998f7e..5963823603f 100644 --- a/gdb/linespec.c +++ b/gdb/linespec.c @@ -1501,7 +1501,9 @@ decode_line_2 (struct linespec_state *self, { prompt = "> "; } - args = command_line_input (prompt, "overload-choice"); + + std::string buffer; + args = command_line_input (buffer, prompt, "overload-choice"); if (args == 0 || *args == 0) error_no_arg (_("one or more choice numbers")); diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c index 408f582ebc5..777ca19af4d 100644 --- a/gdb/mi/mi-cmd-break.c +++ b/gdb/mi/mi-cmd-break.c @@ -564,7 +564,7 @@ mi_cmd_break_commands (const char *command, char **argv, int argc) int count = 1; auto reader - = [&] () + = [&] (std::string &buffer) { const char *result = nullptr; if (count < argc) diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c index 63b18bd0f92..de7b9f4266b 100644 --- a/gdb/python/py-breakpoint.c +++ b/gdb/python/py-breakpoint.c @@ -582,7 +582,7 @@ bppy_set_commands (PyObject *self, PyObject *newvalue, void *closure) bool first = true; char *save_ptr = nullptr; auto reader - = [&] () + = [&] (std::string &buffer) { const char *result = strtok_r (first ? commands.get () : nullptr, "\n", &save_ptr); diff --git a/gdb/python/py-gdb-readline.c b/gdb/python/py-gdb-readline.c index af388d5ed72..c82f1c81ea7 100644 --- a/gdb/python/py-gdb-readline.c +++ b/gdb/python/py-gdb-readline.c @@ -38,11 +38,12 @@ gdbpy_readline_wrapper (FILE *sys_stdin, FILE *sys_stdout, { int n; const char *p = NULL; + std::string buffer; char *q; try { - p = command_line_input (prompt, "python"); + p = command_line_input (buffer, prompt, "python"); } /* Handle errors by raising Python exceptions. */ catch (const gdb_exception &except) diff --git a/gdb/python/python.c b/gdb/python/python.c index 29f2010ee8e..4aa24421dec 100644 --- a/gdb/python/python.c +++ b/gdb/python/python.c @@ -644,7 +644,7 @@ execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw) bool first = true; char *save_ptr = nullptr; auto reader - = [&] () + = [&] (std::string &buffer) { const char *result = strtok_r (first ? &arg_copy[0] : nullptr, "\n", &save_ptr); diff --git a/gdb/top.c b/gdb/top.c index e0e7e48cf7c..45e65beb3b3 100644 --- a/gdb/top.c +++ b/gdb/top.c @@ -307,8 +307,6 @@ ui::ui (FILE *instream_, FILE *outstream_, FILE *errstream_) m_gdb_stderr (new stderr_file (errstream)), m_gdb_stdlog (m_gdb_stderr) { - buffer_init (&line_buffer); - unbuffer_stream (instream_); if (ui_list == NULL) @@ -343,8 +341,6 @@ ui::~ui () delete m_gdb_stdin; delete m_gdb_stdout; delete m_gdb_stderr; - - buffer_free (&line_buffer); } /* Open file named NAME for read/write, making sure not to make it the @@ -452,11 +448,11 @@ read_command_file (FILE *stream) while (ui->instream != NULL && !feof (ui->instream)) { - const char *command; - /* Get a command-line. This calls the readline package. */ - command = command_line_input (NULL, NULL); - if (command == NULL) + std::string command_buffer; + const char *command + = command_line_input (command_buffer, nullptr, nullptr); + if (command == nullptr) break; command_handler (command); } @@ -1333,23 +1329,23 @@ gdb_safe_append_history (void) } } -/* Read one line from the command input stream `instream' into a local - static buffer. The buffer is made bigger as necessary. Returns - the address of the start of the line. +/* Read one line from the command input stream `instream'. + + CMD_LINE_BUFFER is a buffer that the function may use to store the result, if + it needs to be dynamically-allocated. Otherwise, it is unused.string - NULL is returned for end of file. + Return nullptr for end of file. This routine either uses fancy command line editing or simple input as the user has requested. */ const char * -command_line_input (const char *prompt_arg, const char *annotation_suffix) +command_line_input (std::string &cmd_line_buffer, const char *prompt_arg, + const char *annotation_suffix) { - static struct buffer cmd_line_buffer; - static int cmd_line_buffer_initialized; struct ui *ui = current_ui; const char *prompt = prompt_arg; - char *cmd; + const char *cmd; int from_tty = ui->instream == ui->stdin_stream; /* The annotation suffix must be non-NULL. */ @@ -1374,15 +1370,6 @@ command_line_input (const char *prompt_arg, const char *annotation_suffix) prompt = local_prompt; } - if (!cmd_line_buffer_initialized) - { - buffer_init (&cmd_line_buffer); - cmd_line_buffer_initialized = 1; - } - - /* Starting a new command line. */ - cmd_line_buffer.used_size = 0; - #ifdef SIGTSTP if (job_control) signal (SIGTSTP, handle_sigtstp); @@ -1422,7 +1409,7 @@ command_line_input (const char *prompt_arg, const char *annotation_suffix) rl.reset (gdb_readline_no_editing (prompt)); } - cmd = handle_line_of_input (&cmd_line_buffer, rl.get (), + cmd = handle_line_of_input (cmd_line_buffer, rl.get (), 0, annotation_suffix); if (cmd == (char *) EOF) { diff --git a/gdb/top.h b/gdb/top.h index 29778f69262..cbe50432c60 100644 --- a/gdb/top.h +++ b/gdb/top.h @@ -68,7 +68,7 @@ struct ui /* The UI's command line buffer. This is to used to accumulate input until we have a whole command line. */ - struct buffer line_buffer; + std::string line_buffer; /* The callback used by the event loop whenever an event is detected on the UI's input file descriptor. This function incrementally @@ -289,9 +289,9 @@ extern void show_commands (const char *args, int from_tty); extern void set_verbose (const char *, int, struct cmd_list_element *); -extern char *handle_line_of_input (struct buffer *cmd_line_buffer, - const char *rl, int repeat, - const char *annotation_suffix); +extern const char *handle_line_of_input (std::string &cmd_line_buffer, + const char *rl, int repeat, + const char *annotation_suffix); /* Call at startup to see if the user has requested that gdb start up quietly. */