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

[Pedro Alves <palves@redhat.com>]

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