Re: [PATCH] PR mi/15806: Fix quoting of async events

Simon Marchi
2014-05-12T17:56:33Z

Ping ?

On 14-04-26 11:08 PM, Simon Marchi wrote:
> The quoting in whatever goes in the event_channel of MI is little bit broken.
>
> Link for the lazy:
> https://sourceware.org/bugzilla/show_bug.cgi?id=15806
>
> Here is an example of a =library-loaded event with an ill-named directory,
> /tmp/how"are\you (the problem is present with every directory on Windows since
> it uses backslashes as a path separator). The result will be the following:
>
> =library-loaded,id="/tmp/how"are\\you/libexpat.so.1",...
>
> The " between 'how' and 'are' should be escaped.
>
> Another bad behavior is double escaping in =breakpoint-created, for example:
>
> =breakpoint-created,bkpt={...,fullname="/tmp/how\\"are\\\\you/test.c",...}
>
> The two backslashes before 'how' should be one and the four before 'you' should
> be two.
>
> The reason for this is that when sending something to an MI console, escaping
> can take place at two different moments (the actual escaping work is always
> done in the printchar function):
>
> 1. When generating the content, if ui_out_field_* functions are used. Here,
> fields are automatically quoted with " and properly escaped. At least
> mi_field_string does it, not sure about mi_field_fmt, I need to investigate
> further.
>
> 2. When gdb_flush is called, to send the data in the buffer of the console to
> the actual output (stdout). At this point, mi_console_raw_packet takes the
> whole string in the buffer, quotes it, and escapes all occurences of the
> quoting character and backslashes. The event_channel does not specify a quoting
> character, so quotes are not escaped here, only backslashes.
>
> The problem with =library-loaded is that it does use fprintf_unfiltered, which
> doesn't do escaping (so, no #1). When gdb_flush is called, backslashes are
> escaped (#2).
>
> The problem with =breakpoint-created is that it first uses ui_out_field_*
> functions to generate its output, so backslashes and quotes are escaped there
> (#1). backslashes are escaped again in #2, leading to an overdose of
> backslashes.
>
> In retrospect, there is no way escaping can be done reliably in
> mi_console_raw_packet for data that is already formatted, such as
> event_channel. At this point, there is no way to differentiate quotes that
> delimit field values from those that should be escaped. In the case of other MI
> consoles, it is ok since mi_console_raw_packet receives one big string that
> should be quoted and escaped as a whole.
>
> So, first part of the fix: for the MI channels that specify no quoting
> character, no escaping at all should be done in mi_console_raw_packet (that's
> the change in printchar, thanks to Yuanhui Zhang for this). For those channels,
> whoever generates the content is responsible for proper quoting and escaping.
> This will fix the =breakpoint-created kind of problem.
>
> Second part of the fix is to make =library-loaded generate content that is
> properly escaped. For this, we use ui_out_field_* functions, instead of one big
> fprintf_unfiltered. =library-unloaded suffered from the same problem so it is
> modified as well. There might be other events that need fixing too, but that's
> all I found with a quick scan. Those that use fprintf_unfiltered but whose sole
> variable data is a %d are not critical, since it won't generate a " or a \.
>
> Finally, a test has been fixed, as it was expecting an erroneous output.
> Otherwise, all other tests that were previously passing still pass (x86-64
> linux).
>
> gdb/ChangeLog:
>
> 2014-04-26 Simon Marchi <simon.marchi@ericsson.com>
>
> PR mi/15806
> * utils.c (printchar): Don't escape at all if quoter is NUL.
> * mi/mi-interp.c (mi_solib_loaded): Use ui_out_field_* functions to
> generate the output.
> (mi_solib_unloaded): Same.
>
> gdb/testsuite/ChangeLog:
>
> 2014-04-26 Simon Marchi <simon.marchi@ericsson.com>
>
> * gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify): Fix
> erroneous dprintf expected input.
> ---
> gdb/mi/mi-interp.c | 57 ++++++++++++++------------
> gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp | 2 +-
> gdb/utils.c | 2 +-
> 3 files changed, 33 insertions(+), 28 deletions(-)
>
> diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
> index 25bf0a1..491e7f9 100644
> --- a/gdb/mi/mi-interp.c
> +++ b/gdb/mi/mi-interp.c
> @@ -764,22 +764,24 @@ static void
> mi_solib_loaded (struct so_list *solib)
> {
> struct mi_interp *mi = top_level_interpreter_data ();
> + struct ui_out *uiout = interp_ui_out (top_level_interpreter ());
>
> target_terminal_ours ();
> - if (gdbarch_has_global_solist (target_gdbarch ()))
> - fprintf_unfiltered (mi->event_channel,
> - "library-loaded,id=\"%s\",target-name=\"%s\","
> - "host-name=\"%s\",symbols-loaded=\"%d\"",
> - solib->so_original_name, solib->so_original_name,
> - solib->so_name, solib->symbols_loaded);
> - else
> - fprintf_unfiltered (mi->event_channel,
> - "library-loaded,id=\"%s\",target-name=\"%s\","
> - "host-name=\"%s\",symbols-loaded=\"%d\","
> - "thread-group=\"i%d\"",
> - solib->so_original_name, solib->so_original_name,
> - solib->so_name, solib->symbols_loaded,
> - current_inferior ()->num);
> +
> + fprintf_unfiltered (mi->event_channel, "library-loaded");
> +
> + ui_out_redirect (uiout, mi->event_channel);
> +
> + ui_out_field_string (uiout, "id", solib->so_original_name);
> + ui_out_field_string (uiout, "target-name", solib->so_original_name);
> + ui_out_field_string (uiout, "host-name", solib->so_name);
> + ui_out_field_int (uiout, "symbols-loaded", solib->symbols_loaded);
> + if (!gdbarch_has_global_solist (target_gdbarch ()))
> + {
> + ui_out_field_fmt (uiout, "thread-group", "i%d", current_inferior ()->num);
> + }
> +
> + ui_out_redirect (uiout, NULL);
>
> gdb_flush (mi->event_channel);
> }
> @@ -788,20 +790,23 @@ static void
> mi_solib_unloaded (struct so_list *solib)
> {
> struct mi_interp *mi = top_level_interpreter_data ();
> + struct ui_out *uiout = interp_ui_out (top_level_interpreter ());
>
> target_terminal_ours ();
> - if (gdbarch_has_global_solist (target_gdbarch ()))
> - fprintf_unfiltered (mi->event_channel,
> - "library-unloaded,id=\"%s\",target-name=\"%s\","
> - "host-name=\"%s\"",
> - solib->so_original_name, solib->so_original_name,
> - solib->so_name);
> - else
> - fprintf_unfiltered (mi->event_channel,
> - "library-unloaded,id=\"%s\",target-name=\"%s\","
> - "host-name=\"%s\",thread-group=\"i%d\"",
> - solib->so_original_name, solib->so_original_name,
> - solib->so_name, current_inferior ()->num);
> +
> + fprintf_unfiltered (mi->event_channel, "library-unloaded");
> +
> + ui_out_redirect (uiout, mi->event_channel);
> +
> + ui_out_field_string (uiout, "id", solib->so_original_name);
> + ui_out_field_string (uiout, "target-name", solib->so_original_name);
> + ui_out_field_string (uiout, "host-name", solib->so_name);
> + if (!gdbarch_has_global_solist (target_gdbarch ()))
> + {
> + ui_out_field_fmt (uiout, "thread-group", "i%d", current_inferior ()->num);
> + }
> +
> + ui_out_redirect (uiout, NULL);
>
> gdb_flush (mi->event_channel);
> }
> diff --git a/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp b/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
> index cb2f7f6..f023e8b 100644
> --- a/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
> +++ b/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
> @@ -96,7 +96,7 @@ proc test_insert_delete_modify { } {
> $test
> set test "dprintf marker, \"arg\" \""
> mi_gdb_test $test \
> - {.*=breakpoint-created,bkpt=\{number="6",type="dprintf".*,script=\{\"printf \\\\\"arg\\\\\" \\\\\"\"\}.*\}\r\n\^done} \
> + {.*=breakpoint-created,bkpt=\{number="6",type="dprintf".*,script=\{\"printf \\\"arg\\\" \\\"\"\}.*\}\r\n\^done} \
> $test
>
> # 2. when modifying condition
> diff --git a/gdb/utils.c b/gdb/utils.c
> index a802bfa..746a272 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -1515,7 +1515,7 @@ printchar (int c, void (*do_fputs) (const char *, struct ui_file *),
> }
> else
> {
> - if (c == '\\' || c == quoter)
> + if (quoter != 0 && (c == '\\' || c == quoter))
> do_fputs ("\\", stream);
> do_fprintf (stream, "%c", c);
> }
>