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

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Thursday, March 19 2015, Pedro Alves wrote:

> 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

Indeed, this is not true.  I apologize for making this confusion.

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

I totally agree.  As I did with the patch to add the new enum
memory_mapping_state, I am also withdrawing this patch.  I will send a
new series soon.

Thanks,

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/