[PR,mi/18833] Provide unlimited redirection stack for MI

Message ID 047d7b15fcc1c172a6051d8960c4@google.com
State New, archived
Headers

Commit Message

Doug Evans Aug. 17, 2015, 10:32 p.m. UTC
  Hi.

I wrote this patch up before noticing that someone had already done so.
https://sourceware.org/ml/gdb-patches/2015-08/msg00438.html

Adrian's email bounces, so not sure what to do with the attribution.

The problem here is that if we invoke something from python
with to_string=True with MI as the current interpreter,
then that'll set up one level of redirection.
Then if processing that python requires a second redirection,
e.g. mi_command_param_changed,
then MI's stack size of two will discard the original entry,
and then when the python completes MI's uiout will be left pointing
to freed memory from the redirection.

The fix is to just copy what CLI does: use a VEC to record the stack.

Adrian never completed his testcase.
If someone wants to complete it and add it great.
My testcase is sufficient to trigger the crash
and is how I ran into this bug.
I did copy over Pedro's suggestion for skip_python_tests_prompt.

2015-08-17  Doug Evans  <dje@google.com>

	PR mi/18833
	* cli/cli-logging.c (pop_output_files): Don't restore redirection
	if MI-like.
	* mi/mi-out.c: #include "vec.h".
	(ui_filep): New type.
	(DEV_VEC_P (ui_filep)): New type.
	(struct ui_out_data) <buffer, original_buffer>: Delete.
	(struct ui_out_data) <streams>: New member.
	(mi_ui_out_impl): Add data_destroy field.
	(mi_field_string, mi_field_fmt): Update.
	(mi_flush, mi_redirect, field_separator): Update.
	(mi_open, mi_close): Update.
	(mi_out_buffered, mi_out_rewind, mi_out_put): Update.
	(mi_out_data_ctor, mi_out_data_dtor): New functions.
	(mi_out_new): Call mi_out_data_ctor.

	testsuite/
	* lib/gdb.exp (skip_python_tests_prompt): Renamed from
	skip_python_tests.  New arg prompt_regexp.
	(skip_python_tests): New function.
	* lib/mi-support.exp (mi_skip_python_tests): New function.
	* gdb.python/py-mi-objfile-gdb.py: New file.
	* gdb.python/py-mi-objfile.c: New file.
	* gdb.python/py-mi-objfile.exp: New file.

+}
  

Comments

Pedro Alves Aug. 18, 2015, 12:09 p.m. UTC | #1
On 08/17/2015 11:32 PM, Doug Evans wrote:

> I wrote this patch up before noticing that someone had already done so.
> https://sourceware.org/ml/gdb-patches/2015-08/msg00438.html
> 
> Adrian's email bounces, so not sure what to do with the attribution.

Adrian's changes were done under Freescale's copyright assignment
at the time.  We should simply use the email address he had then,
just like we don't go over older ChangeLog entries and change
email addresses when we change jobs.  (tbc, I don't know if he
changed jobs).

> 
> The problem here is that if we invoke something from python
> with to_string=True with MI as the current interpreter,
> then that'll set up one level of redirection.
> Then if processing that python requires a second redirection,
> e.g. mi_command_param_changed,
> then MI's stack size of two will discard the original entry,
> and then when the python completes MI's uiout will be left pointing
> to freed memory from the redirection.
> 
> The fix is to just copy what CLI does: use a VEC to record the stack.
> 
> Adrian never completed his testcase.
> If someone wants to complete it and add it great.
> My testcase is sufficient to trigger the crash
> and is how I ran into this bug.
> I did copy over Pedro's suggestion for skip_python_tests_prompt.

Thanks.  It looks good to me.  A couple minor comments below.

> +/* Constructor for an `mi_out_data' object.  */
> +
> +static void
> +mi_out_data_ctor (int mi_version, mi_out_data *self, struct ui_file  
> *stream)

I find it a (tiny) bit surprising the 'self' isn't the first parameter.

> diff --git a/gdb/testsuite/gdb.python/py-mi-objfile.exp  
> b/gdb/testsuite/gdb.python/py-mi-objfile.exp
> new file mode 100644
> index 0000000..2f69453
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-mi-objfile.exp
> @@ -0,0 +1,58 @@

> +# This file is part of the GDB testsuite.  It tests Python-based
> +# pretty-printing for MI.
> +

This comment looks stale.

Thanks,
Pedro Alves
  

Patch

diff --git a/gdb/cli/cli-logging.c b/gdb/cli/cli-logging.c
index 7d340c1..c282260 100644
--- a/gdb/cli/cli-logging.c
+++ b/gdb/cli/cli-logging.c
@@ -177,7 +177,9 @@  pop_output_files (void)
    saved_output.targ = NULL;
    saved_output.targerr = NULL;

-  ui_out_redirect (current_uiout, NULL);
+  /* Stay consistent with handle_redirections.  */
+  if (!ui_out_is_mi_like_p (current_uiout))
+    ui_out_redirect (current_uiout, NULL);
  }

  /* This is a helper for the `set logging' command.  */
diff --git a/gdb/mi/mi-out.c b/gdb/mi/mi-out.c
index fa8282f..b4693f4 100644
--- a/gdb/mi/mi-out.c
+++ b/gdb/mi/mi-out.c
@@ -22,19 +22,23 @@ 
  #include "defs.h"
  #include "ui-out.h"
  #include "mi-out.h"
+#include "vec.h"
+
+typedef struct ui_file *ui_filep;
+DEF_VEC_P (ui_filep);

  struct ui_out_data
    {
      int suppress_field_separator;
      int suppress_output;
      int mi_version;
-    struct ui_file *buffer;
-    struct ui_file *original_buffer;
+    VEC (ui_filep) *streams;
    };
  typedef struct ui_out_data mi_out_data;

  /* These are the MI output functions */

+static void mi_out_data_dtor (struct ui_out *ui_out);
  static void mi_table_begin (struct ui_out *uiout, int nbrofcols,
  			    int nr_rows, const char *tblid);
  static void mi_table_body (struct ui_out *uiout);
@@ -85,7 +89,7 @@  static const struct ui_out_impl mi_ui_out_impl =
    mi_wrap_hint,
    mi_flush,
    mi_redirect,
-  0,
+  mi_out_data_dtor,
    1, /* Needs MI hacks.  */
  };

@@ -215,17 +219,19 @@  mi_field_string (struct ui_out *uiout, int fldno, int  
width,
  		 enum ui_align align, const char *fldname, const char *string)
  {
    mi_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream;

    if (data->suppress_output)
      return;

+  stream = VEC_last (ui_filep, data->streams);
    field_separator (uiout);
    if (fldname)
-    fprintf_unfiltered (data->buffer, "%s=", fldname);
-  fprintf_unfiltered (data->buffer, "\"");
+    fprintf_unfiltered (stream, "%s=", fldname);
+  fprintf_unfiltered (stream, "\"");
    if (string)
-    fputstr_unfiltered (string, '"', data->buffer);
-  fprintf_unfiltered (data->buffer, "\"");
+    fputstr_unfiltered (string, '"', stream);
+  fprintf_unfiltered (stream, "\"");
  }

  /* This is the only field function that does not align.  */
@@ -236,17 +242,19 @@  mi_field_fmt (struct ui_out *uiout, int fldno, int  
width,
  	      const char *format, va_list args)
  {
    mi_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream;

    if (data->suppress_output)
      return;

+  stream = VEC_last (ui_filep, data->streams);
    field_separator (uiout);
    if (fldname)
-    fprintf_unfiltered (data->buffer, "%s=\"", fldname);
+    fprintf_unfiltered (stream, "%s=\"", fldname);
    else
-    fputs_unfiltered ("\"", data->buffer);
-  vfprintf_unfiltered (data->buffer, format, args);
-  fputs_unfiltered ("\"", data->buffer);
+    fputs_unfiltered ("\"", stream);
+  vfprintf_unfiltered (stream, format, args);
+  fputs_unfiltered ("\"", stream);
  }

  void
@@ -275,8 +283,9 @@  void
  mi_flush (struct ui_out *uiout)
  {
    mi_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream = VEC_last (ui_filep, data->streams);

-  gdb_flush (data->buffer);
+  gdb_flush (stream);
  }

  int
@@ -285,15 +294,9 @@  mi_redirect (struct ui_out *uiout, struct ui_file  
*outstream)
    mi_out_data *data = ui_out_data (uiout);

    if (outstream != NULL)
-    {
-      data->original_buffer = data->buffer;
-      data->buffer = outstream;
-    }
-  else if (data->original_buffer != NULL)
-    {
-      data->buffer = data->original_buffer;
-      data->original_buffer = NULL;
-    }
+    VEC_safe_push (ui_filep, data->streams, outstream);
+  else
+    VEC_pop (ui_filep, data->streams);

    return 0;
  }
@@ -306,29 +309,31 @@  static void
  field_separator (struct ui_out *uiout)
  {
    mi_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream = VEC_last (ui_filep, data->streams);

    if (data->suppress_field_separator)
      data->suppress_field_separator = 0;
    else
-    fputc_unfiltered (',', data->buffer);
+    fputc_unfiltered (',', stream);
  }

  static void
  mi_open (struct ui_out *uiout, const char *name, enum ui_out_type type)
  {
    mi_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream = VEC_last (ui_filep, data->streams);

    field_separator (uiout);
    data->suppress_field_separator = 1;
    if (name)
-    fprintf_unfiltered (data->buffer, "%s=", name);
+    fprintf_unfiltered (stream, "%s=", name);
    switch (type)
      {
      case ui_out_type_tuple:
-      fputc_unfiltered ('{', data->buffer);
+      fputc_unfiltered ('{', stream);
        break;
      case ui_out_type_list:
-      fputc_unfiltered ('[', data->buffer);
+      fputc_unfiltered ('[', stream);
        break;
      default:
        internal_error (__FILE__, __LINE__, _("bad switch"));
@@ -339,14 +344,15 @@  static void
  mi_close (struct ui_out *uiout, enum ui_out_type type)
  {
    mi_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream = VEC_last (ui_filep, data->streams);

    switch (type)
      {
      case ui_out_type_tuple:
-      fputc_unfiltered ('}', data->buffer);
+      fputc_unfiltered ('}', stream);
        break;
      case ui_out_type_list:
-      fputc_unfiltered (']', data->buffer);
+      fputc_unfiltered (']', stream);
        break;
      default:
        internal_error (__FILE__, __LINE__, _("bad switch"));
@@ -360,8 +366,9 @@  void
  mi_out_buffered (struct ui_out *uiout, char *string)
  {
    mi_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream = VEC_last (ui_filep, data->streams);

-  fprintf_unfiltered (data->buffer, "%s", string);
+  fprintf_unfiltered (stream, "%s", string);
  }

  /* Clear the buffer.  */
@@ -370,8 +377,9 @@  void
  mi_out_rewind (struct ui_out *uiout)
  {
    mi_out_data *data = ui_out_data (uiout);
+  struct ui_file *stream = VEC_last (ui_filep, data->streams);

-  ui_file_rewind (data->buffer);
+  ui_file_rewind (stream);
  }

  /* Dump the buffer onto the specified stream.  */
@@ -380,9 +388,10 @@  void
  mi_out_put (struct ui_out *uiout, struct ui_file *stream)
  {
    mi_out_data *data = ui_out_data (uiout);
+  struct ui_file *outstream = VEC_last (ui_filep, data->streams);

-  ui_file_put (data->buffer, ui_file_write_for_put, stream);
-  ui_file_rewind (data->buffer);
+  ui_file_put (outstream, ui_file_write_for_put, stream);
+  ui_file_rewind (outstream);
  }

  /* Return the current MI version.  */
@@ -395,19 +404,41 @@  mi_version (struct ui_out *uiout)
    return data->mi_version;
  }

+/* Constructor for an `mi_out_data' object.  */
+
+static void
+mi_out_data_ctor (int mi_version, mi_out_data *self, struct ui_file  
*stream)
+{
+  gdb_assert (stream != NULL);
+
+  self->streams = NULL;
+  VEC_safe_push (ui_filep, self->streams, stream);
+
+  self->suppress_field_separator = 0;
+  self->suppress_output = 0;
+  self->mi_version = mi_version;
+}
+
+/* The destructor.  */
+
+static void
+mi_out_data_dtor (struct ui_out *ui_out)
+{
+  mi_out_data *data = ui_out_data (ui_out);
+
+  VEC_free (ui_filep, data->streams);
+  xfree (data);
+}
+
  /* Initialize private members at startup.  */

  struct ui_out *
  mi_out_new (int mi_version)
  {
    int flags = 0;
-
    mi_out_data *data = XNEW (mi_out_data);
-  data->suppress_field_separator = 0;
-  data->suppress_output = 0;
-  data->mi_version = mi_version;
-  /* FIXME: This code should be using a ``string_file'' and not the
-     TUI buffer hack. */
-  data->buffer = mem_fileopen ();
+  struct ui_file *stream = mem_fileopen ();
+
+  mi_out_data_ctor (mi_version, data, stream);
    return ui_out_new (&mi_ui_out_impl, data, flags);
  }
diff --git a/gdb/testsuite/gdb.python/py-mi-objfile-gdb.py  
b/gdb/testsuite/gdb.python/py-mi-objfile-gdb.py
new file mode 100644
index 0000000..6e41773
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-mi-objfile-gdb.py
@@ -0,0 +1,27 @@ 
+# Copyright (C) 2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the GDB testsuite.
+
+import gdb
+
+# PR 18833
+# We want to have two levels of redirection while MI is current_uiout.
+# This will create one for to_string=True and then another for the
+# parameter change notification.
+gdb.execute("set width 101", to_string=True)
+# And finally a command that will use the original MI stream, which in a
+# buggy gdb will use just-freed data.
+gdb.execute("list")
diff --git a/gdb/testsuite/gdb.python/py-mi-objfile.c  
b/gdb/testsuite/gdb.python/py-mi-objfile.c
new file mode 100644
index 0000000..b92fefb
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-mi-objfile.c
@@ -0,0 +1,22 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2015 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see  <http://www.gnu.org/licenses/>.   
*/
+
+int
+main (void)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.python/py-mi-objfile.exp  
b/gdb/testsuite/gdb.python/py-mi-objfile.exp
new file mode 100644
index 0000000..2f69453
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-mi-objfile.exp
@@ -0,0 +1,58 @@ 
+# Copyright (C) 2008-2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the GDB testsuite.  It tests Python-based
+# pretty-printing for MI.
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi2"
+
+gdb_exit
+if [mi_gdb_start] {
+    continue
+}
+
+standard_testfile
+set pyfile ${testfile}-gdb.py
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}"  
executable {debug}] != "" } {
+    untested ${testfile}.exp
+    return -1
+}
+
+if { [mi_skip_python_tests] } { continue }
+
+# Make the -gdb.py script available to gdb, it is automagically loaded by  
gdb.
+# Care is taken to put it in the same directory as the binary so that
+# gdb will find it.
+set remote_python_file [gdb_remote_download host  
${srcdir}/${subdir}/${pyfile}]
+
+mi_delete_breakpoints
+mi_gdb_reinitialize_dir $srcdir/$subdir
+mi_gdb_test "set auto-load safe-path ${remote_python_file}" \
+    {.*\^done} \
+    "set safe-path"
+
+if [is_remote host] {
+    set filename ${testfile}
+    remote_download host ${binfile} ${filename}
+} else {
+    set filename ${binfile}
+}
+
+# PR 18833.  This will cause an unpatched gdb to crash.
+mi_gdb_test "-file-exec-and-symbols ${filename}" \
+    ".*\\^done,line=.*${srcfile}\"" \
+    "file-exec-and-symbols operation"
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index c5b7a06..56cde7a 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -1734,35 +1734,35 @@  proc skip_d_tests {} {
  }

  # Return a 1 for configurations that do not support Python scripting.
+# PROMPT_REGEXP is the expected prompt.

-proc skip_python_tests {} {
-    global gdb_prompt
+proc skip_python_tests_prompt { prompt_regexp } {
      global gdb_py_is_py3k
      global gdb_py_is_py24

      gdb_test_multiple "python print ('test')" "verify python support" {
-	-re "not supported.*$gdb_prompt $"	{
+	-re "not supported.*$prompt_regexp" {
  	    unsupported "Python support is disabled."
  	    return 1
  	}
-	-re "$gdb_prompt $"	{}
+	-re "$prompt_regexp" {}
      }

      set gdb_py_is_py24 0
      gdb_test_multiple "python print (sys.version_info\[0\])" "check if  
python 3" {
-	-re "3.*$gdb_prompt $"	{
+	-re "3.*$prompt_regexp" {
              set gdb_py_is_py3k 1
          }
-	-re ".*$gdb_prompt $"	{
+	-re ".*$prompt_regexp" {
              set gdb_py_is_py3k 0
          }
      }
      if { $gdb_py_is_py3k == 0 } {
          gdb_test_multiple "python print (sys.version_info\[1\])" "check if  
python 2.4" {
-	    -re "\[45\].*$gdb_prompt $" {
+	    -re "\[45\].*$prompt_regexp" {
                  set gdb_py_is_py24 1
              }
-	    -re ".*$gdb_prompt $" {
+	    -re ".*$prompt_regexp" {
                  set gdb_py_is_py24 0
              }
          }
@@ -1771,6 +1771,15 @@  proc skip_python_tests {} {
      return 0
  }

+# Return a 1 for configurations that do not support Python scripting.
+# Note: This also sets various globals that specify which version of Python
+# is in use.  See skip_python_tests_prompt.
+
+proc skip_python_tests {} {
+    global gdb_prompt
+    return [skip_python_tests_prompt "$gdb_prompt $"]
+}
+
  # Return a 1 if we should skip shared library tests.

  proc skip_shlib_tests {} {
diff --git a/gdb/testsuite/lib/mi-support.exp  
b/gdb/testsuite/lib/mi-support.exp
index 0d17ecb..dd6c41a 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -2501,3 +2501,12 @@  proc mi_make_breakpoint_table {bp_list} {
      # Assemble the final regexp.
       
return "BreakpointTable={nr_rows=\"$nr\",nr_cols=\"$nc\",$header,$body}"
  }
+
+# Return a 1 for configurations that do not support Python scripting.
+# Note: This also sets various globals that specify which version of Python
+# is in use.  See skip_python_tests_prompt.
+
+proc mi_skip_python_tests {} {
+    global mi_gdb_prompt
+    return [skip_python_tests_prompt "$mi_gdb_prompt$"]