[PATCH 0/2] pe/coff: Improve 'objdump -p' handling of the output of 'objcopy --only-keep-debug'

[PATCH 0/2] pe/coff: Improve 'objdump -p' handling of the output of 'objcopy --only-keep-debug'

[Jon TURNEY]

Jon TURNEY (2):
  pe/coff: Avoid a crash using objdump -p on the output of objcopy
    --only-keep-debug
  pe/coff: Don't try to interpret the contents of sections with no
    contents for 'objdump -p'

 bfd/peXXigen.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

-- 
1.8.3.4

[PATCH 1/2] pe/coff: Avoid a crash using objdump -p on the output of objcopy --only-keep-debug

[Jon TURNEY]

Avoid a crash when using objdump -p on the output of objcopy --only-keep-debug

e.g.

$ objdump -p /usr/lib/debug/usr/bin/cygwin1.dbg
[...]
The Export Tables (interpreted .edata section contents)

Export Flags                    0
Time/Date stamp                 0
Major/Minor                     0/0
Segmentation fault (core dumped)

Verfify that edt.name lies inside the .edata section we have loaded before
dereferencing it.  Change adj to to bfd_vma to avoid signed vs. unsigned
comparison warnings - it could only be negative if a section had a negative
vma.

bfd/Changelog:

2014-03-18  Jon TURNEY  <jon.turney@dronecode.org.uk>

	* peXXigen.c (pe_print_edata): Verify edt.name lies inside
	section before dereferencing.

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 bfd/peXXigen.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/bfd/peXXigen.c b/bfd/peXXigen.c
index 8219ab9..d011c0e 100644
--- a/bfd/peXXigen.c
+++ b/bfd/peXXigen.c
@@ -1373,7 +1373,7 @@ pe_print_edata (bfd * abfd, void * vfile)
   bfd_size_type datasize = 0;
   bfd_size_type dataoff;
   bfd_size_type i;
-  bfd_signed_vma adj;
+  bfd_vma adj;
   struct EDT_type
   {
     long export_flags;          /* Reserved - should be zero.  */
@@ -1478,8 +1478,12 @@ pe_print_edata (bfd * abfd, void * vfile)
   fprintf (file,
 	   _("Name \t\t\t\t"));
   bfd_fprintf_vma (abfd, file, edt.name);
-  fprintf (file,
+
+  if ((edt.name >= adj) && (edt.name < adj + datasize))
+    fprintf (file,
 	   " %s\n", data + edt.name - adj);
+  else
+    fprintf (file, "(outside .edata section)\n");
 
   fprintf (file,
 	   _("Ordinal Base \t\t\t%ld\n"), edt.base);
-- 
1.8.3.4

Re: [PATCH 1/2] pe/coff: Avoid a crash using objdump -p on the output of objcopy --only-keep-debug

[Christopher Faylor]

Nick are you ok with these patches?  They seem ok to me.  I'm sorry that I
missed them previously.

cgf

On Tue, Mar 18, 2014 at 02:39:14PM +0000, Jon TURNEY wrote:
>Avoid a crash when using objdump -p on the output of objcopy --only-keep-debug
>
>e.g.
>
>$ objdump -p /usr/lib/debug/usr/bin/cygwin1.dbg
>[...]
>The Export Tables (interpreted .edata section contents)
>
>Export Flags                    0
>Time/Date stamp                 0
>Major/Minor                     0/0
>Segmentation fault (core dumped)
>
>Verfify that edt.name lies inside the .edata section we have loaded before
>dereferencing it.  Change adj to to bfd_vma to avoid signed vs. unsigned
>comparison warnings - it could only be negative if a section had a negative
>vma.
>
>bfd/Changelog:
>
>2014-03-18  Jon TURNEY  <jon.turney@dronecode.org.uk>
>
>	* peXXigen.c (pe_print_edata): Verify edt.name lies inside
>	section before dereferencing.
>
>Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
>---
> bfd/peXXigen.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
>diff --git a/bfd/peXXigen.c b/bfd/peXXigen.c
>index 8219ab9..d011c0e 100644
>--- a/bfd/peXXigen.c
>+++ b/bfd/peXXigen.c
>@@ -1373,7 +1373,7 @@ pe_print_edata (bfd * abfd, void * vfile)
>   bfd_size_type datasize = 0;
>   bfd_size_type dataoff;
>   bfd_size_type i;
>-  bfd_signed_vma adj;
>+  bfd_vma adj;
>   struct EDT_type
>   {
>     long export_flags;          /* Reserved - should be zero.  */
>@@ -1478,8 +1478,12 @@ pe_print_edata (bfd * abfd, void * vfile)
>   fprintf (file,
> 	   _("Name \t\t\t\t"));
>   bfd_fprintf_vma (abfd, file, edt.name);
>-  fprintf (file,
>+
>+  if ((edt.name >= adj) && (edt.name < adj + datasize))
>+    fprintf (file,
> 	   " %s\n", data + edt.name - adj);
>+  else
>+    fprintf (file, "(outside .edata section)\n");
> 
>   fprintf (file,
> 	   _("Ordinal Base \t\t\t%ld\n"), edt.base);
>-- 
>1.8.3.4
>
>

Re: [PATCH 1/2] pe/coff: Avoid a crash using objdump -p on the output of objcopy --only-keep-debug

[Nicholas Clifton]

Hi Jon,

>> 2014-03-18  Jon TURNEY  <jon.turney@dronecode.org.uk>
>>
>> 	* peXXigen.c (pe_print_edata): Verify edt.name lies inside
>> 	section before dereferencing.

Approved and applied.

Cheers
   Nick

Re: [PATCH 1/2] pe/coff: Avoid a crash using objdump -p on the output of objcopy --only-keep-debug

[Jon TURNEY]

On 03/04/2014 12:28, Nicholas Clifton wrote:
> Hi Jon,
> 
>>> 2014-03-18  Jon TURNEY
>>>
>>>     * peXXigen.c (pe_print_edata): Verify edt.name lies inside
>>>     section before dereferencing.
> 
> Approved and applied.

Thank you.

I also posted a set of patches back in January [1].  I'd be grateful for any
comments on those.

[1] https://sourceware.org/ml/binutils/2014-01/msg00296.html

Re: [PATCH 1/2] pe/coff: Avoid a crash using objdump -p on the output of objcopy --only-keep-debug

[Nicholas Clifton]

Hi Jon,

> I also posted a set of patches back in January [1].  I'd be grateful for any
> comments on those.
>
> [1] https://sourceware.org/ml/binutils/2014-01/msg00296.html


Oops - sorry - that one slipped through the net.

I reviewed and applied the patches.  They were OK although there were a 
few minor problems:

   * You did not update ld.textinfo's description of the --build-id 
linker command line option to mention the support for COFF format files.

   * There were quite a few places where the GNU Coding Standard was not 
being followed.  Especially the space between a function name and its 
opening parenthesis.  Ie "foo (bar)" not "foo(bar)".

   * Since this is a new feature it should also be mentioned in ld/NEWS.

I took care of all of these issues.

Cheers
   Nick

Re: [PATCH 1/2] pe/coff: Avoid a crash using objdump -p on the output of objcopy --only-keep-debug

[Jon TURNEY]

On 08/04/2014 11:04, Nicholas Clifton wrote:
>> I also posted a set of patches back in January [1].  I'd be grateful for any
>> comments on those.
>>
>> [1] https://sourceware.org/ml/binutils/2014-01/msg00296.html
> 
> 
> Oops - sorry - that one slipped through the net.
> 
> I reviewed and applied the patches.  They were OK although there were a few
> minor problems:
> 
>   * You did not update ld.textinfo's description of the --build-id linker
> command line option to mention the support for COFF format files.
> 
>   * There were quite a few places where the GNU Coding Standard was not being
> followed.  Especially the space between a function name and its opening
> parenthesis.  Ie "foo (bar)" not "foo(bar)".
> 
>   * Since this is a new feature it should also be mentioned in ld/NEWS.
> 
> I took care of all of these issues.

Thanks very much for doing that.  I shall try to bear those points in mind in
the future.

When rebasing, I notice that some other changes seem to have been mixed into
this commit:

- some changes related to long section name handling in pe(|p).em
- some changes in ldmain.c to handling -v as the only command line option
- some whitespace changes

I wonder if that was deliberate?

Re: [PATCH 1/2] pe/coff: Avoid a crash using objdump -p on the output of objcopy --only-keep-debug

[Alan Modra]

Fix fallout from splitting ldbuildid.[ch] off elf32.em.

Applied.
    
bfd/
    	* libcoff.h: Regenerate.
ld/
    	* emultempl/spuelf.em: Include safe-ctype.h, remove duplicate errno.h.
    	* emultempl/nds32elf.em: Include bfd_stdint.h.
    	* po/POTFILES.in: Regenerate.

diff --git a/bfd/libcoff.h b/bfd/libcoff.h
index 6cb387c..19ba7e7 100644
--- a/bfd/libcoff.h
+++ b/bfd/libcoff.h
@@ -124,7 +124,7 @@ typedef struct pe_tdata
   bfd_boolean (*in_reloc_p) (bfd *, reloc_howto_type *);
   flagword real_flags;
 
-  /* build-id info. */
+  /* Build-id info.  */
   struct
   {
     bfd_boolean (*after_write_object_contents) (bfd *);
diff --git a/ld/emultempl/nds32elf.em b/ld/emultempl/nds32elf.em
index 417eda5..96e6aa3 100644
--- a/ld/emultempl/nds32elf.em
+++ b/ld/emultempl/nds32elf.em
@@ -25,6 +25,7 @@ fragment <<EOF
 #include "libbfd.h"
 #include "elf-bfd.h"
 #include "elf/nds32.h"
+#include "bfd_stdint.h"
 #include "elf32-nds32.h"
 
 static int relax_fp_as_gp = 1;		/* --mrelax-omit-fp  */
diff --git a/ld/emultempl/spuelf.em b/ld/emultempl/spuelf.em
index eaf4de7..5167b35 100644
--- a/ld/emultempl/spuelf.em
+++ b/ld/emultempl/spuelf.em
@@ -447,7 +447,7 @@ EOF
 
 if grep -q 'ld_elf.*ppc.*_emulation' ldemul-list.h; then
   fragment <<EOF
-#include <errno.h>
+#include "safe-ctype.h"
 #include "filenames.h"
 #include "libiberty.h"
 
diff --git a/ld/po/POTFILES.in b/ld/po/POTFILES.in
index 55cbd13..fcc2894 100644
--- a/ld/po/POTFILES.in
+++ b/ld/po/POTFILES.in
@@ -3,6 +3,8 @@ elf-hints-local.h
 emultempl/armcoff.em
 emultempl/pe.em
 ld.h
+ldbuildid.c
+ldbuildid.h
 ldcref.c
 ldctor.c
 ldctor.h

-- 
Alan Modra
Australia Development Lab, IBM

[PATCH 2/2] pe/coff: Don't try to interpret the contents of sections with no contents for 'objdump -p'

[Jon TURNEY]

Don't try to interpret the contents of sections with no contents for 'objdump -p'.
The meaning of a series of zeros is not very interesting.

bfd/Changelog:

2014-03-18  Jon TURNEY  <jon.turney@dronecode.org.uk>

	* peXXigen.c (pe_print_idata, pe_print_edata, pe_print_reloc)
	(rsrc_print_section): Don't bother interpreting the contents
	of sections which have no contents.

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
---
 bfd/peXXigen.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/bfd/peXXigen.c b/bfd/peXXigen.c
index d011c0e..539c65c 100644
--- a/bfd/peXXigen.c
+++ b/bfd/peXXigen.c
@@ -1111,6 +1111,14 @@ pe_print_idata (bfd * abfd, void * vfile)
 		   _("\nThere is an import table, but the section containing it could not be found\n"));
 	  return TRUE;
 	}
+
+      if (!(section->flags & SEC_HAS_CONTENTS))
+        {
+	  fprintf (file,
+		   _("\nThere is an import table in %s, but that section has no contents\n"),
+		   section->name);
+	  return TRUE;
+        }
     }
 
   fprintf (file, _("\nThere is an import table in %s at 0x%lx\n"),
@@ -1424,6 +1432,14 @@ pe_print_edata (bfd * abfd, void * vfile)
 	  return TRUE;
 	}
 
+      if (!(section->flags & SEC_HAS_CONTENTS))
+        {
+	  fprintf (file,
+		   _("\nThere is an export table in %s, but that section has no contents\n"),
+		   section->name);
+	  return TRUE;
+        }
+
       dataoff = addr - section->vma;
       datasize = extra->DataDirectory[PE_EXPORT_TABLE].Size;
       if (datasize > section->size - dataoff)
@@ -1937,6 +1953,9 @@ pe_print_reloc (bfd * abfd, void * vfile)
   if (section->size == 0)
     return TRUE;
 
+  if (!(section->flags & SEC_HAS_CONTENTS))
+    return TRUE;
+
   fprintf (file,
 	   _("\n\nPE File Base Relocations (interpreted .reloc section contents)\n"));
 
@@ -2185,6 +2204,9 @@ rsrc_print_section (bfd * abfd, void * vfile)
   if (datasize == 0)
     return TRUE;
 
+  if (!(section->flags & SEC_HAS_CONTENTS))
+    return TRUE;
+
   if (! bfd_malloc_and_get_section (abfd, section, & data))
     {
       if (data != NULL)
-- 
1.8.3.4