From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6855 invoked by alias); 12 May 2014 17:56:33 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 6836 invoked by uid 89); 12 May 2014 17:56:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.2 X-HELO: usevmg21.ericsson.net Received: from usevmg21.ericsson.net (HELO usevmg21.ericsson.net) (198.24.6.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Mon, 12 May 2014 17:56:30 +0000 Received: from EUSAAHC005.ericsson.se (Unknown_Domain [147.117.188.87]) by usevmg21.ericsson.net (Symantec Mail Security) with SMTP id ED.7A.02831.599B0735; Mon, 12 May 2014 14:07:50 +0200 (CEST) Received: from [142.133.110.254] (147.117.188.8) by smtps-am.internal.ericsson.com (147.117.188.87) with Microsoft SMTP Server (TLS) id 14.3.174.1; Mon, 12 May 2014 13:56:27 -0400 Message-ID: <53710B36.6020807@ericsson.com> Date: Mon, 12 May 2014 17:56:00 -0000 From: Simon Marchi User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 MIME-Version: 1.0 To: GDB Patches Subject: Re: [PATCH] PR mi/15806: Fix quoting of async events References: <1398568091-21253-1-git-send-email-simon.marchi@ericsson.com> In-Reply-To: <1398568091-21253-1-git-send-email-simon.marchi@ericsson.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2014-05/txt/msg00137.txt.bz2 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 > > 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 > > * 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); > } >