This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing
On Wed, 2018-08-08 at 22:57 -0600, Tom Tromey wrote:
> Tom> Yeah, I'd rather do the deeper fix, because otherwise we will have an
> Tom> API that's difficult to use correctly.
>
> Tom> But if I can't get it finished soon, I'll approve this.
>
> I was very ill and so this didn't happen on quite the schedule I had hoped.
Sorry to hear you were ill, I hope you have fully recovered now.
Thanks for the fix, and no worry for the schedule : your definition of
'late schedule' is a lot 'earlier' than average expectation :).
> But, here's the patch I came up with. Tested by the buildbot. Let me
> know what you think.
I have quickly scanned the patch, I have some comments/questions
(whatever that means, seeing my superficial knowledge of gdb code basis).
This patch fixes the user after free.
There was also a second regression which was fixed by the patch
suggested in the RFA 1/2 (and the test in RFA 2/2 was checking
the regression).
Will you add the fix for the second regression (and the test)
in another commit ?
>
> One oddity I noticed is that, currently, command repetition is global,
> whereas it seems like it would be better per-ui.
>
> Tom
>
> commit d057d1227b386214d3a9527dfe61bf26e2512d69
> Author: Tom Tromey <tom@tromey.com>
> Date: Sat Jul 28 11:03:09 2018 -0600
>
> Fix use-after-free in number_or_range_parser
>
> -fsanitize=address showed a use-after-free in number_or_range_parser.
>
> The cause was that handle_line_of_input could stash the input into
> "saved_command_line", and then this could be freed by reentrant calls.
>
> This fixes the bug by locating an "outer enough" caller to hold the
> storage for the command line, and then threading "struct buffer *"
> arguments through the call stack as needed.
>
> 2018-08-08 Tom Tromey <tom@tromey.com>
>
> * top.c (read_command_file): Update.
> (command_line_input): Add cmd_line_buffer argument.
> (execute_command): Check server_command, not saved_command_line,
> to see if repeating.
> * python/python.c (execute_gdb_command): Update.
> * python/py-gdb-readline.c (gdbpy_readline_wrapper): Update.
> * python/py-breakpoint.c (bppy_set_commands): Update.
> * mi/mi-cmd-break.c (mi_cmd_break_commands): Update.
> * linespec.c (decode_line_2): Update.
> * event-top.c (handle_line_of_input): Do not return
> saved_command_line.
> * defs.h (command_line_input): Add struct buffer * argument.
> * common/buffer.h (struct auto_buffer): New.
> * cli/cli-script.h (read_command_lines_1): Add struct buffer * to
> callback type.
> * cli/cli-script.c (read_next_line): Add "storage" argument.
> (recurse_read_control_structure): Update. Use auto_buffer.
> (read_command_lines_1): Update.
> * breakpoint.c (read_uploaded_action): Update signature.
> * ada-lang.c (get_selections): Update.
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 4d0593f163..fe32581b9b 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,26 @@
> +2018-08-08 Tom Tromey <tom@tromey.com>
> +
> + * top.c (read_command_file): Update.
> + (command_line_input): Add cmd_line_buffer argument.
> + (execute_command): Check server_command, not saved_command_line,
> + to see if repeating.
> + * python/python.c (execute_gdb_command): Update.
> + * python/py-gdb-readline.c (gdbpy_readline_wrapper): Update.
> + * python/py-breakpoint.c (bppy_set_commands): Update.
> + * mi/mi-cmd-break.c (mi_cmd_break_commands): Update.
> + * linespec.c (decode_line_2): Update.
> + * event-top.c (handle_line_of_input): Do not return
> + saved_command_line.
> + * defs.h (command_line_input): Add struct buffer * argument.
> + * common/buffer.h (struct auto_buffer): New.
> + * cli/cli-script.h (read_command_lines_1): Add struct buffer * to
> + callback type.
> + * cli/cli-script.c (read_next_line): Add "storage" argument.
> + (recurse_read_control_structure): Update. Use auto_buffer.
> + (read_command_lines_1): Update.
> + * breakpoint.c (read_uploaded_action): Update signature.
> + * ada-lang.c (get_selections): Update.
> +
> 2018-08-08 Simon Marchi <simon.marchi@ericsson.com>
>
> * target.c (str_comma_list_concat_elem): Fix typo in comment.
> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index 07a0536684..c4914837f0 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -62,6 +62,7 @@
> #include "cli/cli-utils.h"
> #include "common/function-view.h"
> #include "common/byte-vector.h"
> +#include "common/buffer.h"
> #include <algorithm>
>
> /* Define whether or not the C operator '/' truncates towards zero for
> @@ -4041,7 +4042,8 @@ get_selections (int *choices, int n_choices, int max_results,
> if (prompt == NULL)
> prompt = "> ";
>
> - args = command_line_input (prompt, 0, annotation_suffix);
> + auto_buffer storage;
> + args = command_line_input (prompt, 0, annotation_suffix, &storage);
>
> if (args == NULL)
> error_no_arg (_("one or more choice numbers"));
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 8f0feaa474..0bfc8193dd 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -14679,7 +14679,7 @@ static struct uploaded_tp *this_utp;
> static int next_cmd;
>
> static char *
> -read_uploaded_action (void)
> +read_uploaded_action (struct buffer *)
> {
> char *rslt = nullptr;
>
> diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
> index 6f31a40019..dd897ea258 100644
> --- a/gdb/cli/cli-script.c
> +++ b/gdb/cli/cli-script.c
> @@ -40,14 +40,14 @@
>
> static enum command_control_type
> recurse_read_control_structure
> - (gdb::function_view<const char * ()> read_next_line_func,
> + (gdb::function_view<const char * (struct buffer *)> read_next_line_func,
> struct command_line *current_cmd,
> gdb::function_view<void (const char *)> validator);
>
> static void do_define_command (const char *comname, int from_tty,
> const counted_command_line *commands);
>
> -static char *read_next_line (void);
> +static char *read_next_line (struct buffer *);
>
> /* Level of control structure when reading. */
> static int control_level;
> @@ -880,7 +880,7 @@ user_args::insert_args (const char *line) const
> from stdin. */
>
> static char *
> -read_next_line (void)
> +read_next_line (struct buffer *storage)
> {
> struct ui *ui = current_ui;
> char *prompt_ptr, control_prompt[256];
> @@ -903,7 +903,7 @@ read_next_line (void)
> else
> prompt_ptr = NULL;
>
> - return command_line_input (prompt_ptr, from_tty, "commands");
> + return command_line_input (prompt_ptr, from_tty, "commands", storage);
> }
>
> /* Return true if CMD's name is NAME. */
> @@ -1075,9 +1075,10 @@ process_next_line (const char *p, struct command_line **command,
> obtain lines of the command. */
>
> static enum command_control_type
> -recurse_read_control_structure (gdb::function_view<const char * ()> read_next_line_func,
> - struct command_line *current_cmd,
> - gdb::function_view<void (const char *)> validator)
> +recurse_read_control_structure
> + (gdb::function_view<const char * (struct buffer *)> read_next_line_func,
> + struct command_line *current_cmd,
> + gdb::function_view<void (const char *)> validator)
> {
> enum misc_command_type val;
> enum command_control_type ret;
> @@ -1095,8 +1096,9 @@ recurse_read_control_structure (gdb::function_view<const char * ()> read_next_li
> {
> dont_repeat ();
>
> + auto_buffer storage;
> next = NULL;
> - val = process_next_line (read_next_line_func (), &next,
> + val = process_next_line (read_next_line_func (&storage), &next,
> current_cmd->control_type != python_control
> && current_cmd->control_type != guile_control
> && current_cmd->control_type != compile_control,
> @@ -1223,9 +1225,10 @@ 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<const char * ()> read_next_line_func,
> - int parse_commands,
> - gdb::function_view<void (const char *)> validator)
> +read_command_lines_1
> + (gdb::function_view<const char * (struct buffer *)> read_next_line_func,
> + int parse_commands,
> + gdb::function_view<void (const char *)> validator)
> {
> struct command_line *tail, *next;
> counted_command_line head (nullptr, command_lines_deleter ());
> @@ -1238,7 +1241,8 @@ read_command_lines_1 (gdb::function_view<const char * ()> read_next_line_func,
> while (1)
> {
> dont_repeat ();
> - val = process_next_line (read_next_line_func (), &next, parse_commands,
> + auto_buffer storage;
> + val = process_next_line (read_next_line_func (&storage), &next, parse_commands,
> validator);
>
> /* Ignore blank lines or comments. */
> diff --git a/gdb/cli/cli-script.h b/gdb/cli/cli-script.h
> index 736ebb3a7b..fecba181b5 100644
> --- a/gdb/cli/cli-script.h
> +++ b/gdb/cli/cli-script.h
> @@ -109,7 +109,7 @@ private:
> extern counted_command_line read_command_lines
> (const char *, int, int, gdb::function_view<void (const char *)>);
> extern counted_command_line read_command_lines_1
> - (gdb::function_view<const char * ()>, int,
> + (gdb::function_view<const char * (struct buffer *)>, int,
> gdb::function_view<void (const char *)>);
>
>
> diff --git a/gdb/common/buffer.h b/gdb/common/buffer.h
> index 9806fd8ad8..d93793422d 100644
> --- a/gdb/common/buffer.h
> +++ b/gdb/common/buffer.h
> @@ -65,4 +65,22 @@ void buffer_xml_printf (struct buffer *buffer, const char *format, ...)
> #define buffer_grow_str0(BUFFER,STRING) \
> buffer_grow (BUFFER, STRING, strlen (STRING) + 1)
>
> +/* A buffer that frees itself on scope exit. */
> +struct auto_buffer : public buffer
> +{
> + auto_buffer ()
> + {
> + buffer_init (this);
> + }
> +
> + ~auto_buffer ()
> + {
> + buffer_free (this);
> + }
> +
> +private:
> +
> + DISABLE_COPY_AND_ASSIGN (auto_buffer);
> +};
> +
> #endif
> diff --git a/gdb/defs.h b/gdb/defs.h
> index 4cf83f0d44..f066f9ec49 100644
> --- a/gdb/defs.h
> +++ b/gdb/defs.h
> @@ -309,7 +309,8 @@ typedef void initialize_file_ftype (void);
>
> extern char *gdb_readline_wrapper (const char *);
>
> -extern char *command_line_input (const char *, int, const char *);
> +extern char *command_line_input (const char *, int, const char *,
> + struct buffer *);
>
> extern void print_prompt (void);
>
> diff --git a/gdb/event-top.c b/gdb/event-top.c
> index 5852089f09..457488f3c6 100644
> --- a/gdb/event-top.c
> +++ b/gdb/event-top.c
> @@ -714,7 +714,12 @@ handle_line_of_input (struct buffer *cmd_line_buffer,
> for (p1 = cmd; *p1 == ' ' || *p1 == '\t'; p1++)
> ;
> if (repeat && *p1 == '\0')
> - return saved_command_line;
> + {
> + xfree (buffer_finish (cmd_line_buffer));
> + buffer_grow_str0 (cmd_line_buffer, saved_command_line);
> + cmd_line_buffer->used_size = 0;
I was somewhat surprised by used_size being set to 0.
I am not sure to understand in which case it is supposed to be
set to 0 : it is set to 0 in the first few lines of handle_line_of_input
with a comment explaining why.
I however do not understand why it is set to the string length in
the 'Do history expansion' case, and not to 0 : as far as I can see,
cmd will be returned as a full line in case of expansion ?
> + return cmd_line_buffer->buffer;
> + }
>
> /* Add command to history if appropriate. Note: lines consisting
> solely of comments are also added to the command history. This
> @@ -731,10 +736,8 @@ handle_line_of_input (struct buffer *cmd_line_buffer,
> {
> xfree (saved_command_line);
> saved_command_line = xstrdup (cmd);
> - return saved_command_line;
> }
> - else
> - return cmd;
> + return cmd;
> }
>
> /* Handle a complete line of input. This is called by the callback
> diff --git a/gdb/linespec.c b/gdb/linespec.c
> index 790ddf4740..314ff7dcdb 100644
> --- a/gdb/linespec.c
> +++ b/gdb/linespec.c
> @@ -46,6 +46,7 @@
> #include "location.h"
> #include "common/function-view.h"
> #include "common/def-vector.h"
> +#include "common/buffer.h"
> #include <algorithm>
>
> /* An enumeration of the various things a user might attempt to
> @@ -1554,7 +1555,8 @@ decode_line_2 (struct linespec_state *self,
> {
> prompt = "> ";
> }
> - args = command_line_input (prompt, 0, "overload-choice");
> + auto_buffer storage;
> + args = command_line_input (prompt, 0, "overload-choice", &storage);
>
> 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 8897117bb8..1b77f47dde 100644
> --- a/gdb/mi/mi-cmd-break.c
> +++ b/gdb/mi/mi-cmd-break.c
> @@ -494,7 +494,7 @@ mi_cmd_break_commands (const char *command, char **argv, int argc)
>
> int count = 1;
> auto reader
> - = [&] ()
> + = [&] (struct buffer *)
> {
> const char *result = nullptr;
> if (count < argc)
> diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
> index e1db674647..6baf3f0a7c 100644
> --- a/gdb/python/py-breakpoint.c
> +++ b/gdb/python/py-breakpoint.c
> @@ -530,7 +530,7 @@ bppy_set_commands (PyObject *self, PyObject *newvalue, void *closure)
> bool first = true;
> char *save_ptr = nullptr;
> auto reader
> - = [&] ()
> + = [&] (struct 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 a95be41c49..097c772d29 100644
> --- a/gdb/python/py-gdb-readline.c
> +++ b/gdb/python/py-gdb-readline.c
> @@ -38,10 +38,11 @@ gdbpy_readline_wrapper (FILE *sys_stdin, FILE *sys_stdout,
> {
> int n;
> char *p = NULL, *q;
> + auto_buffer buffer;
>
> TRY
> {
> - p = command_line_input (prompt, 0, "python");
> + p = command_line_input (prompt, 0, "python", &buffer);
> }
> /* Handle errors by raising Python exceptions. */
> CATCH (except, RETURN_MASK_ALL)
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index 20fc674f20..2c9d80cb4d 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -592,7 +592,7 @@ execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw)
> bool first = true;
> char *save_ptr = nullptr;
> auto reader
> - = [&] ()
> + = [&] (struct 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 de1a335e40..593cd133df 100644
> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -416,9 +416,10 @@ read_command_file (FILE *stream)
> while (ui->instream != NULL && !feof (ui->instream))
> {
> char *command;
> + auto_buffer storage;
>
> /* Get a command-line. This calls the readline package. */
> - command = command_line_input (NULL, 0, NULL);
> + command = command_line_input (NULL, 0, NULL, &storage);
> if (command == NULL)
> break;
> command_handler (command);
> @@ -634,7 +635,7 @@ execute_command (const char *p, int from_tty)
> /* If this command has been post-hooked, run the hook last. */
> execute_cmd_post_hook (c);
>
> - if (repeat_arguments != NULL && cmd_start == saved_command_line)
> + if (repeat_arguments != NULL && !server_command)
> {
> gdb_assert (strlen (args_pointer) >= strlen (repeat_arguments));
> strcpy (saved_command_line + (args_pointer - cmd_start),
> @@ -1155,9 +1156,9 @@ 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' into a
`current_ui->instream' ?
It is also a little bit strange that the function declares
a local struct ui*, set it to current_ui, uses it to calculate
from_tty, but then uses current_ui in the rest of the function.
> + buffer. The buffer is made bigger as necessary. Returns the
> + address of the start of the line.
IIUC, the returned value will stay valid as long as the
cmd_line_buffer is not destroyed but also only if the buffer is
not touched (e.g. no data can be added to it, as otherwise
the previously returned value might become dangling).
So, for example, the below (pseudo) code would be incorrect:
arg = command_line_input (..., cmd_buffer);
while parsing arg not finished
// here it is forbidden to touch cmd_buffer
// e.g. the below would be bad:
some_other_arg = command_line_input (..., cmd_buffer);
So, I am wondering what kind of usage of this function
will make the buffer bigger. Maybe worth pointing at the
above risk then ?
>
> NULL is returned for end of file.
>
> @@ -1170,10 +1171,9 @@ gdb_safe_append_history (void)
>
> char *
> command_line_input (const char *prompt_arg, int repeat,
> - const char *annotation_suffix)
> + const char *annotation_suffix,
> + struct buffer *cmd_line_buffer)
> {
> - static struct buffer cmd_line_buffer;
> - static int cmd_line_buffer_initialized;
> struct ui *ui = current_ui;
> const char *prompt = prompt_arg;
> char *cmd;
> @@ -1201,14 +1201,8 @@ command_line_input (const char *prompt_arg, int repeat,
> 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;
> + cmd_line_buffer->used_size = 0;
>
> #ifdef SIGTSTP
> if (job_control)
> @@ -1254,7 +1248,7 @@ command_line_input (const char *prompt_arg, int repeat,
> rl = gdb_readline_no_editing (prompt);
> }
>
> - cmd = handle_line_of_input (&cmd_line_buffer, rl,
> + cmd = handle_line_of_input (cmd_line_buffer, rl,
> repeat, annotation_suffix);
> if (cmd == (char *) EOF)
> {