PR mi/15806: Fix quoting of async events

Original patch:
https://sourceware.org/ml/gdb-patches/2014-04/msg00552.html

New in v2:
* In remote.c:escape_buffer, pass '\\' to fputstrn_unfiltered/printchar to
make sure backslashes are escaped in remote debug output.
* Updated function documentation for printchar.

See updated ChangeLog below.

--------------------

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-06-02  Simon Marchi  <simon.marchi@ericsson.com>

	PR mi/15806
	* utils.c (printchar): Don't escape at all if quoter is NUL.
	Update function documentation to clarify effect of parameter
	QUOTER.
	* remote.c (escape_buffer): Pass '\\' as the quoter to
	fputstrn_unfiltered.
	* mi/mi-interp.c (mi_solib_loaded): Use ui_out_field_* functions to
	generate the output.
	(mi_solib_unloaded): Same.

gdb/testsuite/ChangeLog:

2014-06-02  Simon Marchi  <simon.marchi@ericsson.com>

	* gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify): Fix
	erroneous dprintf expected input.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 1fd19f679c82fa558e506fa21b39cde05b7bee12..e671ff5b513675f4068c5372ceb3f3b0bfa9e6e1 100644 (file)
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,15 @@
+2014-06-05  Simon Marchi  <simon.marchi@ericsson.com>
+
+       PR mi/15806
+       * utils.c (printchar): Don't escape at all if quoter is NUL.
+       Update function documentation to clarify effect of parameter
+       QUOTER.
+       * remote.c (escape_buffer): Pass '\\' as the quoter to
+       fputstrn_unfiltered.
+       * mi/mi-interp.c (mi_solib_loaded): Use ui_out_field_* functions to
+       generate the output.
+       (mi_solib_unloaded): Same.
+
 2014-06-05  Joel Brobecker  <brobecker@adacore.com>
 
        * development.sh: Delete.

diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index 52a3a626e4d1027b7cda4f5d513022010404d196..1b994e7d261c73d5dc799c0d7813404327b1b95c 100644 (file)
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -981,22 +981,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);
 }
@@ -1005,20 +1007,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/remote.c b/gdb/remote.c
index 8f9478cc547184a7cf19493c68320f7075380bb6..d5e5272d53b002517b9151dd8057ebf074687d40 100644 (file)
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -7082,7 +7082,7 @@ escape_buffer (const char *buf, int n)
   stb = mem_fileopen ();
   old_chain = make_cleanup_ui_file_delete (stb);
 
-  fputstrn_unfiltered (buf, n, 0, stb);
+  fputstrn_unfiltered (buf, n, '\\', stb);
   str = ui_file_xstrdup (stb, NULL);
   do_cleanups (old_chain);
   return str;

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 45f3512f9b18969c464a7cf9e19fa670c8980926..352638847e60e6a0eb1f88265ece64c944e22075 100644 (file)
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2014-06-05  Simon Marchi  <simon.marchi@ericsson.com>
+
+       * gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify): Fix
+       erroneous dprintf expected input.
+
 2014-06-04  Doug Evans  <xdje42@gmail.com>
 
        * gdb.guile/scm-generics.exp: Delete.

diff --git a/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp b/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
index aa991cf58f290314e7222c0f1c3b9448a9101306..70a787645eeead869c49c29598bbd7e3dc54ef2e 100644 (file)
--- 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 6e8f3849bf16d73b0d0162429e44e36b70955790..f61974d3aab93958f106a03712114c906ce0be57 100644 (file)
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1502,7 +1502,13 @@ parse_escape (struct gdbarch *gdbarch, const char **string_ptr)
 /* Print the character C on STREAM as part of the contents of a literal
    string whose delimiter is QUOTER.  Note that this routine should only
    be call for printing things which are independent of the language
-   of the program being debugged.  */
+   of the program being debugged.
+
+   printchar will normally escape backslashes and instances of QUOTER. If
+   QUOTER is 0, printchar won't escape backslashes or any quoting character.
+   As a side effect, if you pass the backslash character as the QUOTER,
+   printchar will escape backslashes as usual, but not any other quoting
+   character. */
 
 static void
 printchar (int c, void (*do_fputs) (const char *, struct ui_file *),
@@ -1545,7 +1551,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);
     }