Re: PR25993, read of freed memory

[Simon Marchi <simark@simark.ca>]

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<std::string>`?

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