(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.

1 files changed, 30 insertions, 5 deletions

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)