PR mi/15806: Fix quoting of async events

Message ID 1398568091-21253-1-git-send-email-simon.marchi@ericsson.com
State Superseded
Headers

Commit Message

Simon Marchi April 27, 2014, 3:08 a.m. UTC
  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(-)
  

Comments

Marc Khouzam April 28, 2014, 6:36 p.m. UTC | #1
> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Simon Marchi
> Sent: Saturday, April 26, 2014 11:08 PM
> To: gdb-patches@sourceware.org
> Cc: Simon Marchi
> Subject: [PATCH] PR mi/15806: Fix quoting of async events
> 
> 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",...}

Another impact of this bug is that a frontend cannot properly obtain
the string used in a dprintf that was created from the console.  If a 
user manually creates a dprintf, the frontend would use 
=breakpoint-created to show this new dprintf.  With this bug
it becomes problematic to extract the printf string to be used.

I am not familiar with the intricacies of MI output escaping but I can 
confirm that the patch proposed by Simon fixes the issue I am seeing
with Eclipse and dprintf.

I don't know if it is too late for the maintenance release, but in case 
it is not, let me add my vote to have it included in 7.7.1.

Thanks

Marc
  
Simon Marchi May 12, 2014, 5:56 p.m. UTC | #2
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);
>      }
>
  
Tom Tromey May 16, 2014, 3:42 p.m. UTC | #3
>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:

Simon> The quoting in whatever goes in the event_channel of MI is little
Simon> bit broken.  Link for the lazy:
Simon>   https://sourceware.org/bugzilla/show_bug.cgi?id=15806

Thanks for looking at this.

A long time ago I wrote a somewhat similar patch, but I don't think I
noticed some of the issues you dug up.  Mine was here, I don't recall
why it didn't go in:

    https://www.sourceware.org/ml/gdb-patches/2011-01/msg00518.html

Simon> In retrospect, there is no way escaping can be done reliably in
Simon> mi_console_raw_packet for data that is already formatted, such as
Simon> event_channel. At this point, there is no way to differentiate
Simon> quotes that delimit field values from those that should be
Simon> escaped. In the case of other MI consoles, it is ok since
Simon> mi_console_raw_packet receives one big string that should be
Simon> quoted and escaped as a whole.

Simon> So, first part of the fix: for the MI channels that specify no
Simon> quoting character, no escaping at all should be done in
Simon> mi_console_raw_packet (that's the change in printchar, thanks to
Simon> Yuanhui Zhang for this).

Seems reasonable.

Did you check the other callers of printchar, even indirect ones?
I did a spot check and didn't see any issues, but it would be reassuring
if you could look.

Simon> 2014-04-26  Simon Marchi  <simon.marchi@ericsson.com>
Simon> 	PR mi/15806
Simon> 	* utils.c (printchar): Don't escape at all if quoter is NUL.
Simon> 	* mi/mi-interp.c (mi_solib_loaded): Use ui_out_field_* functions to
Simon> 	generate the output.
Simon> 	(mi_solib_unloaded): Same.

Simon> 2014-04-26  Simon Marchi  <simon.marchi@ericsson.com>

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

This is ok.  Before putting it in could you, if needed, look into the
printchar thing and then reply?  Thank you.

Tom
  
Simon Marchi May 16, 2014, 5:58 p.m. UTC | #4
On 14-05-16 11:42 AM, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:
> 
> Simon> The quoting in whatever goes in the event_channel of MI is little
> Simon> bit broken.  Link for the lazy:
> Simon>   https://sourceware.org/bugzilla/show_bug.cgi?id=15806
> 
> Thanks for looking at this.
> 
> A long time ago I wrote a somewhat similar patch, but I don't think I
> noticed some of the issues you dug up.  Mine was here, I don't recall
> why it didn't go in:
> 
>     https://www.sourceware.org/ml/gdb-patches/2011-01/msg00518.html
> 
> Simon> In retrospect, there is no way escaping can be done reliably in
> Simon> mi_console_raw_packet for data that is already formatted, such as
> Simon> event_channel. At this point, there is no way to differentiate
> Simon> quotes that delimit field values from those that should be
> Simon> escaped. In the case of other MI consoles, it is ok since
> Simon> mi_console_raw_packet receives one big string that should be
> Simon> quoted and escaped as a whole.
> 
> Simon> So, first part of the fix: for the MI channels that specify no
> Simon> quoting character, no escaping at all should be done in
> Simon> mi_console_raw_packet (that's the change in printchar, thanks to
> Simon> Yuanhui Zhang for this).
> 
> Seems reasonable.
> 
> Did you check the other callers of printchar, even indirect ones?
> I did a spot check and didn't see any issues, but it would be reassuring
> if you could look.

The only direct callers of printchar are fputstr_filtered, fputstr_unfiltered, fputstrn_filtered and fputstrn_unfiltered. To the best of my knowledge, the change should not impact the callers of those.

Semi-related: while looking at that, I realized that unlike mi_field_string, mi_field_fmt does no escaping at all. It doesn't seem like a problem right now, since mi_field_fmt is only used with things that don't really need escaping anyway (numbers, or hex strings). Still, it makes an inconsistent API and would be nice to fix, I think.

> Simon> 2014-04-26  Simon Marchi  <simon.marchi@ericsson.com>
> Simon> 	PR mi/15806
> Simon> 	* utils.c (printchar): Don't escape at all if quoter is NUL.
> Simon> 	* mi/mi-interp.c (mi_solib_loaded): Use ui_out_field_* functions to
> Simon> 	generate the output.
> Simon> 	(mi_solib_unloaded): Same.
> 
> Simon> 2014-04-26  Simon Marchi  <simon.marchi@ericsson.com>
> 
> Simon> 	* gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify): Fix
> Simon> 	erroneous dprintf expected input.
> 
> This is ok.  Before putting it in could you, if needed, look into the
> printchar thing and then reply?  Thank you.

Like I said, I think it is ok. I will wait a few days so that other people can take a look and see if they find some problems.

Thanks,

Simon
  
Tom Tromey May 16, 2014, 6:24 p.m. UTC | #5
Tom> Did you check the other callers of printchar, even indirect ones?
Tom> I did a spot check and didn't see any issues, but it would be reassuring
Tom> if you could look.

Simon> The only direct callers of printchar are fputstr_filtered,
Simon> fputstr_unfiltered, fputstrn_filtered and fputstrn_unfiltered. To the
Simon> best of my knowledge, the change should not impact the callers of
Simon> those.

Yeah, by "indirect ones" I meant callers of these as well.
I looked through them.  fputstr_unfiltered is safe.  fputstrn_filtered
is not used.  But for fputstrn_unfiltered, I see in remote.c:

    static char *
    escape_buffer (const char *buf, int n)
    [...]
      fputstrn_unfiltered (buf, n, 0, stb);

So it seems like this change could impact remote.
To know for sure, you'd have to dig a bit deeper.

One option might be to make QUOTER==-1 special, then update the call in
mi_console_raw_packet.

Simon> Semi-related: while looking at that, I realized that unlike
Simon> mi_field_string, mi_field_fmt does no escaping at all. It doesn't seem
Simon> like a problem right now, since mi_field_fmt is only used with things
Simon> that don't really need escaping anyway (numbers, or hex
Simon> strings). Still, it makes an inconsistent API and would be nice to
Simon> fix, I think.

Yes, that seems like a bug waiting to happen.

Tom
  
Simon Marchi May 16, 2014, 7:56 p.m. UTC | #6
On Fri 16 May 2014 02:24:07 PM EDT, Tom Tromey wrote:
> Tom> Did you check the other callers of printchar, even indirect ones?
> Tom> I did a spot check and didn't see any issues, but it would be reassuring
> Tom> if you could look.
>
> Simon> The only direct callers of printchar are fputstr_filtered,
> Simon> fputstr_unfiltered, fputstrn_filtered and fputstrn_unfiltered. To the
> Simon> best of my knowledge, the change should not impact the callers of
> Simon> those.
>
> Yeah, by "indirect ones" I meant callers of these as well.
> I looked through them.  fputstr_unfiltered is safe.  fputstrn_filtered
> is not used.  But for fputstrn_unfiltered, I see in remote.c:
>
>     static char *
>     escape_buffer (const char *buf, int n)
>     [...]
>       fputstrn_unfiltered (buf, n, 0, stb);
>
> So it seems like this change could impact remote.
> To know for sure, you'd have to dig a bit deeper.
>
> One option might be to make QUOTER==-1 special, then update the call in
> mi_console_raw_packet.

Indeed, there will be a difference with backslashes in the "set debug remote 1" output. Here is a small test I did. I modified the qxfer threads handler to add

    <!-- hel\lo \n-->

to its output.

Before:
Packet received: l<threads> <!-- hel\\lo \\n-->\n<thread id="p7bbe.7bbe" core="1"/>\n</threads>\n
After:
Packet received: l<threads> <!-- hel\lo \n-->\n<thread id="p7c79.7c79" core="1"/>\n</threads>\n

So yeah, here, we don't have a quoter, but we want to escape backslashes anyway.

As you suggested, quoter == -1 could have a special meaning to modify the behavior of printchar. However, I think we are mixing two things that are not related. Why should the fact that I want to escape the " character influence whether I want to escape \ ? And vice-versa. They seem quite independent to me. And if there are independent, it could mean we need two separate parameters, one saying if we want to escape a quoting character and another saying if we want to escape the backslashes. WDYT?

Simon
  
Tom Tromey May 16, 2014, 8:17 p.m. UTC | #7
>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:

Simon> As you suggested, quoter == -1 could have a special meaning to modify
Simon> the behavior of printchar. However, I think we are mixing two things
Simon> that are not related. Why should the fact that I want to escape the "
Simon> character influence whether I want to escape \ ? And vice-versa.

I think it's just the normal quoting protocol.  It's odd to quote one
character but not also quote the quoting character.

Simon> They seem quite independent to me. And if there are independent,
Simon> it could mean we need two separate parameters, one saying if we
Simon> want to escape a quoting character and another saying if we want
Simon> to escape the backslashes. WDYT?

Another option is to pass '\\' as QUOTER from remote.c.

It occurs to me now that the patch should probably update the comment
before printchar to document the special meaning of QUOTER==0.

Tom
  

Patch

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