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

[Jan Vrany <jan.vrany@fit.cvut.cz>]

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