* 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).