From 0c921b219c0f62004787d352b03a746682c01489 Mon Sep 17 00:00:00 2001 From: Christian Biesinger Date: Tue, 22 Oct 2019 17:25:50 -0500 Subject: [PATCH] Only make a nullterminated string if we need to As of 7bb43059820c5febb4509b15202a93efde442bc6, we no longer need a nullterminated linkage_name to look up the entry in the hash table. So this patch makes it so we only make the copy if the entry was not found. By auditing all callers of symbol_set_names, I found out that all cases where the string may not be nullterminated already pass true for COPY_NAME. So here, I am documenting that as a requirement and am removing the code that relies on undefined behavior in symbol_set_names (it accessed the string past the provided length to check for nulltermination). Note that the Ada case at the beginning of symbol_set_names was already relying on this. gdb/ChangeLog: 2019-10-29 Christian Biesinger * symtab.h (symbol_set_names): Document that copy_name must be set to true for non-nullterminated strings. * symtab.c (symbol_set_names): Only make a nullterminated copy of linkage_name if the entry was not found and we need to demangle. Change-Id: I183302e1f51483ff6dff0fd5c3b0f32f0f04a5d2 --- gdb/ChangeLog | 7 +++++++ gdb/symtab.c | 37 +++++++++++++++++++------------------ gdb/symtab.h | 3 ++- 3 files changed, 28 insertions(+), 19 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index c96b61a07e..1c4e47c78b 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,10 @@ +2019-10-29 Christian Biesinger + + * symtab.h (symbol_set_names): Document that copy_name must be + set to true for non-nullterminated strings. + * symtab.c (symbol_set_names): Only make a nullterminated copy of + linkage_name if the entry was not found and we need to demangle. + 2019-10-29 Christian Biesinger * Makefile.in (HFILES_NO_SRCDIR): Add gdb_binary_search.h. diff --git a/gdb/symtab.c b/gdb/symtab.c index 79c5fde43c..a6a9dc9c6e 100644 --- a/gdb/symtab.c +++ b/gdb/symtab.c @@ -832,8 +832,6 @@ symbol_set_names (struct general_symbol_info *gsymbol, struct objfile_per_bfd_storage *per_bfd) { struct demangled_name_entry **slot; - /* A 0-terminated copy of the linkage name. */ - const char *linkage_name_copy; if (gsymbol->language == language_ada) { @@ -858,20 +856,7 @@ symbol_set_names (struct general_symbol_info *gsymbol, if (per_bfd->demangled_names_hash == NULL) create_demangled_names_hash (per_bfd); - if (linkage_name[len] != '\0') - { - char *alloc_name; - - alloc_name = (char *) alloca (len + 1); - memcpy (alloc_name, linkage_name, len); - alloc_name[len] = '\0'; - - linkage_name_copy = alloc_name; - } - else - linkage_name_copy = linkage_name; - - struct demangled_name_entry entry (gdb::string_view (linkage_name_copy, len)); + struct demangled_name_entry entry (gdb::string_view (linkage_name, len)); slot = ((struct demangled_name_entry **) htab_find_slot (per_bfd->demangled_names_hash.get (), &entry, INSERT)); @@ -882,6 +867,21 @@ symbol_set_names (struct general_symbol_info *gsymbol, This happens to, e.g., main.init (__go_init_main). Cope. */ || (gsymbol->language == language_go && (*slot)->demangled == nullptr)) { + /* A 0-terminated copy of the linkage name. Callers must set COPY_NAME + to true if the string might not be nullterminated. We have to make + this copy because demangling needs a nullterminated string. */ + const char *linkage_name_copy; + if (copy_name) + { + char *alloc_name = (char *) alloca (len + 1); + memcpy (alloc_name, linkage_name, len); + alloc_name[len] = '\0'; + + linkage_name_copy = alloc_name; + } + else + linkage_name_copy = linkage_name; + gdb::unique_xmalloc_ptr demangled_name_ptr (symbol_find_demangled_name (gsymbol, linkage_name_copy)); @@ -894,7 +894,7 @@ symbol_set_names (struct general_symbol_info *gsymbol, It turns out that it is actually important to still save such an entry in the hash table, because storing this name gives us better bcache hit rates for partial symbols. */ - if (!copy_name && linkage_name_copy == linkage_name) + if (!copy_name) { *slot = ((struct demangled_name_entry *) @@ -912,7 +912,8 @@ symbol_set_names (struct general_symbol_info *gsymbol, obstack_alloc (&per_bfd->storage_obstack, sizeof (demangled_name_entry) + len + 1)); char *mangled_ptr = reinterpret_cast (*slot + 1); - strcpy (mangled_ptr, linkage_name_copy); + memcpy (mangled_ptr, linkage_name, len); + mangled_ptr [len] = '\0'; new (*slot) demangled_name_entry (gdb::string_view (mangled_ptr, len)); } diff --git a/gdb/symtab.h b/gdb/symtab.h index 53003839ad..131a74d4ba 100644 --- a/gdb/symtab.h +++ b/gdb/symtab.h @@ -504,7 +504,8 @@ extern void symbol_set_language (struct general_symbol_info *symbol, (symbol)->ginfo.name = (linkage_name) /* Set the linkage and natural names of a symbol, by demangling - the linkage name. */ + the linkage name. If linkage_name may not be nullterminated, + copy_name must be set to true. */ #define SYMBOL_SET_NAMES(symbol,linkage_name,len,copy_name,objfile) \ symbol_set_names (&(symbol)->ginfo, linkage_name, len, copy_name, \ (objfile)->per_bfd) -- 2.34.1