Re: [PATCH 2/4] Update gcore_create_callback to better handle different states of mappings

Pedro Alves
2015-03-19T10:39:32Z

On 03/18/2015 07:38 PM, Sergio Durigan Junior wrote:
> This patch updates gdb/gcore.c's gcore_create_callback function and
> changes its logic to improve the handling of different states of
> memory mappings.
>
> One of the major modifications is that mappings marked as UNMODIFIED
> are now completely ignored by the function (i.e., not even the segment
> headers are created anymore). This is now possible because of the
> introduction of the new UNKNOWN state (see below). We now know that
> when the mapping is UNMODIFIED, it means that the user has either
> chose to ignore it via /proc/PID/coredump_filter, or that it is marked
> as VM_DONTDUMP (and therefore should not be dumped anyway). This is
> what the Linux kernel does, too.
>
> The new UNKNOWN state is being used to identify situations when we
> don't really know whether the mapping should be dumped or not. Based
> on that, we run an existing heuristic responsible for deciding if we
> should include the mapping's contents or not.
>
> One last thing worth mentioning is that this check:
>
> if (read == 0 && write == 0 && exec == 0
> && modified_state == MEMORY_MAPPING_UNMODIFIED)
>
> has been simplified to:
>
> if (read == 0)
>
> This is because if the mapping has not 'read' permission set, it does
> not make sense to include its contents in the corefile (GDB would
> actually not be allowed to do that).

I don't think this is true. GDB can certainly read memory of
mappings that don't have the read permission bit set: ptrace
bypasses the memory protections. Otherwise, how could gdb poke
breakpoints instructions? AFAICS, it can even read memory mapped
with no permissions at all:

(top-gdb) info inferiors
Num Description Executable
* 1 process 24337 /home/pedro/gdb/mygit/build/gdb/gdb
(top-gdb) shell cat /proc/24337/maps
...
3615fb4000-36161b3000 ---p 001b4000 fd:00 263789 /usr/lib64/libc-2.18.so
...
(top-gdb) x/4b 0x3615fb4000
0x3615fb4000: 0xf6 0x9a 0xf7 0x15

> Therefore, we just create a
> segment header for it. The Linux kernel differs from GDB here because
> it actually include the contents of this mapping in the corefile, but
> this is because it can read its contents.
>
> Changes from v2:
>
> - Return immediately if modified_state == MEMORY_MAPPING_UNMODIFIED
>
> - Improve comments explaining the case above, and also the case when
> "read == 0".
>
> gdb/ChangeLog:
> 2015-03-18 Sergio Durigan Junior <sergiodj@redhat.com>
> Jan Kratochvil <jan.kratochvil@redhat.com>
> Oleg Nesterov <oleg@redhat.com>
>
> PR corefiles/16092
> * gcore.c (gcore_create_callback): Update code to handle the case
> when 'modified_state == MEMORY_MAPPING_UNMODIFIED'. Simplify
> condition used to decide when to create only a segment header for
> the mapping. Improve check to decide when to run a heuristic to
> decide whether to dump the mapping's contents.
> ---
> gdb/gcore.c | 39 +++++++++++++++++++++++++--------------
> 1 file changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/gdb/gcore.c b/gdb/gcore.c
> index 751ddac..8dfcc02 100644
> --- a/gdb/gcore.c
> +++ b/gdb/gcore.c
> @@ -422,23 +422,34 @@ gcore_create_callback (CORE_ADDR vaddr, unsigned long size, int read,
> asection *osec;
> flagword flags = SEC_ALLOC | SEC_HAS_CONTENTS | SEC_LOAD;
>
> - /* If the memory segment has no permissions set, ignore it, otherwise
> - when we later try to access it for read/write, we'll get an error
> - or jam the kernel. */
> - if (read == 0 && write == 0 && exec == 0
> - && modified_state == MEMORY_MAPPING_UNMODIFIED)
> + if (modified_state == MEMORY_MAPPING_UNMODIFIED)
> {
> - if (info_verbose)
> - {
> - fprintf_filtered (gdb_stdout, "Ignore segment, %s bytes at %s\n",
> - plongest (size), paddress (target_gdbarch (), vaddr));
> - }
> -
> + /* When the memory mapping is marked as unmodified, this means
> + that it should not be included in the coredump file (either
> + because it was marked as VM_DONTDUMP, or because the user
> + explicitly chose to ignore it using the
> + /proc/PID/coredump_filter mechanism).
> +
> + We could in theory create a section header for it (i.e., mark
> + it as '~(SEC_LOAD | SEC_HAS_CONTENTS)', just like we do when
> + the mapping does not have the 'read' permission set), but the
> + Linux kernel itself ignores these mappings, and so do we. */
> return 0;

It really seems wrong to me to bake in Linuxisms into this generic
function, shared with all supported configurations. As mentioned
elsewhere, I think we should instead leave it up to the (Linux-specific)
caller to simply not call this function if the mapping shouldn't
be dumped at all.

> }
> -
> - if (write == 0 && modified_state == MEMORY_MAPPING_UNMODIFIED
> - && !solib_keep_data_in_core (vaddr, size))
> + else if (read == 0)
> + {
> + /* If the memory segment has no read permission set, then we have to
> + generate a segment header for it, but without contents (i.e.,
> + FileSiz = 0), otherwise when we later try to access it for
> + read/write, we'll get an error or jam the kernel.
> +
> + The Linux kernel differs from GDB in this point because it
> + can actually read this mapping, so it dumps this mapping's
> + contents in the corefile. */
> + flags &= ~(SEC_LOAD | SEC_HAS_CONTENTS);
> + }
> + else if (write == 0 && modified_state == MEMORY_MAPPING_UNKNOWN_STATE
> + && !solib_keep_data_in_core (vaddr, size))
> {
> /* See if this region of memory lies inside a known file on disk.
> If so, we can avoid copying its contents by clearing SEC_LOAD. */
>

Thanks,
Pedro Alves