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

[Simon Marchi <simon.marchi@ericsson.com>]

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);
>      }
>