From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.120]) by sourceware.org (Postfix) with ESMTP id C141A3858D34 for ; Thu, 25 Jun 2020 14:11:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org C141A3858D34 Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-370-KqQbu86lOHCJNeRrGIaxXQ-1; Thu, 25 Jun 2020 10:11:19 -0400 X-MC-Unique: KqQbu86lOHCJNeRrGIaxXQ-1 Received: by mail-wm1-f72.google.com with SMTP id g187so6895028wme.0 for ; Thu, 25 Jun 2020 07:11:19 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=E/Er/iF6ccbppYCNdzBVQ3fmM0Ete8hgDTLUnTWkpIA=; b=mDX9dScs/poISpga/5Mpbo+5b1GTMdC4KsFPZKCfbgZEpHvYMWGKc2tPpuuQYlKe0x Dd3EbyathhMe7O2kpGzpPXOgsnVfR4w4N733qvqgTC6BL04EgzzXLTvS+hOu8+71YYzq ho6TSyucK4Gy9i/rbPtcGxlCJKV8I+4vBtQkaIjdtM7JQXNZmsUrPPPPGSfP2/28h4sW rkgIoir/oAe0dI0zKVysDNfGDjgZspjVo+Mc2dEQEjLBbwGIFm3ZRKjoncoMxoWGPzqq AvN8KtFqiF2VOEcs6/tvnlcrbo4RxmOaU/ffeu/QeA/cTArQOCqmzXAe8eEfM9m7FweE sRfg== X-Gm-Message-State: AOAM531I4kHPxqH8RtsReInFz7f+jolkyOPX+ZXnqJLXXNTg8A8yD8/D FtmE/FDV6yTZkcbWNjblCVyI5GBo4Pf4Gyp6yOPFBpYyfhTjYl+7rl4CXRXCN+wr0C1+zh6tG/u otKyJ7PbSwbdMxsum1ganRg== X-Received: by 2002:adf:f082:: with SMTP id n2mr31231041wro.326.1593094277732; Thu, 25 Jun 2020 07:11:17 -0700 (PDT) X-Google-Smtp-Source: ABdhPJz6WzK6mD7owoB9ga/5wIt+UYoehvMIc0jsczYu1keh+Lw2p68Symb689Yhlig8FL/LvahRVA== X-Received: by 2002:adf:f082:: with SMTP id n2mr31231004wro.326.1593094277221; Thu, 25 Jun 2020 07:11:17 -0700 (PDT) Received: from ?IPv6:2001:8a0:f922:c400:56ee:75ff:fe8d:232b? ([2001:8a0:f922:c400:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id y6sm13262935wmy.0.2020.06.25.07.11.16 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 25 Jun 2020 07:11:16 -0700 (PDT) Subject: Re: [PATCH v3 7/9] Use NT_FILE note section for reading core target memory To: Kevin Buettner , gdb-patches@sourceware.org References: <20200618040824.3405657-1-kevinb@redhat.com> <20200618040824.3405657-8-kevinb@redhat.com> From: Pedro Alves Message-ID: <98a72eab-3bd2-a136-8e24-6a090020baa3@redhat.com> Date: Thu, 25 Jun 2020 15:11:13 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20200618040824.3405657-8-kevinb@redhat.com> Content-Language: en-US X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-8.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 25 Jun 2020 14:11:29 -0000 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 > 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 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