(Ada) GDB crash printing expression with type casting

One of our users reported that trying to print the following expression,
caused GDB to SEGV:

    (gdb) print some_package.some_type (val)

In this particular instance, the crash occurred inside ada_args_match
because it is given a NULL "func", leading to the SEGV because of:

    struct type *func_type = SYMBOL_TYPE (func);

This NULL symbol comes from a list of symbols which was given to
ada_resolve_function (parameter called "syms") which then iterates
over each of them to discard the ones that don't match the actuals:

     for (k = 0; k < nsyms; k += 1)
       {
         struct type *type = ada_check_typedef (SYMBOL_TYPE (syms[k].symbol));

         if (ada_args_match (syms[k].symbol, args, nargs)
             && (fallback || return_match (type, context_type)))
         [...]
       }

What's really interesting is that, when entering the block above for
the first time, all entries in SYMS have a valid (non-NULL) symbol.
However, once we return from the call to ada_check_typedef, the first
entry of our SYMS table gets set to all zeros:

    (gdb) p syms[0]
    $2 = {symbol = 0x0, block = 0x0}

Hence the call to ada_args_match with a NULL symbol, and the ensuing
SEGV.

To find out why this happen, we need to step back a little and look
at how syms was allocated. This list of symbols comes from a symbol
lookup, which means ada_lookup_symbol_list_worker. We have our first
hint when we look at the function's documentation and see:

    This vector is transient---good only to the next call of
    ada_lookup_symbol_list.

Implementation-wise, this is done by using a static global obstack,
which we just re-initialize each time ada_lookup_symbol_list_worker
gets called:

    obstack_free (&symbol_list_obstack, NULL);
    obstack_init (&symbol_list_obstack);

This property was probably established in order to facilitate the use
of the returned vector, since the users of that function would not have
to worry about releasing that memory when no longer needed. However,
I found during this investigation that it is all to easy to indirectly
trigger another symbol lookup while still using the results of a previous
lookup.

In our particular case, there is the call to ada_check_typedef, which
leads to check_typedef. As it happens, my first symbol had a type which
was a typedef to a stub type, so check_typedef calls lookup_symbol to
find the non-stub version. This in turn eventually leads us back to
ada_lookup_symbol_list_worker, where the first thing it does is free
the memory area when our list of symbols have been residing and then
recreates a new one. in other words, SYMS then becomes a dangling
pointer!

This patch fixes the issue by having ada_lookup_symbol_list_worker
return a copy of the list of symbols, with the responsibility of
deallocating that list now transfered to the users of that list.

More generally speaking, it is absolutely amazing that we haven't seen
consequences of this issue before. This can happen fairly frequently.
For instance, I found that ada-exp.y::write_var_or_type calls
ada_lookup_symbol_list, and then, while processing that list, calls
select_possible_type_sym, which leads to ada_prefer_type, eventually
leading to ada_check_typedef again (via eg. ada_is_array_descriptor_type).

Even more amazing is the fact that, while I was able to produce multiple
scenarios where the corruption occurs, none of them leads to incorrect
behavior at the user level. In other words, it requires a very precise
set of conditions for the corruption to become user-visible, and
despite having a megalarge program where the crash occured, using that
as a template for creating a reproducer did not work (pb goes away).
This is why this patch does not come with a reproducer. On the other hand,
this should not be a problem in terms of testing coverage, as the changes
are made in common areas which, at least for the most part, are routinely
exercised during testing.

gdb/ChangeLog:

        * ada-lang.c (symbol_list_obstack): Delete.
        (resolve_subexp): Make sure "candidates" gets xfree'ed.
        (ada_lookup_symbol_list_worker): Remove the limitation that
        the result is only good until the next call, now making it
        the responsibility of the caller to free the result when no
        longer needed.  Adjust the function's intro comment accordingly.
        (ada_lookup_symbol_list): Adjust the function's intro comment.
        (ada_iterate_over_symbols): Make sure "results" gets xfree'ed.
        (ada_lookup_encoded_symbol, get_var_value): Likewise.
        (_initialize_ada_language): Remove symbol_list_obstack
        initialization.
        * ada-exp.y (block_lookup): Make sure "syms" gets xfree'ed.
        (write_var_or_type, write_name_assoc): Likewise.

Tested on x86_64-linux.

3 files changed, 88 insertions, 21 deletions

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index fe67e39..9f4b5e5 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,19 @@
+2017-12-01  Joel Brobecker  <brobecker@adacore.com>
+
+	* ada-lang.c (symbol_list_obstack): Delete.
+	(resolve_subexp): Make sure "candidates" gets xfree'ed.
+	(ada_lookup_symbol_list_worker): Remove the limitation that
+	the result is only good until the next call, now making it
+	the responsibility of the caller to free the result when no
+	longer needed.  Adjust the function's intro comment accordingly.
+	(ada_lookup_symbol_list): Adjust the function's intro comment.
+	(ada_iterate_over_symbols): Make sure "results" gets xfree'ed.
+	(ada_lookup_encoded_symbol, get_var_value): Likewise.
+	(_initialize_ada_language): Remove symbol_list_obstack
+	initialization.
+	* ada-exp.y (block_lookup): Make sure "syms" gets xfree'ed.
+	(write_var_or_type, write_name_assoc): Likewise.
+
 2017-12-01  Andrew Cagney  <cagney@redhat.com>
 	    Joel Brobecker  <brobecker@adacore.com>
 	    Sergio Durigan Junior  <sergiodj@redhat.com>
diff --git a/gdb/ada-exp.y b/gdb/ada-exp.y
index 004d58b..8c9004a 100644
--- a/gdb/ada-exp.y
+++ b/gdb/ada-exp.y
@@ -957,6 +957,8 @@ block_lookup (const struct block *context, const char *raw_name)
   struct block_symbol *syms;
   int nsyms;
   struct symtab *symtab;
+  struct cleanup *old_chain;
+  const struct block *result = NULL;
 
   if (raw_name[0] == '\'')
     {
@@ -967,6 +969,8 @@ block_lookup (const struct block *context, const char *raw_name)
     name = ada_encode (raw_name);
 
   nsyms = ada_lookup_symbol_list (name, context, VAR_DOMAIN, &syms);
+  old_chain = make_cleanup (xfree, syms);
+
   if (context == NULL
       && (nsyms == 0 || SYMBOL_CLASS (syms[0].symbol) != LOC_BLOCK))
     symtab = lookup_symtab (name);
@@ -974,7 +978,7 @@ block_lookup (const struct block *context, const char *raw_name)
     symtab = NULL;
 
   if (symtab != NULL)
-    return BLOCKVECTOR_BLOCK (SYMTAB_BLOCKVECTOR (symtab), STATIC_BLOCK);
+    result = BLOCKVECTOR_BLOCK (SYMTAB_BLOCKVECTOR (symtab), STATIC_BLOCK);
   else if (nsyms == 0 || SYMBOL_CLASS (syms[0].symbol) != LOC_BLOCK)
     {
       if (context == NULL)
@@ -986,8 +990,11 @@ block_lookup (const struct block *context, const char *raw_name)
     {
       if (nsyms > 1)
 	warning (_("Function name \"%s\" ambiguous here"), raw_name);
-      return SYMBOL_BLOCK_VALUE (syms[0].symbol);
+      result = SYMBOL_BLOCK_VALUE (syms[0].symbol);
     }
+
+  do_cleanups (old_chain);
+  return result;
 }
 
 static struct symbol*
@@ -1201,6 +1208,7 @@ write_var_or_type (struct parser_state *par_state,
   int depth;
   char *encoded_name;
   int name_len;
+  struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
 
   if (block == NULL)
     block = expression_context_block;
@@ -1228,6 +1236,7 @@ write_var_or_type (struct parser_state *par_state,
 	  encoded_name[tail_index] = '\0';
 	  nsyms = ada_lookup_symbol_list (encoded_name, block,
 					  VAR_DOMAIN, &syms);
+          make_cleanup (xfree, syms);
 	  encoded_name[tail_index] = terminator;
 
 	  /* A single symbol may rename a package or object. */
@@ -1274,6 +1283,7 @@ write_var_or_type (struct parser_state *par_state,
 	      write_object_renaming (par_state, block, renaming, renaming_len,
 				     renaming_expr, MAX_RENAMING_CHAIN_LENGTH);
 	      write_selectors (par_state, encoded_name + tail_index);
+              do_cleanups (old_chain);
 	      return NULL;
 	    default:
 	      internal_error (__FILE__, __LINE__,
@@ -1285,7 +1295,10 @@ write_var_or_type (struct parser_state *par_state,
               struct type *field_type;
               
               if (tail_index == name_len)
-                return SYMBOL_TYPE (type_sym);
+                {
+                  do_cleanups (old_chain);
+		  return SYMBOL_TYPE (type_sym);
+                }
 
               /* We have some extraneous characters after the type name.
                  If this is an expression "TYPE_NAME.FIELD0.[...].FIELDN",
@@ -1293,7 +1306,10 @@ write_var_or_type (struct parser_state *par_state,
               field_type
                 = get_symbol_field_type (type_sym, encoded_name + tail_index);
               if (field_type != NULL)
-                return field_type;
+		{
+		  do_cleanups (old_chain);
+		  return field_type;
+		}
 	      else 
 		error (_("Invalid attempt to select from type: \"%s\"."),
                        name0.ptr);
@@ -1304,13 +1320,17 @@ write_var_or_type (struct parser_state *par_state,
 						       encoded_name);
 
 	      if (type != NULL)
-		return type;
+		{
+		  do_cleanups (old_chain);
+		  return type;
+		}
 	    }
 
 	  if (nsyms == 1)
 	    {
 	      write_var_from_sym (par_state, syms[0].block, syms[0].symbol);
 	      write_selectors (par_state, encoded_name + tail_index);
+	      do_cleanups (old_chain);
 	      return NULL;
 	    }
 	  else if (nsyms == 0) 
@@ -1322,6 +1342,7 @@ write_var_or_type (struct parser_state *par_state,
 		  write_exp_msymbol (par_state, msym);
 		  /* Maybe cause error here rather than later? FIXME? */
 		  write_selectors (par_state, encoded_name + tail_index);
+		  do_cleanups (old_chain);
 		  return NULL;
 		}
 
@@ -1337,6 +1358,7 @@ write_var_or_type (struct parser_state *par_state,
 	      write_ambiguous_var (par_state, block, encoded_name,
 				   tail_index);
 	      write_selectors (par_state, encoded_name + tail_index);
+	      do_cleanups (old_chain);
 	      return NULL;
 	    }
 	}
@@ -1378,11 +1400,14 @@ write_name_assoc (struct parser_state *par_state, struct stoken name)
       struct block_symbol *syms;
       int nsyms = ada_lookup_symbol_list (name.ptr, expression_context_block,
 					  VAR_DOMAIN, &syms);
+      struct cleanup *old_chain = make_cleanup (xfree, syms);
 
       if (nsyms != 1 || SYMBOL_CLASS (syms[0].symbol) == LOC_TYPEDEF)
 	write_exp_op_with_string (par_state, OP_NAME, name);
       else
 	write_var_from_sym (par_state, syms[0].block, syms[0].symbol);
+
+      do_cleanups (old_chain);
     }
   else
     if (write_var_or_type (par_state, NULL, name) != NULL)
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 8e35ee6..c5636b9 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -345,9 +345,6 @@ static const char *known_auxiliary_function_name_patterns[] = {
   ADA_KNOWN_AUXILIARY_FUNCTION_NAME_PATTERNS NULL
 };
 
-/* Space for allocating results of ada_lookup_symbol_list.  */
-static struct obstack symbol_list_obstack;
-
 /* Maintenance-related settings for this module.  */
 
 static struct cmd_list_element *maint_set_ada_cmdlist;
@@ -3269,6 +3266,7 @@ resolve_subexp (struct expression **expp, int *pos, int deprocedure_p,
   struct value **argvec;        /* Vector of operand types (alloca'ed).  */
   int nargs;                    /* Number of operands.  */
   int oplen;
+  struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
 
   argvec = NULL;
   nargs = 0;
@@ -3441,6 +3439,7 @@ resolve_subexp (struct expression **expp, int *pos, int deprocedure_p,
                                     (exp->elts[pc + 2].symbol),
                                     exp->elts[pc + 1].block, VAR_DOMAIN,
                                     &candidates);
+	  make_cleanup (xfree, candidates);
 
           if (n_candidates > 1)
             {
@@ -3533,6 +3532,8 @@ resolve_subexp (struct expression **expp, int *pos, int deprocedure_p,
                                       (exp->elts[pc + 5].symbol),
                                       exp->elts[pc + 4].block, VAR_DOMAIN,
                                       &candidates);
+	    make_cleanup (xfree, candidates);
+
             if (n_candidates == 1)
               i = 0;
             else
@@ -3585,6 +3586,8 @@ resolve_subexp (struct expression **expp, int *pos, int deprocedure_p,
             ada_lookup_symbol_list (ada_decoded_op_name (op),
                                     (struct block *) NULL, VAR_DOMAIN,
                                     &candidates);
+	  make_cleanup (xfree, candidates);
+
           i = ada_resolve_function (candidates, n_candidates, argvec, nargs,
                                     ada_decoded_op_name (op), NULL);
           if (i < 0)
@@ -3599,10 +3602,12 @@ resolve_subexp (struct expression **expp, int *pos, int deprocedure_p,
 
     case OP_TYPE:
     case OP_REGISTER:
+      do_cleanups (old_chain);
       return NULL;
     }
 
   *pos = pc;
+  do_cleanups (old_chain);
   if (exp->elts[pc].opcode == OP_VAR_MSYM_VALUE)
     return evaluate_var_msym_value (EVAL_AVOID_SIDE_EFFECTS,
 				    exp->elts[pc + 1].objfile,
@@ -5801,10 +5806,10 @@ ada_add_all_symbols (struct obstack *obstackp,
 /* Find symbols in DOMAIN matching LOOKUP_NAME, in BLOCK and, if FULL_SEARCH
    is non-zero, enclosing scope and in global scopes, returning the number of
    matches.
-   Sets *RESULTS to point to a vector of (SYM,BLOCK) tuples,
+   Sets *RESULTS to point to a newly allocated vector of (SYM,BLOCK) tuples,
    indicating the symbols found and the blocks and symbol tables (if
-   any) in which they were found.  This vector is transient---good only to
-   the next call of ada_lookup_symbol_list.
+   any) in which they were found.  This vector should be freed when
+   no longer useful.
 
    When full_search is non-zero, any non-function/non-enumeral
    symbol match within the nest of blocks whose innermost member is BLOCK,
@@ -5824,14 +5829,17 @@ ada_lookup_symbol_list_worker (const lookup_name_info &lookup_name,
 {
   int syms_from_global_search;
   int ndefns;
+  int results_size;
+  auto_obstack obstack;
 
-  obstack_free (&symbol_list_obstack, NULL);
-  obstack_init (&symbol_list_obstack);
-  ada_add_all_symbols (&symbol_list_obstack, block, lookup_name,
+  ada_add_all_symbols (&obstack, block, lookup_name,
 		       domain, full_search, &syms_from_global_search);
 
-  ndefns = num_defns_collected (&symbol_list_obstack);
-  *results = defns_collected (&symbol_list_obstack, 1);
+  ndefns = num_defns_collected (&obstack);
+
+  results_size = obstack_object_size (&obstack);
+  *results = (struct block_symbol *) malloc (results_size);
+  memcpy (*results, defns_collected (&obstack, 1), results_size);
 
   ndefns = remove_extra_symbols (*results, ndefns);
 
@@ -5843,12 +5851,15 @@ ada_lookup_symbol_list_worker (const lookup_name_info &lookup_name,
 		  (*results)[0].symbol, (*results)[0].block);
 
   ndefns = remove_irrelevant_renamings (*results, ndefns, block);
+
   return ndefns;
 }
 
 /* Find symbols in DOMAIN matching NAME, in BLOCK and enclosing scope and
    in global scopes, returning the number of matches, and setting *RESULTS
-   to a vector of (SYM,BLOCK) tuples.
+   to a newly-allocated vector of (SYM,BLOCK) tuples.  This newly-allocated
+   vector should be freed when no longer useful.
+
    See ada_lookup_symbol_list_worker for further details.  */
 
 int
@@ -5871,13 +5882,18 @@ ada_iterate_over_symbols
 {
   int ndefs, i;
   struct block_symbol *results;
+  struct cleanup *old_chain;
 
   ndefs = ada_lookup_symbol_list_worker (name, block, domain, &results, 0);
+  old_chain = make_cleanup (xfree, results);
+
   for (i = 0; i < ndefs; ++i)
     {
       if (!callback (results[i].symbol))
 	break;
     }
+
+  do_cleanups (old_chain);
 }
 
 /* The result is as for ada_lookup_symbol_list with FULL_SEARCH set
@@ -5894,6 +5910,7 @@ ada_lookup_encoded_symbol (const char *name, const struct block *block,
 {
   struct block_symbol *candidates;
   int n_candidates;
+  struct cleanup *old_chain;
 
   /* Since we already have an encoded name, wrap it in '<>' to force a
      verbatim match.  Otherwise, if the name happens to not look like
@@ -5908,11 +5925,18 @@ ada_lookup_encoded_symbol (const char *name, const struct block *block,
 
   n_candidates = ada_lookup_symbol_list (verbatim.c_str (), block,
 					 domain, &candidates);
+  old_chain = make_cleanup (xfree, candidates);
+
   if (n_candidates == 0)
-    return;
+    {
+      do_cleanups (old_chain);
+      return;
+    }
 
   *info = candidates[0];
   info->symbol = fixup_symbol_section (info->symbol, NULL);
+
+  do_cleanups (old_chain);
 }
 
 /* Return a symbol in DOMAIN matching NAME, in BLOCK0 and enclosing
@@ -11565,16 +11589,20 @@ get_var_value (const char *name, const char *err_msg)
   int nsyms = ada_lookup_symbol_list_worker (lookup_name,
 					     get_selected_block (0),
 					     VAR_DOMAIN, &syms, 1);
+  struct cleanup *old_chain = make_cleanup (xfree, syms);
 
   if (nsyms != 1)
     {
+      do_cleanups (old_chain);
       if (err_msg == NULL)
         return 0;
       else
         error (("%s"), err_msg);
     }
 
-  return value_of_variable (syms[0].symbol, syms[0].block);
+  struct value *result = value_of_variable (syms[0].symbol, syms[0].block);
+  do_cleanups (old_chain);
+  return result;
 }
 
 /* Value of integer variable named NAME in the current environment.
@@ -14262,8 +14290,6 @@ When enabled, the debugger will stop using the DW_AT_GNAT_descriptive_type\n\
 DWARF attribute."),
      NULL, NULL, &maint_set_ada_cmdlist, &maint_show_ada_cmdlist);
 
-  obstack_init (&symbol_list_obstack);
-
   decoded_names_store = htab_create_alloc
     (256, htab_hash_string, (int (*)(const void *, const void *)) streq,
      NULL, xcalloc, xfree);