* PR25993, read of freed memory
@ 2020-05-19 4:32 Alan Modra
2020-05-19 13:27 ` Simon Marchi
0 siblings, 1 reply; 10+ messages in thread
From: Alan Modra @ 2020-05-19 4:32 UTC (permalink / raw)
To: binutils, gdb-patches
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??
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);
/* Extracted from libbfd.c. */
diff --git a/bfd/elfcode.h b/bfd/elfcode.h
index 68db3e9ee3..5e6b2a430f 100644
--- a/bfd/elfcode.h
+++ b/bfd/elfcode.h
@@ -1680,7 +1680,6 @@ NAME(_bfd_elf,bfd_from_remote_memory)
bfd_vma high_offset;
bfd_vma shdr_end;
bfd_vma loadbase; /* Bytes. */
- char *filename;
size_t amt;
unsigned int opb = bfd_octets_per_byte (templ, NULL);
@@ -1894,22 +1893,14 @@ NAME(_bfd_elf,bfd_from_remote_memory)
free (contents);
return NULL;
}
- filename = bfd_strdup ("<in-memory>");
- if (filename == NULL)
- {
- free (bim);
- free (contents);
- return NULL;
- }
nbfd = _bfd_new_bfd ();
- if (nbfd == NULL)
+ if (nbfd == NULL
+ || !bfd_set_filename (nbfd, "<in-memory>"))
{
- free (filename);
free (bim);
free (contents);
return NULL;
}
- nbfd->filename = filename;
nbfd->xvec = templ->xvec;
bim->size = high_offset;
bim->buffer = contents;
diff --git a/bfd/mach-o.c b/bfd/mach-o.c
index 33bd81e121..ee8c6154b7 100644
--- a/bfd/mach-o.c
+++ b/bfd/mach-o.c
@@ -5578,21 +5578,18 @@ bfd_mach_o_fat_member_init (bfd *abfd,
if (ap)
{
/* Use the architecture name if known. */
- filename = bfd_strdup (ap->printable_name);
- if (filename == NULL)
- return FALSE;
+ filename = bfd_set_filename (abfd, ap->printable_name);
}
else
{
/* Forge a uniq id. */
- const size_t namelen = 2 + 8 + 1 + 2 + 8 + 1;
- filename = bfd_malloc (namelen);
- if (filename == NULL)
- return FALSE;
- snprintf (filename, namelen, "0x%lx-0x%lx",
+ char buf[2 + 8 + 1 + 2 + 8 + 1];
+ snprintf (buf, sizeof (buf), "0x%lx-0x%lx",
entry->cputype, entry->cpusubtype);
+ filename = bfd_set_filename (abfd, buf);
}
- bfd_set_filename (abfd, filename);
+ if (!filename)
+ return FALSE;
areltdata = bfd_zmalloc (sizeof (struct areltdata));
if (areltdata == NULL)
diff --git a/bfd/opncls.c b/bfd/opncls.c
index 5d3437d382..6c2bec4520 100644
--- a/bfd/opncls.c
+++ b/bfd/opncls.c
@@ -126,7 +126,6 @@ _bfd_delete_bfd (bfd *abfd)
objalloc_free ((struct objalloc *) abfd->memory);
}
- free ((char *) bfd_get_filename (abfd));
free (abfd->arelt_data);
free (abfd);
}
@@ -232,8 +231,7 @@ bfd_fopen (const char *filename, const char *target, const char *mode, int fd)
/* PR 11983: Do not cache the original filename, but
rather make a copy - the original might go away. */
- nbfd->filename = bfd_strdup (filename);
- if (nbfd->filename == NULL)
+ if (!bfd_set_filename (nbfd, filename))
{
fclose (nbfd->iostream);
_bfd_delete_bfd (nbfd);
@@ -406,8 +404,7 @@ bfd_openstreamr (const char *filename, const char *target, void *streamarg)
nbfd->iostream = stream;
/* PR 11983: Do not cache the original filename, but
rather make a copy - the original might go away. */
- nbfd->filename = bfd_strdup (filename);
- if (nbfd->filename == NULL)
+ if (!bfd_set_filename (nbfd, filename))
{
_bfd_delete_bfd (nbfd);
return NULL;
@@ -607,8 +604,7 @@ bfd_openr_iovec (const char *filename, const char *target,
/* PR 11983: Do not cache the original filename, but
rather make a copy - the original might go away. */
- nbfd->filename = bfd_strdup (filename);
- if (nbfd->filename == NULL)
+ if (!bfd_set_filename (nbfd, filename))
{
_bfd_delete_bfd (nbfd);
return NULL;
@@ -679,8 +675,7 @@ bfd_openw (const char *filename, const char *target)
/* PR 11983: Do not cache the original filename, but
rather make a copy - the original might go away. */
- nbfd->filename = bfd_strdup (filename);
- if (nbfd->filename == NULL)
+ if (!bfd_set_filename (nbfd, filename))
{
_bfd_delete_bfd (nbfd);
return NULL;
@@ -824,8 +819,7 @@ bfd_create (const char *filename, bfd *templ)
return NULL;
/* PR 11983: Do not cache the original filename, but
rather make a copy - the original might go away. */
- nbfd->filename = bfd_strdup (filename);
- if (nbfd->filename == NULL)
+ if (!bfd_set_filename (nbfd, filename))
{
_bfd_delete_bfd (nbfd);
return NULL;
@@ -2040,17 +2034,23 @@ FUNCTION
bfd_set_filename
SYNOPSIS
- void bfd_set_filename (bfd *abfd, char *filename);
+ char *bfd_set_filename (bfd *abfd, const char *filename);
DESCRIPTION
- Set the filename of @var{abfd}. The old filename, if any, is freed.
- @var{filename} must be allocated using @code{xmalloc}. After
- this call, it is owned @var{abfd}.
+ Set the filename of @var{abfd}, copying the FILENAME parameter to
+ bfd_alloc'd memory owned by @var{abfd}. Returns a pointer the
+ newly allocated name, or NULL if the allocation failed.
*/
-void
-bfd_set_filename (bfd *abfd, char *filename)
+char *
+bfd_set_filename (bfd *abfd, const char *filename)
{
- free ((char *) abfd->filename);
- abfd->filename = filename;
+ size_t len = strlen (filename) + 1;
+ char *n = bfd_alloc (abfd, len);
+ if (n)
+ {
+ memcpy (n, filename, len);
+ abfd->filename = n;
+ }
+ return n;
}
diff --git a/bfd/vms-lib.c b/bfd/vms-lib.c
index 9504cf4976..6a8a76ecb0 100644
--- a/bfd/vms-lib.c
+++ b/bfd/vms-lib.c
@@ -1452,6 +1452,12 @@ _bfd_vms_lib_get_module (bfd *abfd, unsigned int modidx)
break;
}
bfd_set_filename (res, newname);
+ free (newname);
+ if (bfd_get_filename (res) == NULL)
+ {
+ bfd_close (res);
+ return NULL;
+ }
tdata->cache[modidx] = res;
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);
return object_bfd;
}
diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
index 03d91d1690..498d6e2073 100644
--- a/gdb/solib-darwin.c
+++ b/gdb/solib-darwin.c
@@ -662,7 +662,7 @@ darwin_bfd_open (const char *pathname)
/* The current filename for fat-binary BFDs is a name generated
by BFD, usually a string containing the name of the architecture.
Reset its value to the actual filename. */
- bfd_set_filename (res.get (), xstrdup (pathname));
+ bfd_set_filename (res.get (), pathname);
return res;
}
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);
if (!bfd_check_format (nbfd, bfd_object))
error (_("Got object file from memory but can't read symbols: %s."),
diff --git a/ld/emultempl/pe.em b/ld/emultempl/pe.em
index fe65d2b266..8c5ee76233 100644
--- a/ld/emultempl/pe.em
+++ b/ld/emultempl/pe.em
@@ -1523,7 +1523,6 @@ gld_${EMULATION_NAME}_after_open (void)
struct bfd_symbol *s;
struct bfd_link_hash_entry * blhe;
const char *other_bfd_filename;
- char *n;
s = (relocs[i]->sym_ptr_ptr)[0];
@@ -1550,9 +1549,9 @@ gld_${EMULATION_NAME}_after_open (void)
continue;
/* Rename this implib to match the other one. */
- n = xmalloc (strlen (other_bfd_filename) + 1);
- strcpy (n, other_bfd_filename);
- bfd_set_filename (is->the_bfd->my_archive, n);
+ if (!bfd_set_filename (is->the_bfd->my_archive,
+ other_bfd_filename))
+ einfo ("%F%P: %pB: %E\n", is->the_bfd);
}
free (relocs);
@@ -1655,28 +1654,14 @@ gld_${EMULATION_NAME}_after_open (void)
else /* sentinel */
seq = 'c';
- /* PR 25993: It is possible that is->the_bfd-filename == is->filename.
- In which case calling bfd_set_filename on one will free the memory
- pointed to by the other. */
- if (is->filename == bfd_get_filename (is->the_bfd))
- {
- new_name = xmalloc (strlen (is->filename) + 3);
- sprintf (new_name, "%s.%c", is->filename, seq);
- bfd_set_filename (is->the_bfd, new_name);
- is->filename = new_name;
- }
- else
- {
- new_name
- = xmalloc (strlen (bfd_get_filename (is->the_bfd)) + 3);
- sprintf (new_name, "%s.%c",
- bfd_get_filename (is->the_bfd), seq);
- bfd_set_filename (is->the_bfd, new_name);
-
- new_name = xmalloc (strlen (is->filename) + 3);
- sprintf (new_name, "%s.%c", is->filename, seq);
- is->filename = new_name;
- }
+ new_name
+ = xmalloc (strlen (bfd_get_filename (is->the_bfd)) + 3);
+ sprintf (new_name, "%s.%c",
+ bfd_get_filename (is->the_bfd), seq);
+ is->filename = bfd_set_filename (is->the_bfd, new_name);
+ free (new_name);
+ if (!is->filename)
+ einfo ("%F%P: %pB: %E\n", is->the_bfd);
}
}
}
diff --git a/ld/emultempl/pep.em b/ld/emultempl/pep.em
index 699b86501c..ea8e768ea9 100644
--- a/ld/emultempl/pep.em
+++ b/ld/emultempl/pep.em
@@ -1491,7 +1491,6 @@ gld_${EMULATION_NAME}_after_open (void)
struct bfd_symbol *s;
struct bfd_link_hash_entry * blhe;
const char *other_bfd_filename;
- char *n;
s = (relocs[i]->sym_ptr_ptr)[0];
@@ -1518,9 +1517,9 @@ gld_${EMULATION_NAME}_after_open (void)
continue;
/* Rename this implib to match the other one. */
- n = xmalloc (strlen (other_bfd_filename) + 1);
- strcpy (n, other_bfd_filename);
- bfd_set_filename (is->the_bfd->my_archive, n);
+ if (!bfd_set_filename (is->the_bfd->my_archive,
+ other_bfd_filename))
+ einfo ("%F%P: %pB: %E\n", is->the_bfd);
}
free (relocs);
@@ -1623,28 +1622,14 @@ gld_${EMULATION_NAME}_after_open (void)
else /* sentinel */
seq = 'c';
- /* PR 25993: It is possible that is->the_bfd-filename == is->filename.
- In which case calling bfd_set_filename on one will free the memory
- pointed to by the other. */
- if (is->filename == bfd_get_filename (is->the_bfd))
- {
- new_name = xmalloc (strlen (is->filename) + 3);
- sprintf (new_name, "%s.%c", is->filename, seq);
- bfd_set_filename (is->the_bfd, new_name);
- is->filename = new_name;
- }
- else
- {
- new_name
- = xmalloc (strlen (bfd_get_filename (is->the_bfd)) + 3);
- sprintf (new_name, "%s.%c",
- bfd_get_filename (is->the_bfd), seq);
- bfd_set_filename (is->the_bfd, new_name);
-
- new_name = xmalloc (strlen (is->filename) + 3);
- sprintf (new_name, "%s.%c", is->filename, seq);
- is->filename = new_name;
- }
+ new_name
+ = xmalloc (strlen (bfd_get_filename (is->the_bfd)) + 3);
+ sprintf (new_name, "%s.%c",
+ bfd_get_filename (is->the_bfd), seq);
+ is->filename = bfd_set_filename (is->the_bfd, new_name);
+ free (new_name);
+ if (!is->filename)
+ einfo ("%F%P: %pB: %E\n", is->the_bfd);
}
}
}
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PR25993, read of freed memory
2020-05-19 4:32 PR25993, read of freed memory Alan Modra
@ 2020-05-19 13:27 ` Simon Marchi
2020-05-19 17:25 ` Christian Biesinger
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Simon Marchi @ 2020-05-19 13:27 UTC (permalink / raw)
To: Alan Modra, binutils, gdb-patches
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PR25993, read of freed memory
2020-05-19 13:27 ` Simon Marchi
@ 2020-05-19 17:25 ` Christian Biesinger
2020-05-19 17:26 ` Simon Marchi
2020-05-19 19:37 ` Pedro Alves
2020-05-19 23:40 ` Alan Modra
2 siblings, 1 reply; 10+ messages in thread
From: Christian Biesinger @ 2020-05-19 17:25 UTC (permalink / raw)
To: Simon Marchi; +Cc: Alan Modra, binutils, gdb-patches
On Tue, May 19, 2020 at 8:27 AM Simon Marchi <simark@simark.ca> wrote:
> 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.
We may want to add C++17's std::nullopt to gdb::optional, to make it
clearer what's going on there. Then you could pass gdb::nullopt
instead of {}.
Christian
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PR25993, read of freed memory
2020-05-19 17:25 ` Christian Biesinger
@ 2020-05-19 17:26 ` Simon Marchi
2020-05-19 18:58 ` Christian Biesinger
0 siblings, 1 reply; 10+ messages in thread
From: Simon Marchi @ 2020-05-19 17:26 UTC (permalink / raw)
To: Christian Biesinger; +Cc: Alan Modra, binutils, gdb-patches
On 2020-05-19 1:25 p.m., Christian Biesinger wrote:
> On Tue, May 19, 2020 at 8:27 AM Simon Marchi <simark@simark.ca> wrote:
>> 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.
>
> We may want to add C++17's std::nullopt to gdb::optional, to make it
> clearer what's going on there. Then you could pass gdb::nullopt
> instead of {}.
If it's technically possible, I completely agree.
Simon
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PR25993, read of freed memory
2020-05-19 17:26 ` Simon Marchi
@ 2020-05-19 18:58 ` Christian Biesinger
0 siblings, 0 replies; 10+ messages in thread
From: Christian Biesinger @ 2020-05-19 18:58 UTC (permalink / raw)
To: Simon Marchi; +Cc: Alan Modra, binutils, gdb-patches
On Tue, May 19, 2020 at 12:27 PM Simon Marchi <simark@simark.ca> wrote:
>
> On 2020-05-19 1:25 p.m., Christian Biesinger wrote:
> > On Tue, May 19, 2020 at 8:27 AM Simon Marchi <simark@simark.ca> wrote:
> >> 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.
> >
> > We may want to add C++17's std::nullopt to gdb::optional, to make it
> > clearer what's going on there. Then you could pass gdb::nullopt
> > instead of {}.
>
> If it's technically possible, I completely agree.
Yep, it is. Sent a patch to add it.
Christian
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PR25993, read of freed memory
2020-05-19 13:27 ` Simon Marchi
2020-05-19 17:25 ` Christian Biesinger
@ 2020-05-19 19:37 ` Pedro Alves
2020-05-19 19:38 ` Simon Marchi
2020-05-19 23:40 ` Alan Modra
2 siblings, 1 reply; 10+ messages in thread
From: Pedro Alves @ 2020-05-19 19:37 UTC (permalink / raw)
To: Simon Marchi, Alan Modra, binutils, gdb-patches
On 5/19/20 2:27 PM, Simon Marchi wrote:
> 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.
Wouldn't 'const char * or NULL' work? Seems way simpler to me.
>
> The `if (name == NULL)` would become something like:
>
> if (!name.has_value ())
> name.emplace ("shared object read from target memory");
>
This would just be
if (name == NULL)
name = "shared object read from target memory";
> And then:
>
> bfd_set_filename (nbfd, name->c_str ());
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PR25993, read of freed memory
2020-05-19 19:37 ` Pedro Alves
@ 2020-05-19 19:38 ` Simon Marchi
0 siblings, 0 replies; 10+ messages in thread
From: Simon Marchi @ 2020-05-19 19:38 UTC (permalink / raw)
To: Pedro Alves, Alan Modra, binutils, gdb-patches
On 2020-05-19 3:37 p.m., Pedro Alves via Gdb-patches wrote:
> On 5/19/20 2:27 PM, Simon Marchi wrote:
>> 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.
>
> Wouldn't 'const char * or NULL' work? Seems way simpler to me.
>
>>
>> The `if (name == NULL)` would become something like:
>>
>> if (!name.has_value ())
>> name.emplace ("shared object read from target memory");
>>
>
> This would just be
>
> if (name == NULL)
> name = "shared object read from target memory";
Ah yes that's better. Though the caller should still be changed to use string_printf and
str.c_str ().
Simon
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PR25993, read of freed memory
2020-05-19 13:27 ` Simon Marchi
2020-05-19 17:25 ` Christian Biesinger
2020-05-19 19:37 ` Pedro Alves
@ 2020-05-19 23:40 ` Alan Modra
2020-05-20 0:19 ` Simon Marchi
2 siblings, 1 reply; 10+ messages in thread
From: Alan Modra @ 2020-05-19 23:40 UTC (permalink / raw)
To: Simon Marchi; +Cc: binutils, gdb-patches
On Tue, May 19, 2020 at 09:27:15AM -0400, Simon Marchi wrote:
> On 2020-05-19 12:32 a.m., Alan Modra via Gdb-patches wrote:
> > -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.
Yes, I've updated the return type. One minor change needed to
mach-o.c.
> Since the string gets copied by bfd_set_filename, let's use std::string
> to avoid having to free:
Done, and symfile-mem.c updated as per down-thread suggestion to make
name a const char*.
I've left the return status from bfd_set_filename in gdb unchecked,
ie. the out-of-memory NULL return, since it seems to me that not
getting the expected name change is a minor detail very likely to be
lost in some later OOM.
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/elfcode.h b/bfd/elfcode.h
index 68db3e9ee3..5e6b2a430f 100644
--- a/bfd/elfcode.h
+++ b/bfd/elfcode.h
@@ -1680,7 +1680,6 @@ NAME(_bfd_elf,bfd_from_remote_memory)
bfd_vma high_offset;
bfd_vma shdr_end;
bfd_vma loadbase; /* Bytes. */
- char *filename;
size_t amt;
unsigned int opb = bfd_octets_per_byte (templ, NULL);
@@ -1894,22 +1893,14 @@ NAME(_bfd_elf,bfd_from_remote_memory)
free (contents);
return NULL;
}
- filename = bfd_strdup ("<in-memory>");
- if (filename == NULL)
- {
- free (bim);
- free (contents);
- return NULL;
- }
nbfd = _bfd_new_bfd ();
- if (nbfd == NULL)
+ if (nbfd == NULL
+ || !bfd_set_filename (nbfd, "<in-memory>"))
{
- free (filename);
free (bim);
free (contents);
return NULL;
}
- nbfd->filename = filename;
nbfd->xvec = templ->xvec;
bim->size = high_offset;
bim->buffer = contents;
diff --git a/bfd/mach-o.c b/bfd/mach-o.c
index 33bd81e121..43fa56cb58 100644
--- a/bfd/mach-o.c
+++ b/bfd/mach-o.c
@@ -5573,26 +5573,23 @@ bfd_mach_o_fat_member_init (bfd *abfd,
struct areltdata *areltdata;
/* Create the member filename. Use ARCH_NAME. */
const bfd_arch_info_type *ap = bfd_lookup_arch (arch_type, arch_subtype);
- char *filename;
+ const char *filename;
if (ap)
{
/* Use the architecture name if known. */
- filename = bfd_strdup (ap->printable_name);
- if (filename == NULL)
- return FALSE;
+ filename = bfd_set_filename (abfd, ap->printable_name);
}
else
{
/* Forge a uniq id. */
- const size_t namelen = 2 + 8 + 1 + 2 + 8 + 1;
- filename = bfd_malloc (namelen);
- if (filename == NULL)
- return FALSE;
- snprintf (filename, namelen, "0x%lx-0x%lx",
+ char buf[2 + 8 + 1 + 2 + 8 + 1];
+ snprintf (buf, sizeof (buf), "0x%lx-0x%lx",
entry->cputype, entry->cpusubtype);
+ filename = bfd_set_filename (abfd, buf);
}
- bfd_set_filename (abfd, filename);
+ if (!filename)
+ return FALSE;
areltdata = bfd_zmalloc (sizeof (struct areltdata));
if (areltdata == NULL)
diff --git a/bfd/opncls.c b/bfd/opncls.c
index 5d3437d382..78b2ad7dd7 100644
--- a/bfd/opncls.c
+++ b/bfd/opncls.c
@@ -126,7 +126,6 @@ _bfd_delete_bfd (bfd *abfd)
objalloc_free ((struct objalloc *) abfd->memory);
}
- free ((char *) bfd_get_filename (abfd));
free (abfd->arelt_data);
free (abfd);
}
@@ -232,8 +231,7 @@ bfd_fopen (const char *filename, const char *target, const char *mode, int fd)
/* PR 11983: Do not cache the original filename, but
rather make a copy - the original might go away. */
- nbfd->filename = bfd_strdup (filename);
- if (nbfd->filename == NULL)
+ if (!bfd_set_filename (nbfd, filename))
{
fclose (nbfd->iostream);
_bfd_delete_bfd (nbfd);
@@ -406,8 +404,7 @@ bfd_openstreamr (const char *filename, const char *target, void *streamarg)
nbfd->iostream = stream;
/* PR 11983: Do not cache the original filename, but
rather make a copy - the original might go away. */
- nbfd->filename = bfd_strdup (filename);
- if (nbfd->filename == NULL)
+ if (!bfd_set_filename (nbfd, filename))
{
_bfd_delete_bfd (nbfd);
return NULL;
@@ -607,8 +604,7 @@ bfd_openr_iovec (const char *filename, const char *target,
/* PR 11983: Do not cache the original filename, but
rather make a copy - the original might go away. */
- nbfd->filename = bfd_strdup (filename);
- if (nbfd->filename == NULL)
+ if (!bfd_set_filename (nbfd, filename))
{
_bfd_delete_bfd (nbfd);
return NULL;
@@ -679,8 +675,7 @@ bfd_openw (const char *filename, const char *target)
/* PR 11983: Do not cache the original filename, but
rather make a copy - the original might go away. */
- nbfd->filename = bfd_strdup (filename);
- if (nbfd->filename == NULL)
+ if (!bfd_set_filename (nbfd, filename))
{
_bfd_delete_bfd (nbfd);
return NULL;
@@ -824,8 +819,7 @@ bfd_create (const char *filename, bfd *templ)
return NULL;
/* PR 11983: Do not cache the original filename, but
rather make a copy - the original might go away. */
- nbfd->filename = bfd_strdup (filename);
- if (nbfd->filename == NULL)
+ if (!bfd_set_filename (nbfd, filename))
{
_bfd_delete_bfd (nbfd);
return NULL;
@@ -2040,17 +2034,23 @@ FUNCTION
bfd_set_filename
SYNOPSIS
- void bfd_set_filename (bfd *abfd, char *filename);
+ const char *bfd_set_filename (bfd *abfd, const char *filename);
DESCRIPTION
- Set the filename of @var{abfd}. The old filename, if any, is freed.
- @var{filename} must be allocated using @code{xmalloc}. After
- this call, it is owned @var{abfd}.
+ Set the filename of @var{abfd}, copying the FILENAME parameter to
+ bfd_alloc'd memory owned by @var{abfd}. Returns a pointer the
+ newly allocated name, or NULL if the allocation failed.
*/
-void
-bfd_set_filename (bfd *abfd, char *filename)
+const char *
+bfd_set_filename (bfd *abfd, const char *filename)
{
- free ((char *) abfd->filename);
- abfd->filename = filename;
+ size_t len = strlen (filename) + 1;
+ char *n = bfd_alloc (abfd, len);
+ if (n)
+ {
+ memcpy (n, filename, len);
+ abfd->filename = n;
+ }
+ return n;
}
diff --git a/bfd/vms-lib.c b/bfd/vms-lib.c
index 9504cf4976..6a8a76ecb0 100644
--- a/bfd/vms-lib.c
+++ b/bfd/vms-lib.c
@@ -1452,6 +1452,12 @@ _bfd_vms_lib_get_module (bfd *abfd, unsigned int modidx)
break;
}
bfd_set_filename (res, newname);
+ free (newname);
+ if (bfd_get_filename (res) == NULL)
+ {
+ bfd_close (res);
+ return NULL;
+ }
tdata->cache[modidx] = res;
diff --git a/gdb/solib-aix.c b/gdb/solib-aix.c
index 5da1214a25..344c1f5760 100644
--- a/gdb/solib-aix.c
+++ b/gdb/solib-aix.c
@@ -637,10 +637,10 @@ 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));
+ std::string fname = string_printf ("%s%s",
+ bfd_get_filename (archive_bfd.get ()),
+ sep);
+ bfd_set_filename (object_bfd.get (), fname.c_str ());
return object_bfd;
}
diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
index e740a41fa4..ee0483d2c8 100644
--- a/gdb/solib-darwin.c
+++ b/gdb/solib-darwin.c
@@ -662,7 +662,7 @@ darwin_bfd_open (const char *pathname)
/* The current filename for fat-binary BFDs is a name generated
by BFD, usually a string containing the name of the architecture.
Reset its value to the actual filename. */
- bfd_set_filename (res.get (), xstrdup (pathname));
+ bfd_set_filename (res.get (), pathname);
return res;
}
diff --git a/gdb/symfile-mem.c b/gdb/symfile-mem.c
index e2d2e43d7f..78096fcbae 100644
--- a/gdb/symfile-mem.c
+++ b/gdb/symfile-mem.c
@@ -78,11 +78,10 @@ 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. */
static struct objfile *
symbol_file_add_from_memory (struct bfd *templ, CORE_ADDR addr,
- size_t size, char *name, int from_tty)
+ size_t size, const char *name, int from_tty)
{
struct objfile *objf;
struct bfd *nbfd;
@@ -102,7 +101,7 @@ symbol_file_add_from_memory (struct bfd *templ, CORE_ADDR addr,
gdb_bfd_ref_ptr nbfd_holder = gdb_bfd_ref_ptr::new_reference (nbfd);
if (name == NULL)
- name = xstrdup ("shared object read from target memory");
+ name = "shared object read from target memory";
bfd_set_filename (nbfd, name);
if (!bfd_check_format (nbfd, bfd_object))
@@ -183,8 +182,9 @@ add_vsyscall_page (struct target_ops *target, int from_tty)
return;
}
- char *name = xstrprintf ("system-supplied DSO at %s",
- paddress (target_gdbarch (), vsyscall_range.start));
+ std::string name = string_printf ("system-supplied DSO at %s",
+ paddress (target_gdbarch (),
+ vsyscall_range.start));
try
{
/* Pass zero for FROM_TTY, because the action of loading the
@@ -193,7 +193,7 @@ add_vsyscall_page (struct target_ops *target, int from_tty)
symbol_file_add_from_memory (bfd,
vsyscall_range.start,
vsyscall_range.length,
- name,
+ name.c_str (),
0 /* from_tty */);
}
catch (const gdb_exception &ex)
diff --git a/ld/emultempl/pe.em b/ld/emultempl/pe.em
index fe65d2b266..8c5ee76233 100644
--- a/ld/emultempl/pe.em
+++ b/ld/emultempl/pe.em
@@ -1523,7 +1523,6 @@ gld_${EMULATION_NAME}_after_open (void)
struct bfd_symbol *s;
struct bfd_link_hash_entry * blhe;
const char *other_bfd_filename;
- char *n;
s = (relocs[i]->sym_ptr_ptr)[0];
@@ -1550,9 +1549,9 @@ gld_${EMULATION_NAME}_after_open (void)
continue;
/* Rename this implib to match the other one. */
- n = xmalloc (strlen (other_bfd_filename) + 1);
- strcpy (n, other_bfd_filename);
- bfd_set_filename (is->the_bfd->my_archive, n);
+ if (!bfd_set_filename (is->the_bfd->my_archive,
+ other_bfd_filename))
+ einfo ("%F%P: %pB: %E\n", is->the_bfd);
}
free (relocs);
@@ -1655,28 +1654,14 @@ gld_${EMULATION_NAME}_after_open (void)
else /* sentinel */
seq = 'c';
- /* PR 25993: It is possible that is->the_bfd-filename == is->filename.
- In which case calling bfd_set_filename on one will free the memory
- pointed to by the other. */
- if (is->filename == bfd_get_filename (is->the_bfd))
- {
- new_name = xmalloc (strlen (is->filename) + 3);
- sprintf (new_name, "%s.%c", is->filename, seq);
- bfd_set_filename (is->the_bfd, new_name);
- is->filename = new_name;
- }
- else
- {
- new_name
- = xmalloc (strlen (bfd_get_filename (is->the_bfd)) + 3);
- sprintf (new_name, "%s.%c",
- bfd_get_filename (is->the_bfd), seq);
- bfd_set_filename (is->the_bfd, new_name);
-
- new_name = xmalloc (strlen (is->filename) + 3);
- sprintf (new_name, "%s.%c", is->filename, seq);
- is->filename = new_name;
- }
+ new_name
+ = xmalloc (strlen (bfd_get_filename (is->the_bfd)) + 3);
+ sprintf (new_name, "%s.%c",
+ bfd_get_filename (is->the_bfd), seq);
+ is->filename = bfd_set_filename (is->the_bfd, new_name);
+ free (new_name);
+ if (!is->filename)
+ einfo ("%F%P: %pB: %E\n", is->the_bfd);
}
}
}
diff --git a/ld/emultempl/pep.em b/ld/emultempl/pep.em
index 699b86501c..ea8e768ea9 100644
--- a/ld/emultempl/pep.em
+++ b/ld/emultempl/pep.em
@@ -1491,7 +1491,6 @@ gld_${EMULATION_NAME}_after_open (void)
struct bfd_symbol *s;
struct bfd_link_hash_entry * blhe;
const char *other_bfd_filename;
- char *n;
s = (relocs[i]->sym_ptr_ptr)[0];
@@ -1518,9 +1517,9 @@ gld_${EMULATION_NAME}_after_open (void)
continue;
/* Rename this implib to match the other one. */
- n = xmalloc (strlen (other_bfd_filename) + 1);
- strcpy (n, other_bfd_filename);
- bfd_set_filename (is->the_bfd->my_archive, n);
+ if (!bfd_set_filename (is->the_bfd->my_archive,
+ other_bfd_filename))
+ einfo ("%F%P: %pB: %E\n", is->the_bfd);
}
free (relocs);
@@ -1623,28 +1622,14 @@ gld_${EMULATION_NAME}_after_open (void)
else /* sentinel */
seq = 'c';
- /* PR 25993: It is possible that is->the_bfd-filename == is->filename.
- In which case calling bfd_set_filename on one will free the memory
- pointed to by the other. */
- if (is->filename == bfd_get_filename (is->the_bfd))
- {
- new_name = xmalloc (strlen (is->filename) + 3);
- sprintf (new_name, "%s.%c", is->filename, seq);
- bfd_set_filename (is->the_bfd, new_name);
- is->filename = new_name;
- }
- else
- {
- new_name
- = xmalloc (strlen (bfd_get_filename (is->the_bfd)) + 3);
- sprintf (new_name, "%s.%c",
- bfd_get_filename (is->the_bfd), seq);
- bfd_set_filename (is->the_bfd, new_name);
-
- new_name = xmalloc (strlen (is->filename) + 3);
- sprintf (new_name, "%s.%c", is->filename, seq);
- is->filename = new_name;
- }
+ new_name
+ = xmalloc (strlen (bfd_get_filename (is->the_bfd)) + 3);
+ sprintf (new_name, "%s.%c",
+ bfd_get_filename (is->the_bfd), seq);
+ is->filename = bfd_set_filename (is->the_bfd, new_name);
+ free (new_name);
+ if (!is->filename)
+ einfo ("%F%P: %pB: %E\n", is->the_bfd);
}
}
}
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PR25993, read of freed memory
2020-05-19 23:40 ` Alan Modra
@ 2020-05-20 0:19 ` Simon Marchi
2020-05-20 1:51 ` Alan Modra
0 siblings, 1 reply; 10+ messages in thread
From: Simon Marchi @ 2020-05-20 0:19 UTC (permalink / raw)
To: Alan Modra; +Cc: binutils, gdb-patches
On 2020-05-19 7:40 p.m., Alan Modra wrote:
> On Tue, May 19, 2020 at 09:27:15AM -0400, Simon Marchi wrote:
>> On 2020-05-19 12:32 a.m., Alan Modra via Gdb-patches wrote:
>>> -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.
> Yes, I've updated the return type. One minor change needed to
> mach-o.c.
>
>> Since the string gets copied by bfd_set_filename, let's use std::string
>> to avoid having to free:
> Done, and symfile-mem.c updated as per down-thread suggestion to make
> name a const char*.
>
> I've left the return status from bfd_set_filename in gdb unchecked,
> ie. the out-of-memory NULL return, since it seems to me that not
> getting the expected name change is a minor detail very likely to be
> lost in some later OOM.
That LGTM for the GDB side, but I get a build failure in bfd/archive.c. I think
bfd-in2.h needs to be regenerated? Or maybe in BFD you don't typically include
re-generated files in your patches?
Simon
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: PR25993, read of freed memory
2020-05-20 0:19 ` Simon Marchi
@ 2020-05-20 1:51 ` Alan Modra
0 siblings, 0 replies; 10+ messages in thread
From: Alan Modra @ 2020-05-20 1:51 UTC (permalink / raw)
To: Simon Marchi; +Cc: binutils, gdb-patches
On Tue, May 19, 2020 at 08:19:08PM -0400, Simon Marchi wrote:
> That LGTM for the GDB side, but I get a build failure in bfd/archive.c. I think
> bfd-in2.h needs to be regenerated? Or maybe in BFD you don't typically include
> re-generated files in your patches?
Yes, I stripped that out when posting the patch. Thanks for reviewing!
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-05-20 1:51 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-19 4:32 PR25993, read of freed memory Alan Modra
2020-05-19 13:27 ` Simon Marchi
2020-05-19 17:25 ` Christian Biesinger
2020-05-19 17:26 ` Simon Marchi
2020-05-19 18:58 ` Christian Biesinger
2020-05-19 19:37 ` Pedro Alves
2020-05-19 19:38 ` Simon Marchi
2020-05-19 23:40 ` Alan Modra
2020-05-20 0:19 ` Simon Marchi
2020-05-20 1:51 ` Alan Modra
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).