From patchwork Fri Dec 1 21:21:00 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joel Brobecker X-Patchwork-Id: 24683 Received: (qmail 117706 invoked by alias); 1 Dec 2017 21:21:08 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 117397 invoked by uid 89); 1 Dec 2017 21:21:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.2 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_STOCKGEN, KB_WAM_FROM_NAME_SINGLEWORD, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=residing, 9868, sk:express, amazing X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 01 Dec 2017 21:21:04 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 98AAF116CC8; Fri, 1 Dec 2017 16:21:02 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id XOsxiIkS85Cv; Fri, 1 Dec 2017 16:21:02 -0500 (EST) Received: from tron.gnat.com (tron.gnat.com [IPv6:2620:20:4000:0:46a8:42ff:fe0e:e294]) by rock.gnat.com (Postfix) with ESMTP id 86163116CC6; Fri, 1 Dec 2017 16:21:02 -0500 (EST) Received: by tron.gnat.com (Postfix, from userid 4233) id 822FD175; Fri, 1 Dec 2017 16:21:02 -0500 (EST) From: Joel Brobecker To: gdb-patches@sourceware.org Cc: Xavier Roirand Subject: [pushed] (Ada) GDB crash printing expression with type casting Date: Fri, 1 Dec 2017 16:21:00 -0500 Message-Id: <1512163260-128673-1-git-send-email-brobecker@adacore.com> Hello, 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, pushed to master. Thanks, 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 + + * 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 Joel Brobecker Sergio Durigan Junior 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);