From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 97334 invoked by alias); 28 Oct 2019 19:01:07 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 93969 invoked by uid 89); 28 Oct 2019 19:00:58 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-20.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_STOCKGEN autolearn=ham version=3.3.1 spammy= X-HELO: mx1.osci.io Received: from polly.osci.io (HELO mx1.osci.io) (8.43.85.229) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 28 Oct 2019 19:00:56 +0000 Received: by mx1.osci.io (Postfix, from userid 994) id 194EC2047F; Mon, 28 Oct 2019 15:00:55 -0400 (EDT) Received: from gnutoolchain-gerrit.osci.io (gnutoolchain-gerrit.osci.io [8.43.85.239]) by mx1.osci.io (Postfix) with ESMTP id 649E42047F; Mon, 28 Oct 2019 15:00:50 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by gnutoolchain-gerrit.osci.io (Postfix) with ESMTP id 5447020AF6; Mon, 28 Oct 2019 15:00:50 -0400 (EDT) X-Gerrit-PatchSet: 4 Date: Mon, 28 Oct 2019 19:01:00 -0000 From: "Christian Biesinger (Code Review)" To: Christian Biesinger , Luis Machado , gdb-patches@sourceware.org Cc: Tom Tromey Auto-Submitted: auto-generated X-Gerrit-MessageType: newpatchset Subject: [review v4] Only make a nullterminated string if we need to X-Gerrit-Change-Id: I183302e1f51483ff6dff0fd5c3b0f32f0f04a5d2 X-Gerrit-Change-Number: 222 X-Gerrit-ChangeURL: X-Gerrit-Commit: 219d662fda1ce18e437e6b69855d06b920c7e442 In-Reply-To: References: Reply-To: cbiesinger@google.com, tromey@sourceware.org, luis.machado@linaro.org, cbiesinger@google.com, gdb-patches@sourceware.org MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Disposition: inline User-Agent: Gerrit/3.0.3-74-g460fb0f7e9 Content-Type: text/plain; charset=UTF-8 Message-Id: <20191028190050.5447020AF6@gnutoolchain-gerrit.osci.io> X-SW-Source: 2019-10/txt/msg00997.txt.bz2 Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/222 ...................................................................... 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-28 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 --- M gdb/symtab.c M gdb/symtab.h 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/gdb/symtab.c b/gdb/symtab.c index 79c5fde..a6a9dc9 100644 --- a/gdb/symtab.c +++ b/gdb/symtab.c @@ -832,8 +832,6 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 5300383..131a74d 100644 --- a/gdb/symtab.h +++ b/gdb/symtab.h @@ -504,7 +504,8 @@ (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) -- Gerrit-Project: binutils-gdb Gerrit-Branch: master Gerrit-Change-Id: I183302e1f51483ff6dff0fd5c3b0f32f0f04a5d2 Gerrit-Change-Number: 222 Gerrit-PatchSet: 4 Gerrit-Owner: Christian Biesinger Gerrit-Reviewer: Christian Biesinger Gerrit-Reviewer: Luis Machado Gerrit-CC: Tom Tromey Gerrit-MessageType: newpatchset