RFC: Change readelf/objdump to automatically follow debug links

RFC: Change readelf/objdump to automatically follow debug links

[Nick Clifton]

[-- Attachment #1: Type: text/plain, Size: 899 bytes --]

Hi Guys,

  I received a bug report from a Fedora rawhide user[1] that requests
  that the objdump program should follow debug links automatically when
  it is disassembling code.  The request seems reasonable to me, given
  that many distributions these days make use of separate debug info
  files, so I have created the attached patch.

  The patch adds a new configure time option --enable-follow-debugs-links=[yes|no]
  which can be used to set the default behaviour for both objdump and
  readelf.  If the option is not used, the default is to follow the
  links.

  What do people think ?  Any objections to this idea ?

Cheers
  Nick

PS.  Whilst checking for testsuite regressions with the patch I came
  across a bug in objdump's merging of symbol tables from debug info
  files, so the patch includes a fix for this problem too.

[1]: https://bugzilla.redhat.com/show_bug.cgi?id=1926743


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: bz1926473.patch --]
[-- Type: text/x-patch, Size: 18949 bytes --]

binutils/ChangeLog
2021-02-10  Nick Clifton  <nickc@redhat.com>

	* configure.ac (follow-debug-links): Add option to enable or
	disable the following of debug links by default.  Set the
	default for the option to be 'follow'.
	* dwarf.c (do_follow_links): Initialise with DEFAULT_FOR_FOLLOW_LINKS.
	(dwarf_select_sections_by_names): Add no-follow-links option.
	(dwarf_select_sections_by_letter): Add 'N' option.
	* objdump.c (usage): Add conditional text describing the
	follow links option.
	(slurp_symtab): Ensure that there is a NULL entry at the end
	of the symbol table.
	(slurp_dynamic_symtab): Likewise.
	(dump_bfd): When extending the symbol table, ensure that there
	is still a NULL entry at the end.
	* readelf.c (usage): Add conditional text describing the
	follow links option.
	* doc/binutils.texi: Update documentation for objcopy and
	readelf.
	* doc/debug.options.texi: Update documentation of the
	follow-links option.
	* config.in: Regenerate.
	* configure: Regenerate.
	* testsuite/binutils-all/compress.exp: Add the -WN option to
	objdump command lines that are not expecting to follow links.
	* testsuite/binutils-all/readelf.exp: Add the
	--debug-dump=no-follow-links option to tests that are not
	expecting to follow debug links.

gas/ChangeLog
2021-02-10  Nick Clifton  <nickc@redhat.com>

	* testsuite/gas/mach-o/sections-1.d: Stop automatic debug link
        following.
	* testsuite/gas/xgate/insns-dwarf2.d: Likewise.

ld/ChangeLog
2021-02-10  Nick Clifton  <nickc@redhat.com>

	* testsuite/ld-elf/sec64k.exp: Stop readelf from automatically
	following debug links.

diff --git a/binutils/configure.ac b/binutils/configure.ac
index 6ab85466370..af24c0c9a8a 100644
--- a/binutils/configure.ac
+++ b/binutils/configure.ac
@@ -82,6 +82,20 @@ fi], [default_f_for_ifunc=0])
 AC_DEFINE_UNQUOTED(DEFAULT_F_FOR_IFUNC_SYMBOLS, $default_f_for_ifunc,
 		   [Have nm use F and f for global and local ifunc symbols])
 
+
+AC_ARG_ENABLE(follow-debug-links,
+[AS_HELP_STRING([--enable-follow-debug-links],
+	[Have readelf and objdump follow debug links by default])], [
+if test "${enableval}" = no; then
+  default_for_follow_links=0
+else
+  default_for_follow_links=1
+fi], [default_for_follow_links=1])
+
+AC_DEFINE_UNQUOTED(DEFAULT_FOR_FOLLOW_LINKS, $default_for_follow_links,
+		   [Have readelf and objdump follow debug links by default])
+
+
 AC_DEBUGINFOD
 
 GCC_ENABLE([libctf], [yes], [], [Handle .ctf type-info sections])
diff --git a/binutils/doc/binutils.texi b/binutils/doc/binutils.texi
index 526efe4940d..94ea5720be8 100644
--- a/binutils/doc/binutils.texi
+++ b/binutils/doc/binutils.texi
@@ -2187,8 +2187,10 @@ objdump [@option{-a}|@option{--archive-headers}]
         [@option{-r}|@option{--reloc}]
         [@option{-R}|@option{--dynamic-reloc}]
         [@option{-s}|@option{--full-contents}]
-        [@option{-W[lLiaprmfFsoORtUuTgAckK]}|
-         @option{--dwarf}[=rawline,=decodedline,=info,=abbrev,=pubnames,=aranges,=macro,=frames,=frames-interp,=str,=str-offsets,=loc,=Ranges,=pubtypes,=trace_info,=trace_abbrev,=trace_aranges,=gdb_index,=addr,=cu_index,=links,=follow-links]]
+        [@option{-W[lLiaprmfFsoORtUuTgAck]}|
+         @option{--dwarf}[=rawline,=decodedline,=info,=abbrev,=pubnames,=aranges,=macro,=frames,=frames-interp,=str,=str-offsets,=loc,=Ranges,=pubtypes,=trace_info,=trace_abbrev,=trace_aranges,=gdb_index,=addr,=cu_index,=links]]
+        [@option{-WK}|@option{--dwarf=follow-links}]
+        [@option{-WN}|@option{--dwarf=no-follow-links}]
         [@option{--ctf=}@var{section}]
         [@option{-G}|@option{--stabs}]
         [@option{-t}|@option{--syms}]
@@ -2325,7 +2327,7 @@ will stop at the end of the function, otherwise it will stop when the
 next symbol is encountered.  If there are no matches for @var{symbol}
 then nothing will be displayed.
 
-Note if the @option{--dwarf=follow-links} option has also been enabled
+Note if the @option{--dwarf=follow-links} option is enabled
 then any symbol tables in linked debug info files will be read in and
 used when disassembling.
 
@@ -2347,7 +2349,7 @@ If the target is an ARM architecture this switch also has the effect
 of forcing the disassembler to decode pieces of data found in code
 sections as if they were instructions.
 
-Note if the @option{--dwarf=follow-links} option has also been enabled
+Note if the @option{--dwarf=follow-links} option is enabled
 then any symbol tables in linked debug info files will be read in and
 used when disassembling.
 
@@ -4753,8 +4755,10 @@ readelf [@option{-a}|@option{--all}]
         [@option{-R} <number or name>|@option{--relocated-dump=}<number or name>]
         [@option{-z}|@option{--decompress}]
         [@option{-c}|@option{--archive-index}]
-        [@option{-w[lLiaprmfFsoORtUuTgAckK]}|
-         @option{--debug-dump}[=rawline,=decodedline,=info,=abbrev,=pubnames,=aranges,=macro,=frames,=frames-interp,=str,=str-offsets,=loc,=Ranges,=pubtypes,=trace_info,=trace_abbrev,=trace_aranges,=gdb_index,=addr,=cu_index,=links,=follow-links]]
+        [@option{-w[lLiaprmfFsoORtUuTgAck]}|
+         @option{--debug-dump}[=rawline,=decodedline,=info,=abbrev,=pubnames,=aranges,=macro,=frames,=frames-interp,=str,=str-offsets,=loc,=Ranges,=pubtypes,=trace_info,=trace_abbrev,=trace_aranges,=gdb_index,=addr,=cu_index,=links]]
+        [@option{-wK}|@option{--debug-dump=follow-links}]
+        [@option{-wN}|@option{--debug-dump=no-follow-links}]
         [@option{--dwarf-depth=@var{n}}]
         [@option{--dwarf-start=@var{n}}]
         [@option{--ctf=}@var{section}]
diff --git a/binutils/doc/debug.options.texi b/binutils/doc/debug.options.texi
index 31260c1d19e..2ad8f03626d 100644
--- a/binutils/doc/debug.options.texi
+++ b/binutils/doc/debug.options.texi
@@ -61,6 +61,17 @@ In addition, when displaying DWARF attributes, if a form is found that
 references the separate debug info file, then the referenced contents
 will also be displayed.
 
+Note - in some distributions this option is enabled by default.  It
+can be disabled via the @option{N} debug option.  The default can be
+chosen when configuring the binutils via the
+@option{--enable-follow-debug-links=yes} or
+@option{--enable-follow-debug-links=no} options.  If these are not
+used then the default is to enable the following of debug links.
+
+@item N
+@itemx =no-follow-links
+Disables the following of links to separate debug info files.
+
 @item l
 @itemx =rawline
 Displays the contents of the @samp{.debug_line} section in a raw
diff --git a/binutils/dwarf.c b/binutils/dwarf.c
index d6eb8926dbf..bd60824e9c1 100644
--- a/binutils/dwarf.c
+++ b/binutils/dwarf.c
@@ -99,7 +99,7 @@ int do_debug_addr;
 int do_debug_cu_index;
 int do_wide;
 int do_debug_links;
-int do_follow_links;
+int do_follow_links = DEFAULT_FOR_FOLLOW_LINKS;
 bfd_boolean do_checks;
 
 int dwarf_cutoff_level = -1;
@@ -11343,6 +11343,7 @@ dwarf_select_sections_by_names (const char *names)
       { "links", & do_debug_links, 1 },
       { "loc",  & do_debug_loc, 1 },
       { "macro", & do_debug_macinfo, 1 },
+      { "no-follow-links", & do_follow_links, 0 },
       { "pubnames", & do_debug_pubnames, 1 },
       { "pubtypes", & do_debug_pubtypes, 1 },
       /* This entry is for compatibility
@@ -11372,7 +11373,7 @@ dwarf_select_sections_by_names (const char *names)
 	  if (strncmp (p, entry->option, len) == 0
 	      && (p[len] == ',' || p[len] == '\0'))
 	    {
-	      * entry->variable |= entry->val;
+	      * entry->variable = entry->val;
 
 	      /* The --debug-dump=frames-interp option also
 		 enables the --debug-dump=frames option.  */
@@ -11413,6 +11414,7 @@ dwarf_select_sections_by_letters (const char *letters)
       case 'g':	do_gdb_index = 1; break;
       case 'i':	do_debug_info = 1; break;
       case 'K': do_follow_links = 1; break;
+      case 'N': do_follow_links = 0; break;
       case 'k':	do_debug_links = 1; break;
       case 'l':	do_debug_lines |= FLAG_DEBUG_LINES_RAW;	break;
       case 'L':	do_debug_lines |= FLAG_DEBUG_LINES_DECODED; break;
diff --git a/binutils/objdump.c b/binutils/objdump.c
index 2cd0d84cefd..fde5f59c9ac 100644
--- a/binutils/objdump.c
+++ b/binutils/objdump.c
@@ -229,13 +229,24 @@ usage (FILE *stream, int status)
   -g, --debugging          Display debug information in object file\n\
   -e, --debugging-tags     Display debug information using ctags style\n\
   -G, --stabs              Display (in raw form) any STABS info in the file\n\
-  -W[lLiaprmfFsoORtUuTgAckK] or\n\
+  -W[lLiaprmfFsoORtUuTgAck] or\n\
   --dwarf[=rawline,=decodedline,=info,=abbrev,=pubnames,=aranges,=macro,=frames,\n\
           =frames-interp,=str,=str-offsets,=loc,=Ranges,=pubtypes,\n\
           =gdb_index,=trace_info,=trace_abbrev,=trace_aranges,\n\
-          =addr,=cu_index,=links,=follow-links]\n\
+          =addr,=cu_index,=links]\n\
                            Display DWARF info in the file\n\
 "));
+#if DEFAULT_FOR_FOLLOW_LINKS
+  fprintf (stream, _("\
+  -WK,--dwarf=follow-links     Follow links to separate debug info files (default)\n\
+  -WN,--dwarf=no-follow-links  Do not follow links to separate debug info files\n\
+"));
+#else
+  fprintf (stream, _("\
+  -WK,--dwarf=follow-links     Follow links to separate debug info files\n\
+  -WN,--dwarf=no-follow-links  Do not follow links to separate debug info files (default)\n\
+"));
+#endif
 #ifdef ENABLE_LIBCTF
   fprintf (stream, _("\
   --ctf=SECTION            Display CTF info from SECTION\n\
@@ -737,31 +748,32 @@ slurp_symtab (bfd *abfd)
       non_fatal (_("failed to read symbol table from: %s"), bfd_get_filename (abfd));
       bfd_fatal (_("error message was"));
     }
-  if (storage)
-    {
-      off_t filesize = bfd_get_file_size (abfd);
-
-      /* qv PR 24707.  */
-      if (filesize > 0
-	  && filesize < storage
-	  /* The MMO file format supports its own special compression
-	     technique, so its sections can be larger than the file size.  */
-	  && bfd_get_flavour (abfd) != bfd_target_mmo_flavour)	  
-	{
-	  bfd_nonfatal_message (bfd_get_filename (abfd), abfd, NULL,
-				_("error: symbol table size (%#lx) is larger than filesize (%#lx)"),
-			storage, (long) filesize);
-	  exit_status = 1;
-	  symcount = 0;
-	  return NULL;
-	}
+  /* Add an extra entry (at the end) with a NULL pointer.  */
+  storage += sizeof (asymbol *);
 
-      sy = (asymbol **) xmalloc (storage);
+  off_t filesize = bfd_get_file_size (abfd);
+
+  /* qv PR 24707.  */
+  if (filesize > 0
+      && filesize < storage
+      /* The MMO file format supports its own special compression
+	 technique, so its sections can be larger than the file size.  */
+      && bfd_get_flavour (abfd) != bfd_target_mmo_flavour)	  
+    {
+      bfd_nonfatal_message (bfd_get_filename (abfd), abfd, NULL,
+			    _("error: symbol table size (%#lx) is larger than filesize (%#lx)"),
+			    storage, (long) filesize);
+      exit_status = 1;
+      symcount = 0;
+      return NULL;
     }
 
+  sy = (asymbol **) xmalloc (storage);
   symcount = bfd_canonicalize_symtab (abfd, sy);
   if (symcount < 0)
     bfd_fatal (bfd_get_filename (abfd));
+  /* assert (symcount < (storage / sizeof (asymbol *))) */
+  sy[symcount] = NULL;
   return sy;
 }
 
@@ -774,6 +786,7 @@ slurp_dynamic_symtab (bfd *abfd)
   long storage;
 
   storage = bfd_get_dynamic_symtab_upper_bound (abfd);
+  /* Add an extra entry (at the end) with a NULL pointer.  */
   if (storage < 0)
     {
       if (!(bfd_get_file_flags (abfd) & DYNAMIC))
@@ -786,12 +799,15 @@ slurp_dynamic_symtab (bfd *abfd)
 
       bfd_fatal (bfd_get_filename (abfd));
     }
-  if (storage)
-    sy = (asymbol **) xmalloc (storage);
+
+  storage += sizeof (asymbol *);
+  sy = (asymbol **) xmalloc (storage);
 
   dynsymcount = bfd_canonicalize_dynamic_symtab (abfd, sy);
   if (dynsymcount < 0)
     bfd_fatal (bfd_get_filename (abfd));
+  /* assert (symcount < (storage / sizeof (asymbol *))) */
+  sy[dynsymcount] = NULL;
   return sy;
 }
 
@@ -4899,10 +4915,12 @@ dump_bfd (bfd *abfd, bfd_boolean is_mainfile)
 		    }
 		  else
 		    {
-		      syms = xrealloc (syms, (symcount + old_symcount) * sizeof (asymbol *));
+		      syms = xrealloc (syms, (symcount + old_symcount + 1) * sizeof (asymbol *));
 		      memcpy (syms + old_symcount,
 			      extra_syms,
 			      symcount * sizeof (asymbol *));
+		      /* Preserve the NULL entry at the end of the symbol table.  */
+		      syms[symcount + old_symcount] = NULL;
 		    }
 		}
 
diff --git a/binutils/readelf.c b/binutils/readelf.c
index c61219a4652..77e450a6195 100644
--- a/binutils/readelf.c
+++ b/binutils/readelf.c
@@ -4624,12 +4624,23 @@ usage (FILE * stream)
   -R --relocated-dump=<number|name>\n\
                          Dump the contents of section <number|name> as relocated bytes\n\
   -z --decompress        Decompress section before dumping it\n\
-  -w[lLiaprmfFsoORtUuTgAckK] or\n\
+  -w[lLiaprmfFsoORtUuTgAck] or\n\
   --debug-dump[=rawline,=decodedline,=info,=abbrev,=pubnames,=aranges,=macro,=frames,\n\
                =frames-interp,=str,=str-offsets,=loc,=Ranges,=pubtypes,\n\
                =gdb_index,=trace_info,=trace_abbrev,=trace_aranges,\n\
-               =addr,=cu_index,=links,=follow-links]\n\
+               =addr,=cu_index,=links]\n\
                          Display the contents of DWARF debug sections\n"));
+#if DEFAULT_FOR_FOLLOW_LINKS
+  fprintf (stream, _("\
+  -wK,--debug-dump=follow-links     Follow links to separate debug info files (default)\n\
+  -wN,--debug-dump=no-follow-links  Do not follow links to separate debug info files\n\
+"));
+#else
+  fprintf (stream, _("\
+  -wK,--debug-dump=follow-links     Follow links to separate debug info files\n\
+  -wN,--debug-dump=no-follow-links  Do not follow links to separate debug info files (default)\n\
+"));
+#endif
   fprintf (stream, _("\
   --dwarf-depth=N        Do not display DIEs at depth N or greater\n\
   --dwarf-start=N        Display DIEs starting with N, at the same depth\n\
diff --git a/binutils/testsuite/binutils-all/compress.exp b/binutils/testsuite/binutils-all/compress.exp
index 7009bd53a04..fb991aa0405 100644
--- a/binutils/testsuite/binutils-all/compress.exp
+++ b/binutils/testsuite/binutils-all/compress.exp
@@ -717,7 +717,7 @@ proc test_gnu_debuglink {} {
 	fail "$test (objcopy link decompress)"
 	return
     }
-    set got [remote_exec host "$OBJDUMP -S tmpdir/testprog" "" "/dev/null" "tmpdir/testprog.decompress.dump"]
+    set got [remote_exec host "$OBJDUMP -S -WN tmpdir/testprog" "" "/dev/null" "tmpdir/testprog.decompress.dump"]
     if { [lindex $got 0] != 0 || ![string match "" [lindex $got 1]] } then {
 	fail "$test (objcopy dump decompress)"
 	return
@@ -726,7 +726,7 @@ proc test_gnu_debuglink {} {
 	fail "$test (objcopy link compress)"
 	return
     }
-    set got [remote_exec host "$OBJDUMP -S tmpdir/testprog" "" "/dev/null" "tmpdir/testprog.compress.dump"]
+    set got [remote_exec host "$OBJDUMP -S -WN tmpdir/testprog" "" "/dev/null" "tmpdir/testprog.compress.dump"]
     if { [lindex $got 0] != 0 || ![string match "" [lindex $got 1]] } then {
 	fail "$test (objcopy dump compress)"
 	return
diff --git a/binutils/testsuite/binutils-all/objdump.exp b/binutils/testsuite/binutils-all/objdump.exp
index aa8c207d4d1..6f95d4039f5 100644
--- a/binutils/testsuite/binutils-all/objdump.exp
+++ b/binutils/testsuite/binutils-all/objdump.exp
@@ -606,7 +606,7 @@ if { [is_elf_format] } then {
 	set testfile tmpdir/debuglink.${obj}
     }
 
-    set got [remote_exec host "$OBJDUMP $OBJDUMPFLAGS -Wk $testfile" "" "/dev/null" "objdump.out"]
+    set got [remote_exec host "$OBJDUMP $OBJDUMPFLAGS -Wk -WN $testfile" "" "/dev/null" "objdump.out"]
 
     if { [lindex $got 0] != 0 || ![string match "" [lindex $got 1]] } then {
 	fail "objdump -Wk (reason: unexpected output)"
@@ -615,7 +615,7 @@ if { [is_elf_format] } then {
     }
 
     if { [regexp_diff objdump.out $srcdir/$subdir/objdump.Wk] } then {
-	fail "objdump -Wk"
+	fail "objdump -Wk (reason: output does not match expectations)"
     } else {
 	pass "objdump -Wk"
     }
diff --git a/binutils/testsuite/binutils-all/readelf.exp b/binutils/testsuite/binutils-all/readelf.exp
index 51d26b762b3..2725f436384 100644
--- a/binutils/testsuite/binutils-all/readelf.exp
+++ b/binutils/testsuite/binutils-all/readelf.exp
@@ -507,7 +507,7 @@ if {![binutils_assemble $srcdir/$subdir/debuglink.s tmpdir/debuglink.o]} then {
 	set tempfile [remote_download host tmpdir/debuglink.o]
     }
 
-    readelf_test {--debug-dump=links} $tempfile readelf.k  {}
+    readelf_test {--debug-dump=links -wN} $tempfile readelf.k  {}
 
     # Check that debug link sections can be followed.
     if {![binutils_assemble $srcdir/$subdir/linkdebug.s tmpdir/linkdebug.debug]} then {
@@ -530,7 +530,7 @@ if {![binutils_assemble $srcdir/$subdir/dwo.s tmpdir/dwo.o]} then {
 	set tempfile [remote_download host tmpdir/dwo.o]
     }
 
-    readelf_test {--debug-dump=links} $tempfile readelf.k2  {}
+    readelf_test {--debug-dump=links --debug-dump=no-follow-links} $tempfile readelf.k2  {}
 }
 
 if {![binutils_assemble $srcdir/$subdir/zero-sec.s tmpdir/zero-sec.o]} then {
@@ -555,6 +555,6 @@ if ![is_remote host] {
     if {[catch "system \"bzip2 -dc $test > $tempfile\""] != 0} {
 	untested "bzip2 -dc ($testname)"
     } else {
-	readelf_test {--debug-dump=macro} $tempfile pr26112.r {}
+	readelf_test {--debug-dump=macro -wN} $tempfile pr26112.r {}
     }
 }
diff --git a/gas/testsuite/gas/mach-o/sections-1.d b/gas/testsuite/gas/mach-o/sections-1.d
index 5b8a8fa8c3a..cf767692504 100644
--- a/gas/testsuite/gas/mach-o/sections-1.d
+++ b/gas/testsuite/gas/mach-o/sections-1.d
@@ -1,4 +1,4 @@
-#objdump: -P section
+#objdump: -P section -WN
 .*: +file format mach-o.*
 #...
  Section: __text           __TEXT           \(bfdname: .text\)
diff --git a/gas/testsuite/gas/xgate/insns-dwarf2.d b/gas/testsuite/gas/xgate/insns-dwarf2.d
index 3a8e6992aac..fd658a6912e 100644
--- a/gas/testsuite/gas/xgate/insns-dwarf2.d
+++ b/gas/testsuite/gas/xgate/insns-dwarf2.d
@@ -1,4 +1,4 @@
-#objdump: -S
+#objdump: -S -WN
 #as: -gdwarf2
 #name: Dwarf2 test on insns.s
 #source: insns.s
diff --git a/ld/testsuite/ld-elf/sec64k.exp b/ld/testsuite/ld-elf/sec64k.exp
index d148c26d8fe..a8ce9335819 100644
--- a/ld/testsuite/ld-elf/sec64k.exp
+++ b/ld/testsuite/ld-elf/sec64k.exp
@@ -135,7 +135,7 @@ if { ![istarget "m32r-*-*"] } then {
 	puts $ofd "#as: -ez80-adl"
     }
     puts $ofd "#ld: -r"
-    puts $ofd "#readelf: -W -Ss"
+    puts $ofd "#readelf: -W -wN -Ss"
     puts $ofd "There are 680.. section headers.*:"
     puts $ofd "#..."
     puts $ofd "  \\\[ 0\\\] .* 680\[0-9\]\[0-9\]\[ \]+0\[ \]+0"
@@ -190,7 +190,7 @@ if { ![istarget "d10v-*-*"]
     if { [istarget "z80-*-*"] } then {
 	puts $ofd "#as: -ez80-adl"
     }
-    puts $ofd "#readelf: -W -Ss"
+    puts $ofd "#readelf: -W -wN -Ss"
     puts $ofd "There are 660.. section headers.*:"
     puts $ofd "#..."
     puts $ofd "  \\\[ 0\\\] .* 660..\[ \]+0\[ \]+0"

Re: RFC: Change readelf/objdump to automatically follow debug links

[Florian Weimer]

* Nick Clifton:

>   The patch adds a new configure time option
>   --enable-follow-debugs-links=[yes|no] which can be used to set the
>   default behaviour for both objdump and readelf.  If the option is
>   not used, the default is to follow the links.

What happens if the debuglink contains '/'?  Maybe it's prudent to
restrict loading of debuginfo data if it comes from the system default
location.

Thanks for looking into this,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: RFC: Change readelf/objdump to automatically follow debug links

[Nick Clifton]

Hi Florian,

>>    The patch adds a new configure time option
>>    --enable-follow-debugs-links=[yes|no] which can be used to set the
>>    default behaviour for both objdump and readelf.  If the option is
>>    not used, the default is to follow the links.
> 
> What happens if the debuglink contains '/'?  Maybe it's prudent to
> restrict loading of debuginfo data if it comes from the system default
> location.

Hmm, this could pose problems.  There are binaries with debug-links that
use absolute paths that are not rooted in system directories, but which
are still valid.  For example see:

   https://sourceware.org/bugzilla/show_bug.cgi?id=27391

Is there really a problem with following these paths ?  Could an attacker
really exploit this somehow ?

Cheers
   Nick

Re: RFC: Change readelf/objdump to automatically follow debug links

[Florian Weimer]

* Nick Clifton:

> Hi Florian,
>
>>>    The patch adds a new configure time option
>>>    --enable-follow-debugs-links=[yes|no] which can be used to set the
>>>    default behaviour for both objdump and readelf.  If the option is
>>>    not used, the default is to follow the links.
>> What happens if the debuglink contains '/'?  Maybe it's prudent to
>> restrict loading of debuginfo data if it comes from the system default
>> location.
>
> Hmm, this could pose problems.  There are binaries with debug-links that
> use absolute paths that are not rooted in system directories, but which
> are still valid.  For example see:
>
>   https://sourceware.org/bugzilla/show_bug.cgi?id=27391
>
> Is there really a problem with following these paths ?  Could an attacker
> really exploit this somehow ?

Such indirection can lead to information disclosure if the output is
shared.  Whether this applies in this particular case is hard to tell.

A compromise might be to follow the debuglinks by default as long as
they are under /usr/lib/debug, otherwise require explicit opt-in.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: RFC: Change readelf/objdump to automatically follow debug links

[Mark Wielaard]

Hi Nick,

On Wed, 2021-02-10 at 10:30 +0000, Nick Clifton via Binutils wrote:
>   I received a bug report from a Fedora rawhide user[1] that requests
>   that the objdump program should follow debug links automatically when
>   it is disassembling code.  The request seems reasonable to me, given
>   that many distributions these days make use of separate debug info
>   files, so I have created the attached patch.
> 
>   The patch adds a new configure time option --enable-follow-debugs-links=[yes|no]
>   which can be used to set the default behaviour for both objdump and
>   readelf.  If the option is not used, the default is to follow the
>   links.
> 
>   What do people think ?  Any objections to this idea ?

As discussed on the dwz mailinglist already, making this the default
breaks various things when made the default. That is because it seems
to mix two concepts. First displaying information from linked files,
like strings from an alt/dup file when displaying debug-dump sections.
This part seems fine and could even be the default (eu-readelf does
this by default).

But secondly it combines this with dumping sections from the linked
files itself (intermixed with dumping the sections of the actual input
file). This might be useful to some users, but is very confusion for
tools relying on just dumping (debug) sections from specific file. The
dwz and debugedit testsuites get very confused by this. And it causes
bugs like PR27486 and PR27478

Could these features be split (--follow-links vs --dump-links) with the
first possibly being made the default and the second only on request of
the user?

Thanks,

Mark

Re: RFC: Change readelf/objdump to automatically follow debug links

[Nick Clifton]

Hi Mark,

> But secondly it combines this with dumping sections from the linked
> files itself (intermixed with dumping the sections of the actual input
> file). This might be useful to some users, but is very confusion for
> tools relying on just dumping (debug) sections from specific file. The
> dwz and debugedit testsuites get very confused by this. And it causes
> bugs like PR27486 and PR27478
> 
> Could these features be split (--follow-links vs --dump-links) with the
> first possibly being made the default and the second only on request of
> the user?

I think that this is already done.  Ie =follow-links is on by default
but =dump-links is off by default.  (The =dump-links option just displays
the contents of link sections, it does not mean 'show the contents of
sections in linked files').

 From what you have said however it sounds as if users want readelf to
follow links, and display the contents of sections in linked files, only
if this has been explicitly requested.  (Or possibly only when processing
debugging info is some way).  But there is a problem: the change to
following debug links by default came about because objdump's disassembler
was not taking advantage of the symbol tables in debug info files.  So
for some non-debug-info related tasks we do want to follow the links and
load the files, but for others we do not.  I am not sure how this can
resolved.

Cheers
   Nick

PS.  Just to be clear there is an option to disable the automatic
   following of links, so users do already have control over readelf's
   behaviour.  It is more a case of choosing what the default behaviour
   should be.

Re: RFC: Change readelf/objdump to automatically follow debug links

[Mark Wielaard]

Hi,

On Mon, 2021-03-01 at 13:53 +0000, Nick Clifton wrote:
> > But secondly it combines this with dumping sections from the linked
> > files itself (intermixed with dumping the sections of the actual input
> > file). This might be useful to some users, but is very confusion for
> > tools relying on just dumping (debug) sections from specific file. The
> > dwz and debugedit testsuites get very confused by this. And it causes
> > bugs like PR27486 and PR27478
> > 
> > Could these features be split (--follow-links vs --dump-links) with the
> > first possibly being made the default and the second only on request of
> > the user?
> 
> I think that this is already done.  Ie =follow-links is on by default
> but =dump-links is off by default.  (The =dump-links option just displays
> the contents of link sections, it does not mean 'show the contents of
> sections in linked files').

Ah, unfortunate, I didn't mean the existing dump-links option (which I
forgot about). I meant to suggest a new option that made clear we
explicitly wanted to dump sections from linked files, not just display
data referenced by the main object file that happens to have a link to
it.

>  From what you have said however it sounds as if users want readelf to
> follow links, and display the contents of sections in linked files, only
> if this has been explicitly requested.  (Or possibly only when processing
> debugging info is some way).  But there is a problem: the change to
> following debug links by default came about because objdump's disassembler
> was not taking advantage of the symbol tables in debug info files.  So
> for some non-debug-info related tasks we do want to follow the links and
> load the files, but for others we do not.  I am not sure how this can
> resolved.

Right, I think that is the confusing consequence of the option. It was
meant to dump data in a way to showed auxiliary data from linked files.
But for readelf it doesn't just do that, but also dumps sections from
the linked files itself. I think the second usage needs its own option
(that is off by default).

e.g. I think it is fine if follow-links implies that objdump shows
disassambly with addresses resolved to symbols from linked files, or
have readelf show strings for a supplemental file for
DW_FORM_GNU_alt_strp. But that shouldn't imply dumping the section/data
from the linked file(s) themselves.

Cheers,

Mark