[PATCH v2] Fix segfault when invoking -var-info-path-expression on a dynamic varobj

[PATCH v2] Fix segfault when invoking -var-info-path-expression on a dynamic varobj

[Jan Vrany]

Invoking -var-info-path-expression on a dynamic varobj lead either in wrong
(nonsense) result or to a segmentation fault in cplus_describe_child().
This was caused by the fact that varobj_get_path_expr() called
cplus_path_expr_of_child() ignoring the fact the parent of the variable
is dynamic. Then, cplus_describe_child() accessed the underlaying C type
members by index, causing (i) either wrong (nonsense) expression being
returned (since dynamic child may be completely arbibtrary value)
or (ii) segmentation fault (in case the index higher than number of
underlaying C type members.

This fixes the problem by checking whether a varobj is a child of a dynamic
varobj and, if so, reporting an error to MI consumer as described in
the documentation.

gdb/ChangeLog:

	* mi/mi-cmd-var.c: Report an error if varobj is a child of dynamic
	varobj as stated in documentation.
	* varobj.c: New assert.

gdb/testsuite/Changelog:

	* gdb.python/py-mi-var-info-path-expression.c: New file.
	* gdb.python/py-mi-var-info-path-expression.py: New file.
	* gdb.python/py-mi-var-info-path-expression.exp: New file.
---
 gdb/ChangeLog                                 |  6 ++
 gdb/mi/mi-cmd-var.c                           | 10 +-
 gdb/testsuite/ChangeLog                       |  6 ++
 .../py-mi-var-info-path-expression.c          | 62 +++++++++++++
 .../py-mi-var-info-path-expression.exp        | 91 +++++++++++++++++++
 .../py-mi-var-info-path-expression.py         | 51 +++++++++++
 gdb/varobj.c                                  |  6 +-
 7 files changed, 230 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.python/py-mi-var-info-path-expression.c
 create mode 100644 gdb/testsuite/gdb.python/py-mi-var-info-path-expression.exp
 create mode 100644 gdb/testsuite/gdb.python/py-mi-var-info-path-expression.py

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 42995dfc93..5b0dde9d01 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2018-06-04  Jan Vrany  <jan.vrany@fit.cvut.cz>
+
+	* mi/mi-cmd-var.c: Report an error if varobj is a child of dynamic
+	varobj as stated in documentation.
+	* varobj.c: New assert.
+	
 2018-06-13  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
 
 	* procfs.c (_initialize_procfs): Use add_inf_child_target.
diff --git a/gdb/mi/mi-cmd-var.c b/gdb/mi/mi-cmd-var.c
index 38c59c7e66..fa47387357 100644
--- a/gdb/mi/mi-cmd-var.c
+++ b/gdb/mi/mi-cmd-var.c
@@ -438,7 +438,15 @@ mi_cmd_var_info_path_expression (const char *command, char **argv, int argc)
 
   /* Get varobj handle, if a valid var obj name was specified.  */
   var = varobj_get_handle (argv[0]);
-  
+
+  /* -var-info-path-expression is currently not valid for children of
+     a dynamic varobj.  */
+  for (struct varobj *cur = var->parent; cur != nullptr; cur = cur->parent)
+    {
+      if (varobj_is_dynamic_p (cur))
+        error (_("Invalid variable object (child of a dynamic varobj)"));
+    }
+
   const char *path_expr = varobj_get_path_expr (var);
 
   uiout->field_string ("path_expr", path_expr);
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 0059828b7b..5b8cd6ff0e 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2018-06-04  Jan Vrany  <jan.vrany@fit.cvut.cz>
+
+	* gdb.python/py-mi-var-info-path-expression.c: New file.
+	* gdb.python/py-mi-var-info-path-expression.py: New file.
+	* gdb.python/py-mi-var-info-path-expression.exp: New file.
+
 2018-06-12  Andrew Burgess  <andrew.burgess@embecosm.com>
 	    Stephen Roberts  <stephen.roberts@arm.com>
 
diff --git a/gdb/testsuite/gdb.python/py-mi-var-info-path-expression.c b/gdb/testsuite/gdb.python/py-mi-var-info-path-expression.c
new file mode 100644
index 0000000000..9d6ff126dd
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-mi-var-info-path-expression.c
@@ -0,0 +1,62 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright (C) 1999-2018 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/>.  */
+
+enum cons_type
+{
+  type_atom = 0,
+  type_cons = 1
+};
+
+struct atom
+{
+  int ival;
+};
+
+struct cons
+{
+  enum cons_type type;
+  union
+  {
+    struct atom atom;
+    struct cons *slots[2];
+  };
+};
+
+#define nil ((struct cons*)0);
+
+int
+main ()
+{
+  struct cons c1, c2, c3, c4;
+
+  c1.type = type_cons;
+  c1.slots[0] = &c4;
+  c1.slots[1] = &c2;
+
+  c2.type = type_cons;
+  c2.slots[0] = nil;
+  c2.slots[1] = &c3;
+
+  c3.type = type_cons;
+  c3.slots[0] = nil;
+  c3.slots[1] = nil;
+
+  c4.type = type_atom;
+  c4.atom.ival = 13;
+
+  return 0;			/* next line */
+}
diff --git a/gdb/testsuite/gdb.python/py-mi-var-info-path-expression.exp b/gdb/testsuite/gdb.python/py-mi-var-info-path-expression.exp
new file mode 100644
index 0000000000..ecebc2e761
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-mi-var-info-path-expression.exp
@@ -0,0 +1,91 @@
+# Copyright (C) 2008-2018 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/>.
+
+# Tests whether -var-info-path-expression fails as documented when
+# invoked on a dynamic varobj.
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+gdb_exit
+if {[mi_gdb_start]} {
+    continue
+}
+
+#
+# Start here
+#
+standard_testfile
+
+if {[gdb_compile "$srcdir/$subdir/$srcfile" $binfile executable {debug}] != "" } {
+    return -1
+}
+
+mi_gdb_test "source ${srcdir}/${subdir}/${testfile}.py" \
+  ".*\\^done" \
+  "load python file"
+
+mi_gdb_test "-enable-pretty-printing" \
+  "\\^done" \
+  "-enable-pretty-printing"
+
+mi_gdb_test "set python print-stack full" \
+  ".*\\^done" \
+  "set python print-stack full"
+
+
+mi_run_to_main
+
+
+mi_continue_to_line [gdb_get_line_number "next line" ${srcfile}] \
+  "step to breakpoint"
+
+mi_gdb_test "-var-create c1 * &c1" \
+   "\\^done.*" \
+   "-var-create c1 * &c1"
+
+mi_gdb_test "-var-info-path-expression c1" \
+  "\\^done,path_expr=\"&c1\"" \
+  "-var-info-path-expression c1"
+
+mi_gdb_test "-var-list-children c1" \
+  "\\^done,numchild=\"2\",children=.child=\{name=\"c1.car\".*child=\{name=\"c1.cdr\".*" \
+  "-var-list-children c1"
+
+mi_gdb_test "-var-info-path-expression c1.cdr" \
+  "\\^error,msg=\".*\"" \
+  "-var-info-path-expression c1.cdr"
+
+mi_gdb_test "-var-list-children c1.cdr" \
+  "\\^done,numchild=\"2\",children=.child=\{name=\"c1.cdr.car\".*child=\{name=\"c1.cdr.cdr\".*" \
+  "-var-list-children c1.cdr"
+
+mi_gdb_test "-var-info-path-expression c1.cdr.cdr" \
+  "\\^error,msg=\".*\"" \
+  "-var-info-path-expression c1.cdr.cdr"
+
+mi_gdb_test "-var-list-children c1.car" \
+  "\\^done,numchild=\"1\",children=.child=\{name=\"c1.car.atom\".*" \
+  "-var-list-children c1.car"
+
+mi_gdb_test "-var-list-children c1.car.atom" \
+  "\\^done,numchild=\"1\",children=.child=\{name=\"c1.car.atom.ival\".*" \
+  "-var-list-children c1.car.atom"
+
+mi_gdb_test "-var-info-path-expression c1.car.atom.ival" \
+  "\\^error,msg=\".*\"" \
+  "-var-info-path-expression c1.car.atom.ival"
+
+
diff --git a/gdb/testsuite/gdb.python/py-mi-var-info-path-expression.py b/gdb/testsuite/gdb.python/py-mi-var-info-path-expression.py
new file mode 100644
index 0000000000..13f9742016
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-mi-var-info-path-expression.py
@@ -0,0 +1,51 @@
+# Copyright (C) 2008-2018 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/>.
+
+import gdb
+import gdb.types
+
+class cons_pp(object):
+  def __init__(self, val):
+    self._val = val
+
+  def to_string(self):
+    if int(self._val) == 0:
+      return "nil"
+    elif int(self._val['type']) == 0:
+      return "( . )"
+    else:
+      return "%d" % self._val['atom']['ival']
+
+  def children(self):
+    if int(self._val) == 0:
+      return []
+    elif int(self._val['type']) == 0:
+      return [
+        ('atom', self._val['atom'])
+      ]
+    else:
+      return [
+        ('car' ,  self._val["slots"][0]),
+        ('cdr' ,  self._val["slots"][1]),
+      ]
+
+def cons_pp_lookup(val):
+  if str(val.type) == 'struct cons *':
+    return cons_pp(val)
+  else:
+    return None
+
+del gdb.pretty_printers[1:]
+gdb.pretty_printers.append(cons_pp_lookup)
\ No newline at end of file
diff --git a/gdb/varobj.c b/gdb/varobj.c
index a0df485ae9..6b044abdbf 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -962,9 +962,13 @@ varobj_get_path_expr (const struct varobj *var)
       /* For root varobjs, we initialize path_expr
 	 when creating varobj, so here it should be
 	 child varobj.  */
-      struct varobj *mutable_var = (struct varobj *) var;
       gdb_assert (!is_root_p (var));
 
+      /* Computation of full rooted expression for children of dynamic
+         varobjs is not supported.  */
+      gdb_assert (!varobj_is_dynamic_p (cur));
+
+      struct varobj *mutable_var = (struct varobj *) var;
       mutable_var->path_expr = (*var->root->lang_ops->path_expr_of_child) (var);
     }
 
-- 
2.18.0

Re: [PATCH v2] Fix segfault when invoking -var-info-path-expression on a dynamic varobj

[Simon Marchi]

On 2018-06-25 04:59 PM, Jan Vrany wrote:
> Invoking -var-info-path-expression on a dynamic varobj lead either in wrong
> (nonsense) result or to a segmentation fault in cplus_describe_child().
> This was caused by the fact that varobj_get_path_expr() called
> cplus_path_expr_of_child() ignoring the fact the parent of the variable
> is dynamic. Then, cplus_describe_child() accessed the underlaying C type
> members by index, causing (i) either wrong (nonsense) expression being
> returned (since dynamic child may be completely arbibtrary value)
> or (ii) segmentation fault (in case the index higher than number of
> underlaying C type members.
> 
> This fixes the problem by checking whether a varobj is a child of a dynamic
> varobj and, if so, reporting an error to MI consumer as described in
> the documentation.

Hi Jan,

The patch does not compile for me:

  CXX    varobj.o
In file included from /home/simark/src/binutils-gdb/gdb/common/common-defs.h:90,
                 from /home/simark/src/binutils-gdb/gdb/defs.h:28,
                 from /home/simark/src/binutils-gdb/gdb/varobj.c:18:
/home/simark/src/binutils-gdb/gdb/varobj.c: In function ‘const char* varobj_get_path_expr(const varobj*)’:
/home/simark/src/binutils-gdb/gdb/varobj.c:969:41: error: ‘cur’ was not declared in this scope
       gdb_assert (!varobj_is_dynamic_p (cur));
                                         ^~~
/home/simark/src/binutils-gdb/gdb/common/gdb_assert.h:33:13: note: in definition of macro ‘gdb_assert’
   ((void) ((expr) ? 0 :                                                       \
             ^~~~
> diff --git a/gdb/mi/mi-cmd-var.c b/gdb/mi/mi-cmd-var.c
> index 38c59c7e66..fa47387357 100644
> --- a/gdb/mi/mi-cmd-var.c
> +++ b/gdb/mi/mi-cmd-var.c
> @@ -438,7 +438,15 @@ mi_cmd_var_info_path_expression (const char *command, char **argv, int argc)
>  
>    /* Get varobj handle, if a valid var obj name was specified.  */
>    var = varobj_get_handle (argv[0]);
> -  
> +
> +  /* -var-info-path-expression is currently not valid for children of
> +     a dynamic varobj.  */
> +  for (struct varobj *cur = var->parent; cur != nullptr; cur = cur->parent)
> +    {
> +      if (varobj_is_dynamic_p (cur))
> +        error (_("Invalid variable object (child of a dynamic varobj)"));
> +    }
> +

To make it simpler, instead of checking all the parents recursively here, have you
considered adding a check to the varobj_get_path_expr_parent, like this?  I haven't
given it a very long though, but it does pass your test :).

diff --git a/gdb/varobj.c b/gdb/varobj.c
index f2c10ddc57ff..e601cf0b9780 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -948,6 +948,9 @@ varobj_get_path_expr_parent (const struct varobj *var)
   while (!is_root_p (parent) && !is_path_expr_parent (parent))
     parent = parent->parent;

+  if (varobj_is_dynamic_p (parent))
+    error (_("Invalid variable object (child of a dynamic varobj)"));
+
   return parent;
 }


Thanks,

Simon

Re: [PATCH v2] Fix segfault when invoking -var-info-path-expression on a dynamic varobj

[Jan Vrany]

On Tue, 2018-06-26 at 22:24 -0400, Simon Marchi wrote:
> On 2018-06-25 04:59 PM, Jan Vrany wrote:
> > Invoking -var-info-path-expression on a dynamic varobj lead either
> > in wrong
> > (nonsense) result or to a segmentation fault in
> > cplus_describe_child().
> > This was caused by the fact that varobj_get_path_expr() called
> > cplus_path_expr_of_child() ignoring the fact the parent of the
> > variable
> > is dynamic. Then, cplus_describe_child() accessed the underlaying C
> > type
> > members by index, causing (i) either wrong (nonsense) expression
> > being
> > returned (since dynamic child may be completely arbibtrary value)
> > or (ii) segmentation fault (in case the index higher than number of
> > underlaying C type members.
> > 
> > This fixes the problem by checking whether a varobj is a child of a
> > dynamic
> > varobj and, if so, reporting an error to MI consumer as described
> > in
> > the documentation.
> 
> Hi Jan,
> 
> The patch does not compile for me:
> 
>   CXX    varobj.o
> In file included from /home/simark/src/binutils-
> gdb/gdb/common/common-defs.h:90,
>                  from /home/simark/src/binutils-gdb/gdb/defs.h:28,
>                  from /home/simark/src/binutils-gdb/gdb/varobj.c:18:
> /home/simark/src/binutils-gdb/gdb/varobj.c: In function ‘const char*
> varobj_get_path_expr(const varobj*)’:
> /home/simark/src/binutils-gdb/gdb/varobj.c:969:41: error: ‘cur’ was
> not declared in this scope
>        gdb_assert (!varobj_is_dynamic_p (cur));
>                                          ^~~
> /home/simark/src/binutils-gdb/gdb/common/gdb_assert.h:33:13: note: in
> definition of macro ‘gdb_assert’
>    ((void) ((expr) ? 0
> :                                                       \
> 

Hi Simon, 

argh, sorry. Forgotten "git add". Will fix. 

>              ^~~~
> > diff --git a/gdb/mi/mi-cmd-var.c b/gdb/mi/mi-cmd-var.c
> > index 38c59c7e66..fa47387357 100644
> > --- a/gdb/mi/mi-cmd-var.c
> > +++ b/gdb/mi/mi-cmd-var.c
> > @@ -438,7 +438,15 @@ mi_cmd_var_info_path_expression (const char
> > *command, char **argv, int argc)
> >  
> >    /* Get varobj handle, if a valid var obj name was specified.  */
> >    var = varobj_get_handle (argv[0]);
> > -  
> > +
> > +  /* -var-info-path-expression is currently not valid for children
> > of
> > +     a dynamic varobj.  */
> > +  for (struct varobj *cur = var->parent; cur != nullptr; cur =
> > cur->parent)
> > +    {
> > +      if (varobj_is_dynamic_p (cur))
> > +        error (_("Invalid variable object (child of a dynamic
> > varobj)"));
> > +    }
> > +
> 
> To make it simpler, instead of checking all the parents recursively
> here, have you
> considered adding a check to the varobj_get_path_expr_parent, like
> this?  I haven't
> given it a very long though, but it does pass your test :).
> 
> diff --git a/gdb/varobj.c b/gdb/varobj.c
> index f2c10ddc57ff..e601cf0b9780 100644
> --- a/gdb/varobj.c
> +++ b/gdb/varobj.c
> @@ -948,6 +948,9 @@ varobj_get_path_expr_parent (const struct varobj
> *var)
>    while (!is_root_p (parent) && !is_path_expr_parent (parent))
>      parent = parent->parent;
> 
> +  if (varobj_is_dynamic_p (parent))
> +    error (_("Invalid variable object (child of a dynamic
> varobj)"));
> +
>    return parent;
>  }
> 

No, I have not considered this - still learning internals. Generally
I prefer to validate input as soon as possible rather than go on and
fail later but I agree it's a matter of taste. 

However, varobj_get_path_expr_parent() is indeed much better place for
the assert (the one that failed to compile :), something like:

diff --git a/gdb/varobj.c b/gdb/varobj.c
index 02441410e8..050c240bef 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -948,6 +948,10 @@ varobj_get_path_expr_parent (const struct varobj
*var)
   while (!is_root_p (parent) && !is_path_expr_parent (parent))
     parent = parent->parent;
 
+  /* Computation of full rooted expression for children of dynamic
+     varobjs is not supported.  */
+  gdb_assert (!varobj_is_dynamic_p (parent));
+
   return parent;
 }

What do you think? I'll send a new version once we decide. 

Thanks! Jan

Re: [PATCH v2] Fix segfault when invoking -var-info-path-expression on a dynamic varobj

[Simon Marchi]

On 2018-06-28 05:31, Jan Vrany wrote:
>> diff --git a/gdb/varobj.c b/gdb/varobj.c
>> index f2c10ddc57ff..e601cf0b9780 100644
>> --- a/gdb/varobj.c
>> +++ b/gdb/varobj.c
>> @@ -948,6 +948,9 @@ varobj_get_path_expr_parent (const struct varobj
>> *var)
>>    while (!is_root_p (parent) && !is_path_expr_parent (parent))
>>      parent = parent->parent;
>> 
>> +  if (varobj_is_dynamic_p (parent))
>> +    error (_("Invalid variable object (child of a dynamic
>> varobj)"));
>> +
>>    return parent;
>>  }
>> 
> 
> No, I have not considered this - still learning internals. Generally
> I prefer to validate input as soon as possible rather than go on and
> fail later but I agree it's a matter of taste.

The counter argument would be to avoid spreading the "knowledge" of the 
varobj internals in the MI/frontend code.  Also, if other parts of GDB 
want to find out the path expression parent of a varobj, the same case 
will be properly handled.

> However, varobj_get_path_expr_parent() is indeed much better place for
> the assert (the one that failed to compile :), something like:
> 
> diff --git a/gdb/varobj.c b/gdb/varobj.c
> index 02441410e8..050c240bef 100644
> --- a/gdb/varobj.c
> +++ b/gdb/varobj.c
> @@ -948,6 +948,10 @@ varobj_get_path_expr_parent (const struct varobj
> *var)
>    while (!is_root_p (parent) && !is_path_expr_parent (parent))
>      parent = parent->parent;
> 
> +  /* Computation of full rooted expression for children of dynamic
> +     varobjs is not supported.  */
> +  gdb_assert (!varobj_is_dynamic_p (parent));
> +
>    return parent;
>  }
> 
> What do you think? I'll send a new version once we decide.

Personally, I'd prefer the check in varobj_get_path_expr_parent.

Thanks,

Simon