[PATCH] Work around incorrect/broken pathnames in NT_FILE note

[Kevin Buettner]

Hi Luis,

That new failure is expected in your docker environment.  It might
be possible to XFAIL it.  We need to be able to detect that we're
running in docker with the AUFS storage driver.  Since we know that
the kernel is leaking docker host paths, we should see these when the
"info proc mappings" command is used with a loaded core file.  I'll
give it a try...

Thanks for testing out my patch.

Kevin

On Sat, 8 Aug 2020 07:19:23 -0300
Luis Machado <luis.machado@linaro.org> wrote:

> Hi Kevin,
> 
> Thanks for working on this. I gave this a try and the failures below are 
> gone, but there is a new one that showed up for gdb.base/corefile.exp:
> 
> FAIL: gdb.base/corefile.exp: core-file warning-free
> 
> I'll take a look at it later.
> 
> gdb.base/corefile2.exp came out clean though.
> 
> On 8/7/20 7:20 PM, Kevin Buettner wrote:
> > Luis Machado reported some regressions after I pushed recent core file
> > related patches fixing BZ 25631:
> > 
> >      FAIL: gdb.base/corefile.exp: backtrace in corefile.exp
> >      FAIL: gdb.base/corefile.exp: core-file warning-free
> >      FAIL: gdb.base/corefile.exp: print func2::coremaker_local
> >      FAIL: gdb.base/corefile.exp: up in corefile.exp
> >      FAIL: gdb.base/corefile.exp: up in corefile.exp (reinit)
> > 
> > While I haven't been able to reproduce these failures, I think I
> > understand why they're happening.  It is my hope that this commit will
> > fix those regressions.
> > 
> > Luis is testing in a docker container which is using the AUFS storage
> > driver.  It turns out that the kernel is placing docker host paths in
> > the NT_FILE note instead of paths within the container.
> > 
> > I've made a similar docker environment (though apparently not similar
> > enough to reproduce the regressions).  This is one of the paths that
> > I see mentioned in the warning messages printed while loading the
> > core file during NT_FILE note processing - note that I've shortened
> > the path component starting with "d07c4":
> > 
> > /var/lib/docker/aufs/diff/d07c4...21/lib/x86_64-linux-gnu/ld-2.27.so
> > 
> > This is a path on the docker host; it does not exist in the
> > container.  In the docker container, this is the path:
> > 
> > /lib/x86_64-linux-gnu/ld-2.27.so
> > 
> > My first thought was to disable all NT_FILE mappings when any path was
> > found to be bad.  This would have caused GDB to fall back to accessing
> > memory using the file stratum as it did before I added the NT_FILE
> > note loading code.  After further consideration, I realized that we
> > could do better than this.  For file-backed memory access, we can
> > still use the NT_FILE mappings when available, and then attempt to
> > access memory using the file stratum constrainted to those address
> > ranges corresponding to the "broken" mappings.
> > 
> > In order to test it, I made some additions to corefile2.exp in which
> > the test case's executable is renamed.  The core file is then loaded;
> > due to the fact that the executable has been renamed, those mappings
> > will be unavailable.  After loading the core file, the executable is
> > renamed back to its original name at which point it is loaded using
> > GDB's "file" command.  The "interesting" tests are then run.  These
> > tests will print out values in file-backed memory regions along with
> > mmap'd regions placed within/over the file-backed regions.  Despite
> > the fact that the executable could not be found during the NT_FILE
> > note processing, these tests still work correctly due to the fact that
> > memory is availble from the file stratum combined with the fact that
> > the broken NT_FILE mappings are used to prevent file-backed access
> > outside of the "broken" mappings.
> > 
> > gdb/ChangeLog:
> > 
> > 	* corelow.c (class core_target): Add field
> > 	'm_core_unavailable_mappings'.
> > 	(core_target::build_file_mappings): Print only one warning
> > 	per inaccessible file.  Add unavailable/broken mappings
> > 	to m_core_unavailable_mappings.
> > 	(core_target::xfer_partial): Call...
> > 	(core_target::xfer_memory_via_mappings): New method.
> > 
> > gdb/testsuite/ChangeLog:
> > 
> > 	* gdb.base/corefile2.exp (renamed binfile): New tests.
> > ---
> >   gdb/corelow.c                        | 84 +++++++++++++++++++++++++---
> >   gdb/testsuite/gdb.base/corefile2.exp | 35 ++++++++++++
> >   2 files changed, 111 insertions(+), 8 deletions(-)
> > 
> > diff --git a/gdb/corelow.c b/gdb/corelow.c
> > index b6ee219f57..2e6be74a24 100644
> > --- a/gdb/corelow.c
> > +++ b/gdb/corelow.c
> > @@ -131,9 +131,21 @@ class core_target final : public process_stratum_target
> >        information about memory mapped files.  */
> >     target_section_table m_core_file_mappings {};
> >   
> > +  /* Unavailable mappings.  These correspond to pathnames which either
> > +     weren't found or could not be opened.  Knowing these addresses can
> > +     still be useful.  */
> > +  std::vector<mem_range> m_core_unavailable_mappings;
> > +
> >     /* Build m_core_file_mappings.  Called from the constructor.  */
> >     void build_file_mappings ();
> >   
> > +  /* Helper method for xfer_partial.  */
> > +  enum target_xfer_status xfer_memory_via_mappings (gdb_byte *readbuf,
> > +                                                    const gdb_byte *writebuf,
> > +						    ULONGEST offset,
> > +						    ULONGEST len,
> > +						    ULONGEST *xfered_len);
> > +
> >     /* FIXME: kettenis/20031023: Eventually this field should
> >        disappear.  */
> >     struct gdbarch *m_core_gdbarch = NULL;
> > @@ -182,6 +194,7 @@ void
> >   core_target::build_file_mappings ()
> >   {
> >     std::unordered_map<std::string, struct bfd *> bfd_map;
> > +  std::unordered_map<std::string, bool> unavailable_paths;
> >   
> >     /* See linux_read_core_file_mappings() in linux-tdep.c for an example
> >        read_core_file_mappings method.  */
> > @@ -216,9 +229,13 @@ core_target::build_file_mappings ()
> >   	      = exec_file_find (filename, NULL);
> >   	    if (expanded_fname == nullptr)
> >   	      {
> > -		warning (_("Can't open file %s during file-backed mapping "
> > -			   "note processing"),
> > -			 filename);
> > +		m_core_unavailable_mappings.emplace_back (start, end - start);
> > +		if (!unavailable_paths[filename])
> > +		  warning (_("Can't open file %s during file-backed mapping "
> > +			     "note processing"),
> > +			   filename);
> > +		/* Print just one warning per path.  */
> > +		unavailable_paths[filename] = true;
> >   		return;
> >   	      }
> >   
> > @@ -227,6 +244,7 @@ core_target::build_file_mappings ()
> >   
> >   	    if (bfd == nullptr || !bfd_check_format (bfd, bfd_object))
> >   	      {
> > +		m_core_unavailable_mappings.emplace_back (start, end - start);
> >   		/* If we get here, there's a good chance that it's due to
> >   		   an internal error.  We issue a warning instead of an
> >   		   internal error because of the possibility that the
> > @@ -268,6 +286,8 @@ core_target::build_file_mappings ()
> >   	ts->owner = nullptr;
> >   	ts->the_bfd_section = sec;
> >         });
> > +
> > +  normalize_mem_ranges (&m_core_unavailable_mappings);
> >   }
> >   
> >   static void add_to_thread_list (bfd *, asection *, void *);
> > @@ -728,6 +748,57 @@ core_target::files_info ()
> >     print_section_info (&m_core_section_table, core_bfd);
> >   }
> >   

> > +/* Helper method for core_target::xfer_partial.  */
> > +
> > +enum target_xfer_status
> > +core_target::xfer_memory_via_mappings (gdb_byte *readbuf,
> > +				       const gdb_byte *writebuf,
> > +				       ULONGEST offset, ULONGEST len,
> > +				       ULONGEST *xfered_len)
> > +{
> > +  enum target_xfer_status xfer_status;
> > +
> > +  xfer_status = section_table_xfer_memory_partial
> > +		  (readbuf, writebuf,
> > +		   offset, len, xfered_len,
> > +		   m_core_file_mappings.sections,
> > +		   m_core_file_mappings.sections_end);
> > +
> > +  if (xfer_status == TARGET_XFER_OK || m_core_unavailable_mappings.empty ())
> > +    return xfer_status;
> > +
> > +  /* There are instances - e.g. when debugging within a docker
> > +     container using the AUFS storage driver - where the pathnames
> > +     obtained from the note section are incorrect.  Despite the path
> > +     being wrong, just knowing the start and end addresses of the
> > +     mappings is still useful; we can attempt an access of the file
> > +     stratum constrained to the address ranges corresponding to the
> > +     unavailable mappings.  */
> > +
> > +  ULONGEST memaddr = offset;
> > +  ULONGEST memend = offset + len;
> > +
> > +  for (const auto &mr : m_core_unavailable_mappings)
> > +    {
> > +      if (address_in_mem_range (memaddr, &mr))
> > +        {
> > +	  if (!address_in_mem_range (memend, &mr))
> > +	    len = mr.start + mr.length - memaddr;
> > +
> > +	  xfer_status = this->beneath ()->xfer_partial (TARGET_OBJECT_MEMORY,
> > +							NULL,
> > +							readbuf,
> > +							writebuf,
> > +							offset,
> > +							len,
> > +							xfered_len);
> > +	  break;
> > +	}
> > +    }
> > +
> > +  return xfer_status;
> > +}
> > +
> >   enum target_xfer_status
> >   core_target::xfer_partial (enum target_object object, const char *annex,
> >   			   gdb_byte *readbuf, const gdb_byte *writebuf,
> > @@ -761,11 +832,8 @@ core_target::xfer_partial (enum target_object object, const char *annex,
> >   	   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);
> > +	  xfer_status = xfer_memory_via_mappings (readbuf, writebuf, offset,
> > +						  len, xfered_len);
> >   	else
> >   	  xfer_status = this->beneath ()->xfer_partial (object, annex, readbuf,
> >   							writebuf, offset, len,
> > diff --git a/gdb/testsuite/gdb.base/corefile2.exp b/gdb/testsuite/gdb.base/corefile2.exp
> > index 5de7ead4d4..11915a6185 100644
> > --- a/gdb/testsuite/gdb.base/corefile2.exp
> > +++ b/gdb/testsuite/gdb.base/corefile2.exp
> > @@ -143,6 +143,41 @@ gdb_test_multiple $test "" {
> >       }
> >   }
> >   
> > +# Test again with executable renamed during loading of core file.
> > +
> > +with_test_prefix "renamed binfile" {
> > +    # We don't use clean_restart here since we want to defer loading
> > +    # of $binfile until after the core file has been loaded.  (BFD
> > +    # will complain that $binfile has disappeared after the rename
> > +    # if it's loaded first.)
> > +    gdb_exit
> > +    gdb_start
> > +    gdb_reinitialize_dir $srcdir/$subdir
> > +
> > +    # Rename $binfile so that it won't be found during loading of
> > +    # the core file.
> > +    set hide_binfile [standard_output_file "${testfile}.hide"]
> > +    remote_exec host "mv -f $binfile $hide_binfile"
> > +
> > +    # Load core file - check that a warning is printed.
> > +    global xfail
> > +    if { $xfail } { setup_xfail "*-*-*" }
> > +    gdb_test_multiple "core-file $corefile" $test {
> > +	-re "warning: Can't open file.*during.* note processing.*Core was generated by .*\#0  .*\(\).*\r\n$gdb_prompt $" {
> > +	    pass $test
> > +	}
> > +	-re "\r\n$gdb_prompt $" {
> > +	    fail $test
> > +	}
> > +    }
> > +
> > +    # Restore $binfile and then load it.
> > +    remote_exec host "mv -f $hide_binfile $binfile"
> > +    gdb_load ${binfile}
> > +
> > +    do_tests
> > +}
> > +
> >   # Restart and run to the abort call.
> >   
> >   clean_restart $binfile
> >   
>