[PATCH v3 7/9] Use NT_FILE note section for reading core target memory
Pedro Alves
palves@redhat.com
Thu Jun 25 14:11:13 GMT 2020
On 6/18/20 5:08 AM, Kevin Buettner via Gdb-patches wrote:
> In his reviews of my v1 and v2 corefile related patches, Pedro
> identified two cases which weren't handled by those patches.
>
> In https://sourceware.org/pipermail/gdb-patches/2020-May/168826.html,
> Pedro showed that debugging a core file in which mmap() is used to
> create a new mapping over an existing file-backed mapping will
> produce incorrect results. I.e, for his example, GDB would
> show:
>
> (gdb) disassemble main
> Dump of assembler code for function main:
> 0x00000000004004e6 <+0>: push %rbp
> 0x00000000004004e7 <+1>: mov %rsp,%rbp
> => 0x00000000004004ea <+4>: callq 0x4003f0 <abort@plt>
> End of assembler dump.
>
> This sort of looks like it might be correct, but is not due to the
> fact that mmap(...MAP_FIXED...) was used to create a mapping (of all
> zeros) on top of the .text section. So, the correct result should be:
>
> (gdb) disassemble main
> Dump of assembler code for function main:
> 0x00000000004004e6 <+0>: add %al,(%rax)
> 0x00000000004004e8 <+2>: add %al,(%rax)
> => 0x00000000004004ea <+4>: add %al,(%rax)
> 0x00000000004004ec <+6>: add %al,(%rax)
> 0x00000000004004ee <+8>: add %al,(%rax)
> End of assembler dump.
>
> The other case that Pedro found involved an attempted examination of a
> particular section in the test case from gdb.base/corefile.exp. On
> Fedora 27 or 28, the following behavior may be observed:
>
> (gdb) info proc mappings
> Mapped address spaces:
>
> Start Addr End Addr Size Offset objfile
> ...
> 0x7ffff7839000 0x7ffff7a38000 0x1ff000 0x1b5000 /usr/lib64/libc-2.27.so
> ...
> (gdb) x/4x 0x7ffff7839000
> 0x7ffff7839000: Cannot access memory at address 0x7ffff7839000
>
> FYI, this section appears to be unrelocated vtable data. See
> https://sourceware.org/pipermail/gdb-patches/2020-May/168331.html for
> a detailed analysis.
>
> The important thing here is that GDB should be able to access this
> address since it should be backed by the shared library. I.e. it
> should do this:
>
> (gdb) x/4x 0x7ffff7839000
> 0x7ffff7839000: 0x0007ddf0 0x00000000 0x0007dba0 0x00000000
>
> Both of these cases are fixed with this commit.
>
> In a nutshell, this commit opens a "binary" target BFD for each of the
> files that are mentioned in an NT_FILE / .note.linuxcore.file note
> section. It then uses these mappings instead of the file stratum
> mappings that GDB has used in the past.
>
> If this note section doesn't exist or is mangled for some reason, then
> GDB will use the file stratum as before. Should this happen, then
> we can expect both of the above problems to again be present.
>
> See the code comments in the commit for other details.
>
> gdb/ChangeLog:
>
> * corelow.c (complaints.h): Include.
> (class core_target): Add field m_core_file_mappings and
> method build_file_mappings.
> (core_target::core_target): Call build_file_mappings.
> (core_target::~core_target): Free memory associated with
> m_core_file_mappings.
> (core_target::build_file_mappings): New method.
> (core_target::xfer_partial): Use m_core_file_mappings
> for memory transfers.
> ---
> gdb/corelow.c | 199 +++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 195 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/corelow.c b/gdb/corelow.c
> index 17e862aee9..846d4f2164 100644
> --- a/gdb/corelow.c
> +++ b/gdb/corelow.c
> @@ -45,6 +45,7 @@
> #include "gdbsupport/filestuff.h"
> #include "build-id.h"
> #include "gdbsupport/pathstuff.h"
> +#include "complaints.h"
>
> #ifndef O_LARGEFILE
> #define O_LARGEFILE 0
> @@ -121,9 +122,17 @@ class core_target final : public process_stratum_target
> targets. */
> target_section_table m_core_section_table {};
>
> + /* File-backed address space mappings: some core files include
> + information about memory mapped files. */
> + target_section_table m_core_file_mappings {};
> +
> + /* Build m_core_file_mappings. Called from the constructor. */
> + void build_file_mappings ();
> +
> /* FIXME: kettenis/20031023: Eventually this field should
> disappear. */
> struct gdbarch *m_core_gdbarch = NULL;
> +
Spurious whitespace.
> };
>
> core_target::core_target ()
> @@ -141,11 +150,182 @@ core_target::core_target ()
> &m_core_section_table.sections_end))
> error (_("\"%s\": Can't find sections: %s"),
> bfd_get_filename (core_bfd), bfd_errmsg (bfd_get_error ()));
> +
> + build_file_mappings ();
> }
>
> core_target::~core_target ()
> {
> xfree (m_core_section_table.sections);
> + xfree (m_core_file_mappings.sections);
> +}
> +
> +/* Construct the target_section_table for file-based mappings if
> + they exist.
> +
> + At the moment, we only handle mapping descriptions from the note
> + .note.linuxcore.file. The Linux kernel sources document the format
> + of the note, NT_FILE, in fs/binfmt_elf.c:
> +
> + long count -- how many files are mapped
> + long page_size -- units for file_ofs
> + array of [COUNT] elements of
> + long start
> + long end
> + long file_ofs
> + followed by COUNT filenames in ASCII: "FILE1" NUL "FILE2" NUL...
> +
> + For each unique path in the note, we'll open a BFD with a bfd
> + target of "binary". This is an unstructured bfd target upon which
> + we'll impose a structure from the mappings in the NT_FILE note.
> + Thus, we'll allocate and initialize a BFD section for each mapping.
> +
> + We take care to not share already open bfds with other parts of
> + GDB; in particular, we don't want to add new sections to existing
> + BFDs. We do, however, ensure that the BFDs that we allocate here
> + will go away (be deallocated) when the core target is detached. */
> +
> +void
> +core_target::build_file_mappings ()
> +{
> + struct gdbarch *gdbarch = m_core_gdbarch;
> +
> + /* Ensure that ULONGEST is big enough for reading 64-bit core files. */
> + gdb_static_assert (sizeof (ULONGEST) >= 8);
> +
> + /* It's not required that the NT_FILE note exists, so return silently
> + if it's not found. Beyond this point though, we'll complain
> + if problems are found. */
> + asection *section = bfd_get_section_by_name (core_bfd,
> + ".note.linuxcore.file");
> + if (section == nullptr)
> + return;
> +
> + unsigned int addr_size_bits = gdbarch_addr_bit (gdbarch);
> + unsigned int addr_size = addr_size_bits / 8;
> + size_t note_size = bfd_section_size (section);
> +
> + if (note_size < 2 * addr_size)
> + {
> + complaint (_("malformed core note - too short for header"));
> + return;
> + }
> +
> + gdb::def_vector<gdb_byte> contents (note_size);
> + if (!bfd_get_section_contents (core_bfd, section, contents.data (),
> + 0, note_size))
> + {
> + complaint (_("could not get core note contents"));
> + return;
> + }
> +
> + gdb_byte *descdata = contents.data ();
> + char *descend = (char *) descdata + note_size;
> +
> + if (descdata[note_size - 1] != '\0')
> + {
> + complaint (_("malformed note - does not end with \\0"));
> + return;
> + }
> +
> + ULONGEST count = bfd_get (addr_size_bits, core_bfd, descdata);
> + descdata += addr_size;
> +
> + ULONGEST page_size = bfd_get (addr_size_bits, core_bfd, descdata);
> + descdata += addr_size;
> +
> + if (note_size < 2 * addr_size + count * 3 * addr_size)
> + {
> + complaint (_("malformed note - too short for supplied file count"));
> + return;
> + }
> +
> + char *filenames = (char *) descdata + count * 3 * addr_size;
> +
> + /* Make sure that the correct number of filenames exist. Complain
> + if there aren't enough or are too many. Also, find out how
> + many unique names there are. */
> + unsigned long ucount = 0;
> + char *prev_fname = nullptr;
> + char *f = filenames;
> + for (int i = 0; i < count; i++)
> + {
> + if (f >= descend)
> + {
> + complaint (_("malformed note - filename area is too small"));
> + return;
> + }
> + if (prev_fname == nullptr || strcmp (prev_fname, f) != 0)
> + ucount++;
> + f += strnlen (f, descend - f) + 1;
> + }
Is it assuming that you can't see a mapping with interleaved files,
like:
FileA
FileB
FileA
?
I'm not sure that's a generally safe assumption.
> + /* Complain, but don't return early if the filename area is too big. */
> + if (f != descend)
> + complaint (_("malformed note - filename area is too big"));
> +
> + m_core_file_mappings.sections = XNEWVEC (struct target_section, ucount);
> + m_core_file_mappings.sections_end = m_core_file_mappings.sections;
> +
> + struct bfd *prev_bfd = nullptr;
> + prev_fname = nullptr;
> + for (int i = 0; i < count; i++)
> + {
> + ULONGEST start = bfd_get (addr_size_bits, core_bfd, descdata);
> + descdata += addr_size;
> + ULONGEST end = bfd_get (addr_size_bits, core_bfd, descdata);
> + descdata += addr_size;
> + ULONGEST file_ofs = bfd_get (addr_size_bits, core_bfd, descdata)
> + * page_size;
Wrap in parens so that emacs indents this correctly:
ULONGEST file_ofs = (bfd_get (addr_size_bits, core_bfd, descdata)
* page_size);
Alternatively, break before the =, like:
ULONGEST file_ofs
= bfd_get (addr_size_bits, core_bfd, descdata) * page_size;
> + descdata += addr_size;
I was surprised to see that this section parsing code wasn't shared
with linux_core_info_proc_mappings. I was imagining that that
could be factored out into a function that takes a callback
as parameter, and calls the callback for each mapping entry.
I suppose I'm OK with this as is if that's too hard or ugly for
some reason.
> +
> + struct bfd *bfd;
> + if (prev_fname != nullptr && strcmp (prev_fname, filenames) == 0)
> + bfd = prev_bfd;
> + else
> + bfd = bfd_openr (filenames, "binary");
One issue I see here is that this ignores the sysroot. I.e.,
if you have "set sysroot /whatever", this should open the
file under the sysroot, not on the filesystem. It also
needs to handle the "target:" sysroot, because bfd doesn't
understand that. gdb_bfd_open handles it, but I'm not sure
you can use it here. Since this is known to be a core file, maybe
you can just skip the initial "target:".
> + if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))
> + {
> + if (bfd != prev_bfd)
> + warning (_("Can't open file %s"), filenames);
> + prev_bfd = bfd;
> + continue;
> + }
> +
> + /* Ensure that the bfd will be closed when core_bfd is closed.
> + This can be checked before/after a core file detach via
> + "maint info bfds". */
> + if (prev_bfd != bfd)
> + gdb_bfd_record_inclusion (core_bfd, bfd);
> +
> + prev_bfd = bfd;
> + prev_fname = filenames;
> +
> + /* Make new BFD section. */
> + char secnamebuf[64];
> + sprintf (secnamebuf, "S%04d", i);
> + char *secname = (char *) bfd_alloc (bfd, strlen (secnamebuf) + 1);
> + if (secname == nullptr)
> + error (_("Out of memory"));
> + strcpy (secname, secnamebuf);
> + asection *sec = bfd_make_section_anyway (bfd, secname);
> + if (sec == nullptr)
> + error (_("Can't make section"));
> + sec->filepos = file_ofs;
> + bfd_set_section_flags (sec, SEC_READONLY | SEC_HAS_CONTENTS);
> + bfd_set_section_size (sec, end - start);
> + bfd_set_section_vma (sec, start);
> + bfd_set_section_lma (sec, start);
> + bfd_set_section_alignment (sec, 2);
> +
> + /* Set target_section fields. */
> + struct target_section *ts = m_core_file_mappings.sections_end++;
> + ts->addr = start;
> + ts->endaddr = end;
> + ts->owner = nullptr;
> + ts->the_bfd_section = sec;
> +
> + filenames += strlen ((char *) filenames) + 1;
> + }
> }
>
> static void add_to_thread_list (bfd *, asection *, void *);
> @@ -632,10 +812,21 @@ core_target::xfer_partial (enum target_object object, const char *annex,
> if (xfer_status == TARGET_XFER_OK)
> return TARGET_XFER_OK;
>
> - /* Now check the stratum beneath us; this should be file_stratum. */
> - xfer_status = this->beneath ()->xfer_partial (object, annex, readbuf,
> - writebuf, offset, len,
> - xfered_len);
> + /* Check file backed mappings. If they're available, use
> + core file provided mappings (e.g. from .note.linuxcore.file
> + or the like) as this should provide a more accurate
> + result. If not, check the stratum beneath us, which should
> + be the file stratum. */
> + if (m_core_file_mappings.sections != nullptr)
> + xfer_status = section_table_xfer_memory_partial
> + (readbuf, writebuf,
> + offset, len, xfered_len,
> + m_core_file_mappings.sections,
> + m_core_file_mappings.sections_end);
> + else
> + xfer_status = this->beneath ()->xfer_partial (object, annex, readbuf,
> + writebuf, offset, len,
> + xfered_len);
> if (xfer_status == TARGET_XFER_OK)
> return TARGET_XFER_OK;
>
>
Thanks,
Pedro Alves
More information about the Gdb-patches
mailing list