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

[Pedro Alves <pedro@palves.net>]

Hi Kevin,

This LGTM, with a few minor issues pointed out below addressed.

On 8/7/20 11:20 PM, Kevin Buettner via Gdb-patches 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

"availAble"

> 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);

Seems like a tabs vs spaces mixup here.

> +
>    /* 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;
>  

And unordered_map to store a boolean that means "I've seen it"
sounds like the "wrong" data structure.  A more natural fit
would be:

  std::unordered_set<std::string> unavailable_paths;

Then either the string is in the set or is isn't.

>    /* 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;

and then:

		/* Print just one warning per path.  */
		if (unavailable_paths.insert (filename).second)
		  warning (_("Can't open file %s during file-backed mapping "
			     "note processing"),
			   filename);

unordered_set::insert does not insert if the element already exists,
and it returns a pair where you can check on the second field whether
something was inserted or not.

>  		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

Now that we're expecting to reach here, the comment about an internal
error seems stale.

> @@ -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);
>  }
>  \f
> +/* 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);

Wrap in parens so that emacs will indent this correctly:

  xfer_status = (section_table_xfer_memory_partial
		 (readbuf, writebuf,
		  offset, len, xfered_len,
		  m_core_file_mappings.sections,
		  m_core_file_mappings.sections_end));


That's what the GNU standards say to do.


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

Please still call clean_restart, but just don't pass it any argument.
In that case, it skips loading any file.


> +
> +    # 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 {

$test here is still "maint print core-file-backed-mappings".

> +	-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
> +	}

This match is already done internally by gdb_test_multiple.
That suggests that this gdb_test_multiple call could be a
gdb_test call instead.

Thanks,
Pedro Alves


> +    }
> +
> +    # 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
>