* RFA Fix regressions for multi breakpoints command line setting/clearing @ 2018-08-02 21:26 Philippe Waroquiers 2018-08-02 21:26 ` [RFA 2/2] Modify gdb.base/commands.exp to test multi breakpoint command setting/clearing Philippe Waroquiers 2018-08-02 21:26 ` [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing Philippe Waroquiers 0 siblings, 2 replies; 20+ messages in thread From: Philippe Waroquiers @ 2018-08-02 21:26 UTC (permalink / raw) To: gdb-patches Fix regressions for multi breakpoints command line setting/clearing Fix done in breakpoint.c gdb.base/commands.exp changed to reproduce/test the problems. Note that these regressions appeared in GDB 8.1, e.g. GNU gdb (GDB) 8.1 ... (gdb) command 1 2 Type commands for breakpoint(s) 1 2, one per line. End with a line saying just "end". >p 987654321 >end No breakpoint number 54321. ... (gdb) command 1 2 Type commands for breakpoint(s) 1 2, one per line. End with a line saying just "end". >end Type commands for breakpoint(s) 1 2, one per line. End with a line saying just "end". >end (gdb) So, wondering if this fix should not be merged back to 8.2 (and/or in 8.1.2) ? ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFA 2/2] Modify gdb.base/commands.exp to test multi breakpoint command setting/clearing. 2018-08-02 21:26 RFA Fix regressions for multi breakpoints command line setting/clearing Philippe Waroquiers @ 2018-08-02 21:26 ` Philippe Waroquiers 2018-08-16 15:54 ` Pedro Alves 2018-08-02 21:26 ` [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing Philippe Waroquiers 1 sibling, 1 reply; 20+ messages in thread From: Philippe Waroquiers @ 2018-08-02 21:26 UTC (permalink / raw) To: gdb-patches; +Cc: Philippe Waroquiers gdb/testsuite/ChangeLog 2018-08-02 Philippe Waroquiers <philippe.waroquiers@skynet.be> * gdb.base/commands.exp: Test multi breakpoints setting and clearing. --- gdb/testsuite/gdb.base/commands.exp | 44 +++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/gdb/testsuite/gdb.base/commands.exp b/gdb/testsuite/gdb.base/commands.exp index 259b89b803..57d9348244 100644 --- a/gdb/testsuite/gdb.base/commands.exp +++ b/gdb/testsuite/gdb.base/commands.exp @@ -281,6 +281,49 @@ proc_with_prefix breakpoint_command_test {} { gdb_test "print value" " = 5" } +# Test clearing the commands of several breakpoints with one single "end". +# As this test uses breakpoint numbers, we better run it first to ensure +# the breakpoint numbers are not changing if other tests are added/changed +# so that breakpoint numbers are also changed. +proc_with_prefix run_me_first_breakpoint_clear_command_test {} { + # The below creates breakpoint nr 1. + runto_or_return factorial + + set any "\[^\r\n\]*" + delete_breakpoints + gdb_test "break factorial" "Breakpoint.*at.*" + gdb_test "break main" "Breakpoint.*at.*" + gdb_test \ + [multi_line_input \ + {commands 2 3} \ + { print 1234321} \ + {end}] \ + "End with.*" \ + "commands" + gdb_test "info breakpoints" \ + [multi_line \ + "${any}What${any}" \ + "${any}in factorial${any}" \ + "${any}print 1234321${any}" \ + "${any}in main${any}" \ + "${any}print 1234321${any}" \ + ] \ + "check print 1234321 is there." + gdb_test \ + [multi_line_input \ + {commands 2 3} \ + {end}] \ + "End with.*" \ + "commands" + gdb_test "info breakpoints" \ + [multi_line \ + "${any}What${any}" \ + "${any}in factorial${any}" \ + "${any}in main${any}" \ + ] \ + "check print 1234321 is not there anymore." + } + # Test a simple user defined command (with arguments) proc_with_prefix user_defined_command_test {} { global valnum_re @@ -1125,6 +1168,7 @@ proc_with_prefix backslash_in_multi_line_command_test {} { gdb_test "print 1" "" "run command" } +run_me_first_breakpoint_clear_command_test gdbvar_simple_if_test gdbvar_simple_while_test gdbvar_complex_if_while_test -- 2.18.0 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFA 2/2] Modify gdb.base/commands.exp to test multi breakpoint command setting/clearing. 2018-08-02 21:26 ` [RFA 2/2] Modify gdb.base/commands.exp to test multi breakpoint command setting/clearing Philippe Waroquiers @ 2018-08-16 15:54 ` Pedro Alves 0 siblings, 0 replies; 20+ messages in thread From: Pedro Alves @ 2018-08-16 15:54 UTC (permalink / raw) To: Philippe Waroquiers, gdb-patches Hi Philippe, On 08/02/2018 10:26 PM, Philippe Waroquiers wrote: > gdb/testsuite/ChangeLog > 2018-08-02 Philippe Waroquiers <philippe.waroquiers@skynet.be> > > * gdb.base/commands.exp: Test multi breakpoints setting > and clearing. > --- > gdb/testsuite/gdb.base/commands.exp | 44 +++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/gdb/testsuite/gdb.base/commands.exp b/gdb/testsuite/gdb.base/commands.exp > index 259b89b803..57d9348244 100644 > --- a/gdb/testsuite/gdb.base/commands.exp > +++ b/gdb/testsuite/gdb.base/commands.exp > @@ -281,6 +281,49 @@ proc_with_prefix breakpoint_command_test {} { > gdb_test "print value" " = 5" > } > > +# Test clearing the commands of several breakpoints with one single "end". > +# As this test uses breakpoint numbers, we better run it first to ensure > +# the breakpoint numbers are not changing if other tests are added/changed > +# so that breakpoint numbers are also changed. The "are not changing" here gave me pause. I think you meant "do not change". I'd suggest: # Test clearing the commands of several breakpoints with one single "end". # As this test hardcodes breakpoint numbers, we better run it first to ensure # the breakpoint numbers remain stable, as other tests can also create # breakpoints. But, I think you could simply remove this requirement. You can for example get the first breakpoint number with: gdb_test "break factorial" "Breakpoint.*at.*" set bpnum [get_integer_valueof "\$bpnum" 0] Or in a single shot: set bpnum 0 gdb_test_multiple "break main" "set breakpoint on main" { -re "Breakpoint ($decimal) at .*$gdb_prompt $" { set bpnum $expect_out(1,string) } } > +proc_with_prefix run_me_first_breakpoint_clear_command_test {} { > + # The below creates breakpoint nr 1. > + runto_or_return factorial > + > + set any "\[^\r\n\]*" > + delete_breakpoints > + gdb_test "break factorial" "Breakpoint.*at.*" > + gdb_test "break main" "Breakpoint.*at.*" > + gdb_test \ > + [multi_line_input \ > + {commands 2 3} \ > + { print 1234321} \ > + {end}] \ > + "End with.*" \ > + "commands" > + gdb_test "info breakpoints" \ > + [multi_line \ > + "${any}What${any}" \ > + "${any}in factorial${any}" \ > + "${any}print 1234321${any}" \ > + "${any}in main${any}" \ > + "${any}print 1234321${any}" \ > + ] \ > + "check print 1234321 is there." No period at end of test name. While at it, "check" is redundant, since all tests are checks. > + gdb_test \ > + [multi_line_input \ > + {commands 2 3} \ > + {end}] \ > + "End with.*" \ > + "commands" > + gdb_test "info breakpoints" \ > + [multi_line \ > + "${any}What${any}" \ > + "${any}in factorial${any}" \ > + "${any}in main${any}" \ > + ] \ > + "check print 1234321 is not there anymore." Ditto. Also, you have a duplicated test name above: the "commands" tests: https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique > + } > + > # Test a simple user defined command (with arguments) > proc_with_prefix user_defined_command_test {} { > global valnum_re > @@ -1125,6 +1168,7 @@ proc_with_prefix backslash_in_multi_line_command_test {} { > gdb_test "print 1" "" "run command" > } > > +run_me_first_breakpoint_clear_command_test > gdbvar_simple_if_test > gdbvar_simple_while_test > gdbvar_complex_if_while_test > Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing 2018-08-02 21:26 RFA Fix regressions for multi breakpoints command line setting/clearing Philippe Waroquiers 2018-08-02 21:26 ` [RFA 2/2] Modify gdb.base/commands.exp to test multi breakpoint command setting/clearing Philippe Waroquiers @ 2018-08-02 21:26 ` Philippe Waroquiers 2018-08-03 18:29 ` Tom Tromey 2018-08-21 18:01 ` Tom Tromey 1 sibling, 2 replies; 20+ messages in thread From: Philippe Waroquiers @ 2018-08-02 21:26 UTC (permalink / raw) To: gdb-patches; +Cc: Philippe Waroquiers When setting commands for several breakpoints (such as with 'commands 1 2'), the '1 2' is passed to commands_command_1 as const char* arg. This arg can however be freed, as this is in a just read input line that might be freed by the call to read_command_lines. This patch fixes the problem by ensuring that arg is always a locally allocated string managed via the std::string new_arg. Note that this was the logic before the regression was introduced : the use after free was introduced when (partially) undoing the patch done by Pedro in 896b6bda6904765f36692d76a37b99c0412ca9ae. Note that such problem could also (or should?) be fixed by reworking the way read_command_lines manages the memory of input lines, so as to solve this globally, and not at all places where possibly a 'being handled' line of input might be re-allocated. Tom is looking at this, but in the meantime, this patch just goes back to the previous way to avoid the error (and be able to have the regression tests for the functional regression working). Without the patch, the test fails the following way: ... commands 2 3 Type commands for breakpoint(s) 2 3, one per line. End with a line saying just "end". > print 1234321 >end No breakpoint number 4321. ... (and under valgrind, the above reports access to freed memory). breakpoint.c is also modified to fix the regression introduced when clearing commands of several breakpoint by giving an empty list of commands, by just typing "end". GDB should read an empty list of command once, but it reads it for each breakpoint, as an empty list of command is NULL. gdb/ChangeLog 2018-08-02 Philippe Waroquiers <philippe.waroquiers@skynet.be> * breakpoint.c (commands_command_1): New boolean cmd_read to detect cmd was already read. Always allocate a new_arg c_str to avoid accessing arg after some calls to read_command_line as this can free arg memory. --- gdb/breakpoint.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 6b6e1f6c25..7761bdb496 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -1219,9 +1219,17 @@ commands_command_1 (const char *arg, int from_tty, struct command_line *control) { counted_command_line cmd; + /* cmd_read will be true once we have read cmd. Note that cmd might still be + NULL after the call to read_command_lines if the user provides an empty + list of command by just typing "end". */ + bool cmd_read = false; std::string new_arg; + /* arg might be an input line that might be released when reading + new input lines for the list of commands. So, build a new arg + to keep the input alive during the map_breakpoint_numbers call, + even if the new arg is just a copy of arg. */ if (arg == NULL || !*arg) { if (breakpoint_count - prev_breakpoint_count > 1) @@ -1231,12 +1239,18 @@ commands_command_1 (const char *arg, int from_tty, new_arg = string_printf ("%d", breakpoint_count); arg = new_arg.c_str (); } + else + { + new_arg = arg; + arg = new_arg.c_str (); + } map_breakpoint_numbers (arg, [&] (breakpoint *b) { - if (cmd == NULL) + if (!cmd_read) { + gdb_assert (cmd == NULL); if (control != NULL) cmd = control->body_list_0; else @@ -1256,6 +1270,7 @@ commands_command_1 (const char *arg, int from_tty, cmd = read_command_lines (str.c_str (), from_tty, 1, validator); } + cmd_read = true; } /* If a breakpoint was on the list more than once, we don't need to -- 2.18.0 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing 2018-08-02 21:26 ` [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing Philippe Waroquiers @ 2018-08-03 18:29 ` Tom Tromey 2018-08-09 4:57 ` Tom Tromey 2018-08-21 18:01 ` Tom Tromey 1 sibling, 1 reply; 20+ messages in thread From: Tom Tromey @ 2018-08-03 18:29 UTC (permalink / raw) To: Philippe Waroquiers; +Cc: gdb-patches >>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes: Philippe> Note that such problem could also (or should?) be fixed by reworking Philippe> the way read_command_lines manages the memory of input lines, so as Philippe> to solve this globally, and not at all places where possibly a Philippe> 'being handled' line of input might be re-allocated. Philippe> Tom is looking at this, but in the meantime, Philippe> this patch just goes back to the previous way to avoid the Philippe> error (and be able to have the regression tests for the Philippe> functional regression working). Yeah, I'd rather do the deeper fix, because otherwise we will have an API that's difficult to use correctly. But if I can't get it finished soon, I'll approve this. Tom ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing 2018-08-03 18:29 ` Tom Tromey @ 2018-08-09 4:57 ` Tom Tromey 2018-08-09 20:20 ` Philippe Waroquiers 0 siblings, 1 reply; 20+ messages in thread From: Tom Tromey @ 2018-08-09 4:57 UTC (permalink / raw) To: Tom Tromey; +Cc: Philippe Waroquiers, gdb-patches 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. But, here's the patch I came up with. Tested by the buildbot. Let me know what you think. 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; + 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 + buffer. The buffer is made bigger as necessary. Returns the + address of the start of the line. 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) { ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing 2018-08-09 4:57 ` Tom Tromey @ 2018-08-09 20:20 ` Philippe Waroquiers 2018-08-10 0:35 ` Tom Tromey 0 siblings, 1 reply; 20+ messages in thread From: Philippe Waroquiers @ 2018-08-09 20:20 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches 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) > { ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing 2018-08-09 20:20 ` Philippe Waroquiers @ 2018-08-10 0:35 ` Tom Tromey 2018-08-10 3:05 ` Tom Tromey 0 siblings, 1 reply; 20+ messages in thread From: Tom Tromey @ 2018-08-10 0:35 UTC (permalink / raw) To: Philippe Waroquiers; +Cc: Tom Tromey, gdb-patches >>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes: Philippe> There was also a second regression which was fixed by the patch Philippe> suggested in the RFA 1/2 (and the test in RFA 2/2 was checking Philippe> the regression). Philippe> Will you add the fix for the second regression (and the test) Philippe> in another commit ? Yeah, I can rebase your patches on top of this one. >> 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; Philippe> I was somewhat surprised by used_size being set to 0. Philippe> I am not sure to understand in which case it is supposed to be Philippe> set to 0 : it is set to 0 in the first few lines of handle_line_of_input Philippe> with a comment explaining why. Philippe> I however do not understand why it is set to the string length in Philippe> the 'Do history expansion' case, and not to 0 : as far as I can see, Philippe> cmd will be returned as a full line in case of expansion ? Thanks for noticing this. I suspect the history case (which doesn't seem to be tested...) is just wrong and should also set the used size to 0. I believe the idea being clear the used size is that the buffer still owns the string, but if the buffer is reused then the string is considered ok to overwrite. Which, admittedly, seems odd to me, but this patch seemed like the least intrusive way to avoid the use-after-free... I'll see if I can come up with a test case here. Philippe> IIUC, the returned value will stay valid as long as the Philippe> cmd_line_buffer is not destroyed but also only if the buffer is Philippe> not touched (e.g. no data can be added to it, as otherwise Philippe> the previously returned value might become dangling). Philippe> So, for example, the below (pseudo) code would be incorrect: Philippe> arg = command_line_input (..., cmd_buffer); Philippe> while parsing arg not finished Philippe> // here it is forbidden to touch cmd_buffer Philippe> // e.g. the below would be bad: Philippe> some_other_arg = command_line_input (..., cmd_buffer); Philippe> So, I am wondering what kind of usage of this function Philippe> will make the buffer bigger. Maybe worth pointing at the Philippe> above risk then ? I can add a comment somewhere. I don't really understand why this code works the way it does. It seems unusually convoluted. At the same time, I didn't want to completely tear it apart, since it's not clear that's a good use of one's time. Tom ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing 2018-08-10 0:35 ` Tom Tromey @ 2018-08-10 3:05 ` Tom Tromey 2018-08-10 3:13 ` Tom Tromey 0 siblings, 1 reply; 20+ messages in thread From: Tom Tromey @ 2018-08-10 3:05 UTC (permalink / raw) To: Tom Tromey; +Cc: Philippe Waroquiers, gdb-patches >>>>> "Tom" == Tom Tromey <tom@tromey.com> writes: >>> 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; Philippe> I was somewhat surprised by used_size being set to 0. Philippe> I am not sure to understand in which case it is supposed to be Philippe> set to 0 : it is set to 0 in the first few lines of handle_line_of_input Philippe> with a comment explaining why. Philippe> I however do not understand why it is set to the string length in Philippe> the 'Do history expansion' case, and not to 0 : as far as I can see, Philippe> cmd will be returned as a full line in case of expansion ? Tom> Thanks for noticing this. Tom> I suspect the history case (which doesn't seem to be tested...) is just Tom> wrong and should also set the used size to 0. I think we both misread this, because the history expansion case does: xfree (buffer_finish (cmd_line_buffer)); cmd_line_buffer->buffer = history_value; cmd_line_buffer->buffer_size = len + 1; ... which does not change used_size. So, I think all the cases are behaving properly. Tom> I'll see if I can come up with a test case here. I'm still going to write a history expansion test, because there should be at least one. Philippe> So, I am wondering what kind of usage of this function Philippe> will make the buffer bigger. Maybe worth pointing at the Philippe> above risk then ? Tom> I can add a comment somewhere. Actually I don't think there is a problem here. The growing case, I think, can only happen with continuation lines. Otherwise the buffer is stable -- I think, provided we don't reenter the command loop -- for the duration of the command. Tom ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing 2018-08-10 3:05 ` Tom Tromey @ 2018-08-10 3:13 ` Tom Tromey 2018-08-10 16:07 ` Tom Tromey 2018-08-14 15:02 ` Pedro Alves 0 siblings, 2 replies; 20+ messages in thread From: Tom Tromey @ 2018-08-10 3:13 UTC (permalink / raw) To: Tom Tromey; +Cc: Philippe Waroquiers, gdb-patches Tom> I'm still going to write a history expansion test, because there should Tom> be at least one. Here's a patch with a test. Tom commit aea20ede47dfca4156f19c0b41e3a1b11e724c20 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. Due to some review comments by Philippe, I noticed that there were no existing tests for history expansion, so this patch adds one. gdb/ChangeLog 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. gdb/testsuite/ChangeLog 2018-08-09 Tom Tromey <tom@tromey.com> * gdb.base/history.exp: New file. diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 338813ab37..57747b4527 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-09 Jim Wilson <jimw@sifive.com> * Makefile.in (ALL_TARGET_OBS): Add riscv-linux-tdep.c. 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; + 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/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index bd3c3bfec5..1e9d767f64 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,7 @@ +2018-08-09 Tom Tromey <tom@tromey.com> + + * gdb.base/history.exp: New file. + 2018-08-09 Andrew Burgess <andrew.burgess@embecosm.com> * gdb.base/vla-optimized-out.exp: Add new test. diff --git a/gdb/testsuite/gdb.base/history.exp b/gdb/testsuite/gdb.base/history.exp new file mode 100644 index 0000000000..bdd65a7cc7 --- /dev/null +++ b/gdb/testsuite/gdb.base/history.exp @@ -0,0 +1,32 @@ +# Copyright 2018 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +# This file is part of the gdb testsuite. + +# Test history expansion. + +gdb_start + +# These tests require readline support. +if { ![readline_is_used] } { + unsupported "readline isn't used." + return -1 +} + +gdb_test_no_output "set history expansion on" + +gdb_test "print 23" " = 23" + +gdb_test "!!" " = 23" "history expansion" 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 + buffer. The buffer is made bigger as necessary. Returns the + address of the start of the line. 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) { ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing 2018-08-10 3:13 ` Tom Tromey @ 2018-08-10 16:07 ` Tom Tromey 2018-08-11 20:38 ` Tom Tromey 2018-08-14 15:02 ` Pedro Alves 1 sibling, 1 reply; 20+ messages in thread From: Tom Tromey @ 2018-08-10 16:07 UTC (permalink / raw) To: Tom Tromey; +Cc: Philippe Waroquiers, gdb-patches >>>>> "Tom" == Tom Tromey <tom@tromey.com> writes: Tom> Here's a patch with a test. Further testing shows that this regresses some MI tests when compiled with -fsanitize=address. I'll look into that soon. Tom ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing 2018-08-10 16:07 ` Tom Tromey @ 2018-08-11 20:38 ` Tom Tromey 2018-08-13 21:32 ` Philippe Waroquiers 0 siblings, 1 reply; 20+ messages in thread From: Tom Tromey @ 2018-08-11 20:38 UTC (permalink / raw) To: Tom Tromey; +Cc: Philippe Waroquiers, gdb-patches Tom> Further testing shows that this regresses some MI tests when compiled Tom> with -fsanitize=address. Here's another, more convoluted, variant that seems to fix all the problems. Let me know what you think. I'm eager to be rid of this patch :) Tom commit a501b595984e2ee52a777c9afb062fd224784163 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. Due to some review comments by Philippe, I noticed that there were no existing tests for history expansion, so this patch adds one. gdb/ChangeLog 2018-08-10 Tom Tromey <tom@tromey.com> * tui/tui-stack.c: Include gdbcmd.h. * top.h (execute_command): Don't declare. (handle_line_of_input): Update. * reverse.c: Include gdbcmd.h. * mi/mi-cmd-env.c: Include gdbcmd.h. * main.c: Include gdbcmd.h. * gdbcmd.h (execute_command): Add can_repeat argument. (execute_command): New overload. * event-top.h (command_handler): Add can_repeat argument. * cli/cli-interp.c: Include gdbcmd.h. * top.c (read_command_file): Update. (command_line_input): Add cmd_line_buffer argument. Change type of repeat argument. (execute_command): Add can_repeat argument. * 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. Change type of repeat argument. (command_handler): Add can_repeat argument. (command_line_handler): Update. * defs.h (command_line_input): Add struct buffer * argument. Change type of repeat 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. (read_next_line): Update. * breakpoint.c (read_uploaded_action): Update signature. * ada-lang.c (get_selections): Update. gdb/testsuite/ChangeLog 2018-08-09 Tom Tromey <tom@tromey.com> * gdb.base/history.exp: New file. diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 8559185eb8..9d4c010714 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,40 @@ +2018-08-10 Tom Tromey <tom@tromey.com> + + * tui/tui-stack.c: Include gdbcmd.h. + * top.h (execute_command): Don't declare. + (handle_line_of_input): Update. + * reverse.c: Include gdbcmd.h. + * mi/mi-cmd-env.c: Include gdbcmd.h. + * main.c: Include gdbcmd.h. + * gdbcmd.h (execute_command): Add can_repeat argument. + (execute_command): New overload. + * event-top.h (command_handler): Add can_repeat argument. + * cli/cli-interp.c: Include gdbcmd.h. + * top.c (read_command_file): Update. + (command_line_input): Add cmd_line_buffer argument. Change type + of repeat argument. + (execute_command): Add can_repeat argument. + * 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. Change type of repeat argument. + (command_handler): Add can_repeat argument. + (command_line_handler): Update. + * defs.h (command_line_input): Add struct buffer * argument. + Change type of repeat 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. + (read_next_line): Update. + * breakpoint.c (read_uploaded_action): Update signature. + * ada-lang.c (get_selections): Update. + 2018-08-10 Simon Marchi <simon.marchi@ericsson.com> * nat/linux-osdata.c (commandline_from_pid): Replace xstrprintf 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-interp.c b/gdb/cli/cli-interp.c index 2aa41d6c8b..8deb308ef8 100644 --- a/gdb/cli/cli-interp.c +++ b/gdb/cli/cli-interp.c @@ -30,6 +30,7 @@ #include "gdbthread.h" #include "thread-fsm.h" #include "inferior.h" +#include "gdbcmd.h" cli_interp_base::cli_interp_base (const char *name) : interp (name) diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c index 6f31a40019..96a64dcb53 100644 --- a/gdb/cli/cli-script.c +++ b/gdb/cli/cli-script.c @@ -33,6 +33,7 @@ #include "interps.h" #include "compile/compile.h" #include "common/gdb_string_view.h" +#include "gdbcmd.h" #include <vector> @@ -40,14 +41,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 +881,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 +904,9 @@ read_next_line (void) else prompt_ptr = NULL; - return command_line_input (prompt_ptr, from_tty, "commands"); + bool can_repeat = false; + return command_line_input (prompt_ptr, from_tty ? &can_repeat : nullptr, + "commands", storage); } /* Return true if CMD's name is NAME. */ @@ -1075,9 +1078,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 +1099,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 +1228,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 +1244,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..7d4f75da0f 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 *, bool *, const char *, + struct buffer *); extern void print_prompt (void); diff --git a/gdb/event-top.c b/gdb/event-top.c index 5852089f09..b2abb6aa62 100644 --- a/gdb/event-top.c +++ b/gdb/event-top.c @@ -565,7 +565,7 @@ async_disable_stdin (void) a whole command. */ void -command_handler (const char *command) +command_handler (const char *command, bool can_repeat) { struct ui *ui = current_ui; const char *c; @@ -580,7 +580,7 @@ command_handler (const char *command) ; if (c[0] != '#') { - execute_command (command, ui->instream == ui->stdin_stream); + execute_command (command, ui->instream == ui->stdin_stream, can_repeat); /* Do any commands attached to breakpoint we stopped at. */ bpstat_do_actions (); @@ -631,19 +631,24 @@ command_line_append_input_line (struct buffer *cmd_line_buffer, char *rl) Returns EOF on end of file. - If REPEAT, handle command repetitions: + REPEAT is both an input and output flag. + If REPEAT is NULL, do not handle command repetitions. + Otherwise (if it is not NULL): + + - If the input can possibly be repeated (basically, no "server" + prefix), then *REPEAT is set to true. - If the input command line is NOT empty, the command returned is copied into the global 'saved_command_line' var so that it can be repeated later. - - OTOH, if the input command line IS empty, return the previously - saved command instead of the empty input line. + - OTOH, if the input command line IS empty, return a copy the + previously saved command instead of the empty input line. */ char * handle_line_of_input (struct buffer *cmd_line_buffer, - char *rl, int repeat, const char *annotation_suffix) + char *rl, bool *repeat, const char *annotation_suffix) { struct ui *ui = current_ui; int from_tty = ui->instream == ui->stdin_stream; @@ -713,8 +718,14 @@ handle_line_of_input (struct buffer *cmd_line_buffer, previous command, return the previously saved command. */ for (p1 = cmd; *p1 == ' ' || *p1 == '\t'; p1++) ; - if (repeat && *p1 == '\0') - return saved_command_line; + if (repeat != nullptr && *p1 == '\0') + { + *repeat = true; + xfree (buffer_finish (cmd_line_buffer)); + buffer_grow_str0 (cmd_line_buffer, saved_command_line); + cmd_line_buffer->used_size = 0; + 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 @@ -727,14 +738,13 @@ handle_line_of_input (struct buffer *cmd_line_buffer, gdb_add_history (cmd); /* Save into global buffer if appropriate. */ - if (repeat) + if (repeat != nullptr) { + *repeat = true; 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 @@ -751,8 +761,9 @@ command_line_handler (char *rl) struct buffer *line_buffer = get_command_line_buffer (); struct ui *ui = current_ui; char *cmd; + bool can_repeat = false; - cmd = handle_line_of_input (line_buffer, rl, 1, "prompt"); + cmd = handle_line_of_input (line_buffer, rl, &can_repeat, "prompt"); if (cmd == (char *) EOF) { /* stdin closed. The connection with the terminal is gone. @@ -771,7 +782,7 @@ command_line_handler (char *rl) { ui->prompt_state = PROMPT_NEEDED; - command_handler (cmd); + command_handler (cmd, can_repeat); if (ui->prompt_state != PROMPTED) display_gdb_prompt (0); diff --git a/gdb/event-top.h b/gdb/event-top.h index a77303e5c3..1ff1d5da33 100644 --- a/gdb/event-top.h +++ b/gdb/event-top.h @@ -36,7 +36,7 @@ extern void async_init_signals (void); extern void change_line_handler (int); extern void command_line_handler (char *rl); -extern void command_handler (const char *command); +extern void command_handler (const char *command, bool can_repeat); #ifdef SIGTSTP extern void handle_sigtstp (int sig); diff --git a/gdb/gdbcmd.h b/gdb/gdbcmd.h index b675ae8618..cbb42249d0 100644 --- a/gdb/gdbcmd.h +++ b/gdb/gdbcmd.h @@ -132,7 +132,14 @@ extern struct cmd_list_element *showchecklist; extern struct cmd_list_element *save_cmdlist; -extern void execute_command (const char *, int); +extern void execute_command (const char *, int, bool); + +static inline void +execute_command (const char *arg, int from_tty) +{ + return execute_command (arg, from_tty, false); +} + extern std::string execute_command_to_string (const char *p, int from_tty); extern void print_command_line (struct command_line *, unsigned int, 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/main.c b/gdb/main.c index e925128a42..f8fd23e015 100644 --- a/gdb/main.c +++ b/gdb/main.c @@ -24,6 +24,7 @@ #include "symfile.h" #include "gdbcore.h" #include "getopt.h" +#include "gdbcmd.h" #include <sys/types.h> #include <sys/stat.h> 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/mi/mi-cmd-env.c b/gdb/mi/mi-cmd-env.c index 2f5b803d5b..a532bf9238 100644 --- a/gdb/mi/mi-cmd-env.c +++ b/gdb/mi/mi-cmd-env.c @@ -32,6 +32,7 @@ #include "top.h" #include <sys/stat.h> #include "source.h" +#include "gdbcmd.h" static const char path_var_name[] = "PATH"; static char *orig_path = NULL; diff --git a/gdb/nto-tdep.c b/gdb/nto-tdep.c index 82a4fcbbb9..0feaf47491 100644 --- a/gdb/nto-tdep.c +++ b/gdb/nto-tdep.c @@ -33,6 +33,7 @@ #include "objfiles.h" #include "source.h" #include "common/pathstuff.h" +#include "gdbcmd.h" #define QNX_NOTE_NAME "QNX" #define QNX_INFO_SECT_NAME "QNX_info" 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..1f1ae22bdd 100644 --- a/gdb/python/py-gdb-readline.c +++ b/gdb/python/py-gdb-readline.c @@ -21,6 +21,7 @@ #include "python-internal.h" #include "top.h" #include "cli/cli-utils.h" +#include "common/buffer.h" /* Readline function suitable for PyOS_ReadlineFunctionPointer, which is used for Python's interactive parser and raw_input. In both @@ -38,10 +39,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, nullptr, "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/reverse.c b/gdb/reverse.c index bd9cd20964..a2dd282b40 100644 --- a/gdb/reverse.c +++ b/gdb/reverse.c @@ -26,6 +26,7 @@ #include "inferior.h" #include "infrun.h" #include "regcache.h" +#include "gdbcmd.h" /* User interface: reverse-step, reverse-next etc. */ diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index bd3c3bfec5..1e9d767f64 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,7 @@ +2018-08-09 Tom Tromey <tom@tromey.com> + + * gdb.base/history.exp: New file. + 2018-08-09 Andrew Burgess <andrew.burgess@embecosm.com> * gdb.base/vla-optimized-out.exp: Add new test. diff --git a/gdb/testsuite/gdb.base/history.exp b/gdb/testsuite/gdb.base/history.exp new file mode 100644 index 0000000000..bdd65a7cc7 --- /dev/null +++ b/gdb/testsuite/gdb.base/history.exp @@ -0,0 +1,32 @@ +# Copyright 2018 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +# This file is part of the gdb testsuite. + +# Test history expansion. + +gdb_start + +# These tests require readline support. +if { ![readline_is_used] } { + unsupported "readline isn't used." + return -1 +} + +gdb_test_no_output "set history expansion on" + +gdb_test "print 23" " = 23" + +gdb_test "!!" " = 23" "history expansion" diff --git a/gdb/top.c b/gdb/top.c index de1a335e40..d2b52d1926 100644 --- a/gdb/top.c +++ b/gdb/top.c @@ -416,12 +416,13 @@ 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); + command_handler (command, false); } } \f @@ -537,7 +538,7 @@ set_repeat_arguments (const char *args) Pass FROM_TTY as second argument to the defining function. */ void -execute_command (const char *p, int from_tty) +execute_command (const char *p, int from_tty, bool can_repeat) { struct cleanup *cleanup_if_error; struct cmd_list_element *c; @@ -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 && can_repeat) { 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 + buffer. The buffer is made bigger as necessary. Returns the + address of the start of the line. NULL is returned for end of file. @@ -1169,11 +1170,10 @@ gdb_safe_append_history (void) as the user has requested. */ char * -command_line_input (const char *prompt_arg, int repeat, - const char *annotation_suffix) +command_line_input (const char *prompt_arg, bool *can_repeat, + 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,8 +1248,8 @@ command_line_input (const char *prompt_arg, int repeat, rl = gdb_readline_no_editing (prompt); } - cmd = handle_line_of_input (&cmd_line_buffer, rl, - repeat, annotation_suffix); + cmd = handle_line_of_input (cmd_line_buffer, rl, + can_repeat, annotation_suffix); if (cmd == (char *) EOF) { cmd = NULL; diff --git a/gdb/top.h b/gdb/top.h index b34defa1f2..9a82d85d52 100644 --- a/gdb/top.h +++ b/gdb/top.h @@ -238,7 +238,6 @@ extern int quit_confirm (void); extern void quit_force (int *, int); extern void quit_command (const char *, int); extern void quit_cover (void); -extern void execute_command (const char *, int); /* If the interpreter is in sync mode (we're running a user command's list, running command hooks or similars), and we just ran a @@ -297,7 +296,7 @@ extern void show_history (const char *, int); extern void set_verbose (const char *, int, struct cmd_list_element *); extern char *handle_line_of_input (struct buffer *cmd_line_buffer, - char *rl, int repeat, + char *rl, bool *can_repeat, const char *annotation_suffix); #endif diff --git a/gdb/tui/tui-stack.c b/gdb/tui/tui-stack.c index 07767bbb82..f5733fce13 100644 --- a/gdb/tui/tui-stack.c +++ b/gdb/tui/tui-stack.c @@ -28,6 +28,7 @@ #include "target.h" #include "top.h" #include "gdb-demangle.h" +#include "gdbcmd.h" #include "source.h" #include "tui/tui.h" #include "tui/tui-data.h" ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing 2018-08-11 20:38 ` Tom Tromey @ 2018-08-13 21:32 ` Philippe Waroquiers 0 siblings, 0 replies; 20+ messages in thread From: Philippe Waroquiers @ 2018-08-13 21:32 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On Sat, 2018-08-11 at 14:38 -0600, Tom Tromey wrote: > Tom> Further testing shows that this regresses some MI tests when compiled > Tom> with -fsanitize=address. > > Here's another, more convoluted, variant that seems to fix all the > problems. > > Let me know what you think. I'm eager to be rid of this patch :) Yes, that is understandable, the patch starts to be impressive :). I cannot really give any comment about the patch approach, as all the command line handling/repeating/... is rather mysterious to me, and it is not very clear to me which problem in MI had to be solved. But find below some minor comments or questions ... Thanks Philippe ... > diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c > index 2aa41d6c8b..8deb308ef8 100644 > --- a/gdb/cli/cli-interp.c > +++ b/gdb/cli/cli-interp.c > @@ -30,6 +30,7 @@ > #include "gdbthread.h" > #include "thread-fsm.h" > #include "inferior.h" > +#include "gdbcmd.h" As I understand, this is needed to have the new overload execute_command. Why is this new overload not defined in top.h ? (where the first execute_command was defined ?) Wouldn't that avoid to have to include gdbcmd.h in many places ? So, unclear why the execute_command was moved from top.h to gdbcmd.h (but the implementation of execute_command with the new can_repeat bool is still in top.c). It would seem more natural to have all the execute_command in top.h (and top.c). (maybe the above is very much Ada minded, as Ada enforces a consistency between the declaration and the implementation of a function, including where the declaration is located). Assuming this is better placed in gdbcmd.h, then there is at least one place which says in cli-interp.c: #include "top.h" /* for "execute_command" */ > > cli_interp_base::cli_interp_base (const char *name) > : interp (name) > ... > diff --git a/gdb/event-top.c b/gdb/event-top.c > index 5852089f09..b2abb6aa62 100644 > --- a/gdb/event-top.c > +++ b/gdb/event-top.c > @@ -565,7 +565,7 @@ async_disable_stdin (void) > a whole command. */ > > void > -command_handler (const char *command) > +command_handler (const char *command, bool can_repeat) > { > struct ui *ui = current_ui; > const char *c; > @@ -580,7 +580,7 @@ command_handler (const char *command) > ; > if (c[0] != '#') > { > - execute_command (command, ui->instream == ui->stdin_stream); > + execute_command (command, ui->instream == ui->stdin_stream, can_repeat); > > /* Do any commands attached to breakpoint we stopped at. */ > bpstat_do_actions (); > @@ -631,19 +631,24 @@ command_line_append_input_line (struct buffer *cmd_line_buffer, char *rl) > > Returns EOF on end of file. > > - If REPEAT, handle command repetitions: > + REPEAT is both an input and output flag. *REPEAT is only set to true, never set to false. So, the idea is that the caller initialises it to false ? (the 'input flag' is then a little bit confusing, because the flag value is never read, even if it has to be initialised). Or told otherwise, code like the below repeat = false; /* this might do repeat */ handle_line_of_input (..., &repeat, ....); is a little bit confusing ... Maybe the arg should be called everywhere can_repeat (and should be in output set to false/true) ? > + If REPEAT is NULL, do not handle command repetitions. > + Otherwise (if it is not NULL): > + > + - If the input can possibly be repeated (basically, no "server" > + prefix), then *REPEAT is set to true. > > - If the input command line is NOT empty, the command returned is > copied into the global 'saved_command_line' var so that it can > be repeated later. > > - - OTOH, if the input command line IS empty, return the previously > - saved command instead of the empty input line. > + - OTOH, if the input command line IS empty, return a copy the Typo: a copy of the > + previously saved command instead of the empty input line. > */ > > char * > handle_line_of_input (struct buffer *cmd_line_buffer, > - char *rl, int repeat, const char *annotation_suffix) > + char *rl, bool *repeat, const char *annotation_suffix) > { > struct ui *ui = current_ui; > int from_tty = ui->instream == ui->stdin_stream; > @@ -713,8 +718,14 @@ handle_line_of_input (struct buffer *cmd_line_buffer, > previous command, return the previously saved command. */ > for (p1 = cmd; *p1 == ' ' || *p1 == '\t'; p1++) > ; > - if (repeat && *p1 == '\0') > - return saved_command_line; > + if (repeat != nullptr && *p1 == '\0') > + { > + *repeat = true; > + xfree (buffer_finish (cmd_line_buffer)); > + buffer_grow_str0 (cmd_line_buffer, saved_command_line); > + cmd_line_buffer->used_size = 0; > + 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 > @@ -727,14 +738,13 @@ handle_line_of_input (struct buffer *cmd_line_buffer, > gdb_add_history (cmd); > > /* Save into global buffer if appropriate. */ > - if (repeat) > + if (repeat != nullptr) > { > + *repeat = true; > 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 > @@ -751,8 +761,9 @@ command_line_handler (char *rl) > struct buffer *line_buffer = get_command_line_buffer (); > struct ui *ui = current_ui; > char *cmd; > + bool can_repeat = false; > > - cmd = handle_line_of_input (line_buffer, rl, 1, "prompt"); > + cmd = handle_line_of_input (line_buffer, rl, &can_repeat, "prompt"); > if (cmd == (char *) EOF) > { > /* stdin closed. The connection with the terminal is gone. > @@ -771,7 +782,7 @@ command_line_handler (char *rl) > { > ui->prompt_state = PROMPT_NEEDED; > > - command_handler (cmd); > + command_handler (cmd, can_repeat); > > if (ui->prompt_state != PROMPTED) > display_gdb_prompt (0); > ... > diff --git a/gdb/gdbcmd.h b/gdb/gdbcmd.h > index b675ae8618..cbb42249d0 100644 > --- a/gdb/gdbcmd.h > +++ b/gdb/gdbcmd.h > @@ -132,7 +132,14 @@ extern struct cmd_list_element *showchecklist; > > extern struct cmd_list_element *save_cmdlist; > > -extern void execute_command (const char *, int); > +extern void execute_command (const char *, int, bool); > + > +static inline void > +execute_command (const char *arg, int from_tty) > +{ > + return execute_command (arg, from_tty, false); function is void, no need for the return ? > +} > + > extern std::string execute_command_to_string (const char *p, int from_tty); > > extern void print_command_line (struct command_line *, unsigned int, > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing 2018-08-10 3:13 ` Tom Tromey 2018-08-10 16:07 ` Tom Tromey @ 2018-08-14 15:02 ` Pedro Alves 2018-08-14 18:33 ` Tom Tromey 1 sibling, 1 reply; 20+ messages in thread From: Pedro Alves @ 2018-08-14 15:02 UTC (permalink / raw) To: Tom Tromey; +Cc: Philippe Waroquiers, gdb-patches On 08/10/2018 04:13 AM, Tom Tromey wrote: > commit aea20ede47dfca4156f19c0b41e3a1b11e724c20 > 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. But why is handle_line_of_input freeing saved_command_line on reentrant calls? "repeat" is only supposed to be set for top-level commands, I'd think? Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing 2018-08-14 15:02 ` Pedro Alves @ 2018-08-14 18:33 ` Tom Tromey 2018-08-15 18:24 ` Tom Tromey 0 siblings, 1 reply; 20+ messages in thread From: Tom Tromey @ 2018-08-14 18:33 UTC (permalink / raw) To: Pedro Alves; +Cc: Tom Tromey, Philippe Waroquiers, gdb-patches >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: Pedro> But why is handle_line_of_input freeing saved_command_line on reentrant calls? Pedro> "repeat" is only supposed to be set for top-level commands, I'd think? Wow, that's a good point. I will test a new (much smaller) patch a bit later. Tom ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing 2018-08-14 18:33 ` Tom Tromey @ 2018-08-15 18:24 ` Tom Tromey 2018-08-16 15:34 ` Pedro Alves 0 siblings, 1 reply; 20+ messages in thread From: Tom Tromey @ 2018-08-15 18:24 UTC (permalink / raw) To: Tom Tromey; +Cc: Pedro Alves, Philippe Waroquiers, gdb-patches Tom> I will test a new (much smaller) patch a bit later. This is hilariously simpler and seems to fix the problem. Let me know what you think. Tom commit 06b655186f44cf3cf9f0c9419427a65f6b32cb5c Author: Tom Tromey <tom@tromey.com> Date: Wed Aug 15 08:25:49 2018 -0600 Do not allow command repetition from read_next_line Pedro pointed out that the commands being read by "commands" should not be repeatable. This patch fixes that problem, and the use-after-free bug, by simply modifying read_next_line to disallow repetition. Tested by the buildbot. gdb/ChangeLog 2018-08-15 Tom Tromey <tom@tromey.com> * cli/cli-script.c (read_next_line): Pass 0 as repeat argument to command_line_input. diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 9fac8ccf5f4..1e5f5a8e30d 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,8 @@ +2018-08-15 Tom Tromey <tom@tromey.com> + + * cli/cli-script.c (read_next_line): Pass 0 as repeat argument to + command_line_input. + 2018-08-15 Tom Tromey <tom@tromey.com> * aarch64-linux-tdep.c (aarch64_linux_core_read_vq): Use pulongest. diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c index 6f31a400197..d03b3bcf60b 100644 --- a/gdb/cli/cli-script.c +++ b/gdb/cli/cli-script.c @@ -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, 0, "commands"); } /* Return true if CMD's name is NAME. */ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing 2018-08-15 18:24 ` Tom Tromey @ 2018-08-16 15:34 ` Pedro Alves 2018-08-17 23:12 ` Tom Tromey 0 siblings, 1 reply; 20+ messages in thread From: Pedro Alves @ 2018-08-16 15:34 UTC (permalink / raw) To: Tom Tromey; +Cc: Philippe Waroquiers, gdb-patches On 08/15/2018 07:23 PM, Tom Tromey wrote: > Tom> I will test a new (much smaller) patch a bit later. > > This is hilariously simpler and seems to fix the problem. > Let me know what you think. I think this is the right fix. Please push. > Pedro pointed out that the commands being read by "commands" should > not be repeatable. This patch fixes that problem, and the > use-after-free bug, by simply modifying read_next_line to disallow > repetition. I think it'd be good to expand a bit more about the use-after-free bug here (someone reading git logs later on won't have the context). You already had something in the previous version of the patch, could just copy it in. AFAICT, the "repeat" parameter of command_line_input is always 0 now, and so could be eliminated (in a separate patch). Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing 2018-08-16 15:34 ` Pedro Alves @ 2018-08-17 23:12 ` Tom Tromey 0 siblings, 0 replies; 20+ messages in thread From: Tom Tromey @ 2018-08-17 23:12 UTC (permalink / raw) To: Pedro Alves; +Cc: Tom Tromey, Philippe Waroquiers, gdb-patches >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: Pedro> I think it'd be good to expand a bit more about the use-after-free Pedro> bug here (someone reading git logs later on won't have the context). Pedro> You already had something in the previous version of the patch, Pedro> could just copy it in. Pedro> AFAICT, the "repeat" parameter of command_line_input is always 0 now, Pedro> and so could be eliminated (in a separate patch). I'll send both patches momentarily. I'm going to put them on the 8.2 branch as well. Tom ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing 2018-08-02 21:26 ` [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing Philippe Waroquiers 2018-08-03 18:29 ` Tom Tromey @ 2018-08-21 18:01 ` Tom Tromey 2018-08-25 19:17 ` Philippe Waroquiers 1 sibling, 1 reply; 20+ messages in thread From: Tom Tromey @ 2018-08-21 18:01 UTC (permalink / raw) To: Philippe Waroquiers; +Cc: gdb-patches >>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes: Philippe> 2018-08-02 Philippe Waroquiers <philippe.waroquiers@skynet.be> Philippe> * breakpoint.c (commands_command_1): New boolean cmd_read Philippe> to detect cmd was already read. Always allocate a new_arg Philippe> c_str to avoid accessing arg after some calls to Philippe> read_command_line as this can free arg memory. Hi. I have been meaning to update this to account for the fix I recently put in, but I've been very ill and so I haven't been able to. Philippe> + else Philippe> + { Philippe> + new_arg = arg; Philippe> + arg = new_arg.c_str (); Philippe> + } I think it should work ok now if you just remove this hunk. Tom ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing 2018-08-21 18:01 ` Tom Tromey @ 2018-08-25 19:17 ` Philippe Waroquiers 0 siblings, 0 replies; 20+ messages in thread From: Philippe Waroquiers @ 2018-08-25 19:17 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On Tue, 2018-08-21 at 12:00 -0600, Tom Tromey wrote > Philippe> + else > Philippe> + { > Philippe> + new_arg = arg; > Philippe> + arg = new_arg.c_str (); > Philippe> + } > > I think it should work ok now if you just remove this hunk. Effectively, that bit of code is not needed anymore, thanks to the fix you did for the use after free. I will update the patch (and the test according to the comments of Pedro), and resubmit an RFA. Thanks Philippe ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2018-08-25 19:17 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-02 21:26 RFA Fix regressions for multi breakpoints command line setting/clearing Philippe Waroquiers 2018-08-02 21:26 ` [RFA 2/2] Modify gdb.base/commands.exp to test multi breakpoint command setting/clearing Philippe Waroquiers 2018-08-16 15:54 ` Pedro Alves 2018-08-02 21:26 ` [RFA 1/2] Fix regressions for multi breakpoints command line setting/clearing Philippe Waroquiers 2018-08-03 18:29 ` Tom Tromey 2018-08-09 4:57 ` Tom Tromey 2018-08-09 20:20 ` Philippe Waroquiers 2018-08-10 0:35 ` Tom Tromey 2018-08-10 3:05 ` Tom Tromey 2018-08-10 3:13 ` Tom Tromey 2018-08-10 16:07 ` Tom Tromey 2018-08-11 20:38 ` Tom Tromey 2018-08-13 21:32 ` Philippe Waroquiers 2018-08-14 15:02 ` Pedro Alves 2018-08-14 18:33 ` Tom Tromey 2018-08-15 18:24 ` Tom Tromey 2018-08-16 15:34 ` Pedro Alves 2018-08-17 23:12 ` Tom Tromey 2018-08-21 18:01 ` Tom Tromey 2018-08-25 19:17 ` Philippe Waroquiers
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).