From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 35FEB3851C0A; Tue, 19 May 2020 13:27:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 35FEB3851C0A Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [10.0.0.193] (unknown [192.222.164.54]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 9E1291E79B; Tue, 19 May 2020 09:27:16 -0400 (EDT) Subject: Re: PR25993, read of freed memory To: Alan Modra , binutils@sourceware.org, gdb-patches@sourceware.org References: <20200519043205.GT1088@bubble.grove.modra.org> From: Simon Marchi Message-ID: <111d8c5d-d615-e0ae-36de-519c43a51139@simark.ca> Date: Tue, 19 May 2020 09:27:15 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <20200519043205.GT1088@bubble.grove.modra.org> Content-Type: text/plain; charset=utf-8 Content-Language: tl Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.2 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 19 May 2020 13:27:18 -0000 On 2020-05-19 12:32 a.m., Alan Modra via Gdb-patches wrote: > ldmain.c:add_archive_element copies file name pointers from the bfd to > a lang_input_statement_type. > input->filename = abfd->filename; > input->local_sym_name = abfd->filename; > This results in stale pointers when twiddling the bfd filename in > places like the pe ld after_open. So don't free the bfd filename, > and make copies using bfd_alloc memory that won't result in small > memory leaks that annoy memory checkers. > > The patch that has already been applied for PR25993 can leave > local_sym_name dangling into freed memory, and I think it might even > be possible for ldlang.c:lookup_name to read this memory. > > Since this patch touches gdb files, OK to apply there after the 9.2 > release? Note that I don't properly handle a NULL return from > bfd_set_filename (out of memory condition). I went looking to see > what xstrprintf does, and found gdb gives an internal_error. Really?? Yes, really. As far as I know there isn't really a contingency plan in case an allocation fails, the plan is just to abort. I'm not sure what we could do that would be useful instead. But I don't think I've ever seen it happen anyway. > > PR 25993 > bfd/ > * archive.c (_bfd_get_elt_at_filepos): Don't strdup filename, > use bfd_set_filename. > * elfcode.h (_bfd_elf_bfd_from_remote_memory): Likewise. > * mach-o.c (bfd_mach_o_fat_member_init): Likewise. > * opncls.c (bfd_fopen, bfd_openstreamr, bfd_openr_iovec, bfd_openw), > (bfd_create): Likewise. > (_bfd_delete_bfd): Don't free filename. > (bfd_set_filename): Copy filename param to bfd_alloc'd memory, > return pointer to the copy or NULL on alloc fail. > * vms-lib.c (_bfd_vms_lib_get_module): Free newname and test > result of bfd_set_filename. > * bfd-in2.h: Regenerate. > gdb/ > * solib-darwin.c (darwin_bfd_open): Don't strdup pathname for > bfd_set_filename. > * solib-aix.c (solib_aix_bfd_open): Free name after calling > bfd_set_filename. > * symfile-mem.c (symbol_file_add_from_memory): Likewise. > ld/ > * emultempl/pe.em (gld_${EMULATION_NAME}_after_open): Don't copy > other_bfd_filename for bfd_set_filename, and test result of > bfd_set_filename call. Don't create a new is->filename, simply > copy from bfd filename. Free new_name after bfd_set_filename. > * emultempl/pep.em (gld_${EMULATION_NAME}_after_open): Likewise. > > diff --git a/bfd/archive.c b/bfd/archive.c > index ff64727c44..1322977744 100644 > --- a/bfd/archive.c > +++ b/bfd/archive.c > @@ -737,8 +737,7 @@ _bfd_get_elt_at_filepos (bfd *archive, file_ptr filepos) > else > { > n_bfd->origin = n_bfd->proxy_origin; > - n_bfd->filename = bfd_strdup (filename); > - if (n_bfd->filename == NULL) > + if (!bfd_set_filename (n_bfd, filename)) > goto out; > } > > diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h > index ec23fd4987..44bca15d5a 100644 > --- a/bfd/bfd-in2.h > +++ b/bfd/bfd-in2.h > @@ -643,7 +643,7 @@ bfd_boolean bfd_fill_in_gnu_debuglink_section > > char *bfd_follow_build_id_debuglink (bfd *abfd, const char *dir); > > -void bfd_set_filename (bfd *abfd, char *filename); > +char *bfd_set_filename (bfd *abfd, const char *filename); Should this return a `const char *`, just like bfd_get_filename? I haven't inspected all call sites, but it sounds like the caller shouldn't be able to modify the filename contents. > diff --git a/gdb/solib-aix.c b/gdb/solib-aix.c > index 5da1214a25..ddb14fa81d 100644 > --- a/gdb/solib-aix.c > +++ b/gdb/solib-aix.c > @@ -637,10 +637,11 @@ solib_aix_bfd_open (const char *pathname) > along with appended parenthesized member name in order to allow commands > listing all shared libraries to display. Otherwise, we would only be > displaying the name of the archive member object. */ > - bfd_set_filename (object_bfd.get (), > - xstrprintf ("%s%s", > - bfd_get_filename (archive_bfd.get ()), > - sep)); > + char *fname = xstrprintf ("%s%s", > + bfd_get_filename (archive_bfd.get ()), > + sep); > + bfd_set_filename (object_bfd.get (), fname); > + free (fname); Since the string gets copied by bfd_set_filename, let's use std::string to avoid having to free: std::string fname = string_printf ("%s%s", bfd_get_filename (archive_bfd.get ()), sep); bfd_set_filename (object_bfd.get (), fname.c_str ()); > diff --git a/gdb/symfile-mem.c b/gdb/symfile-mem.c > index e2d2e43d7f..18de07fd1c 100644 > --- a/gdb/symfile-mem.c > +++ b/gdb/symfile-mem.c > @@ -78,8 +78,7 @@ target_read_memory_bfd (bfd_vma memaddr, bfd_byte *myaddr, bfd_size_type len) > and read its in-core symbols out of inferior memory. SIZE, if > non-zero, is the known size of the object. TEMPL is a bfd > representing the target's format. NAME is the name to use for this > - symbol file in messages; it can be NULL or a malloc-allocated string > - which will be attached to the BFD. */ > + symbol file in messages; it can be NULL or a malloc-allocated string. */ > static struct objfile * > symbol_file_add_from_memory (struct bfd *templ, CORE_ADDR addr, > size_t size, char *name, int from_tty) > @@ -104,6 +103,7 @@ symbol_file_add_from_memory (struct bfd *templ, CORE_ADDR addr, > if (name == NULL) > name = xstrdup ("shared object read from target memory"); > bfd_set_filename (nbfd, name); > + free (name); It now doesn't really make sense for symbol_file_add_from_memory to accept a malloc-ed string (the point was that before we gave ownership of that malloc-ed string to bfd). Can you please change `char *name` to be `gdb::optional`? The caller that passes a name should use string_printf to build the string, as mentioned above. The caller that does not pass a name can pass `{}`, to pass an empty optional. The `if (name == NULL)` would become something like: if (!name.has_value ()) name.emplace ("shared object read from target memory"); And then: bfd_set_filename (nbfd, name->c_str ()); Simon