Strip 2.13 corrupts exe file

Strip 2.13 corrupts exe file

[Richard van den Berg]

I have this bizarre problem with strip version 2.13. It seems to corrupt 
an exe file we develop. I am a Linux user myself, but this dmt.exe is a 
Lotus Notes tool, so it needs to run under Windows. I have 3 versions of 
strip:

/usr/bin/strip on my Debian system (binutils-multiarch version 
2.13.90.0.18-1)
/usr/bin/strip.exe on a Win-NT cygwin system (binutils version 20030307-1)
c:\mingw\bin\strip.exe on a WinMe system (MinGW version 2.0.0-3 includes 
binutils-2.13-20020903-1)

The first 2 corrupt the exe in a way that it does not work anymore: the 
dos prompt is simply returned without any output. Only the MinGW version 
gets it right (the exe still works). Here is "strip -V" of the above (in 
the same order):

GNU strip 2.13.90.0.18 20030121 Debian GNU/Linux
GNU strip 2.13.90 20030308
GNU strip 2.13

Some details about the files:

$ ls -la *.exe
77824 Mar 10 14:45 dmt-org.exe
74240 Mar 10 15:24 dmt-stripped-with-cygwin.exe
74240 Mar 10 15:42 dmt-stripped-with-linux.exe
74240 Mar 10 14:53 dmt-stripped-with-mingw.exe

$ cmp -lb dmt-stripped-with-linux.exe dmt-stripped-with-cygwin.exe
   137 145 e     21 ^Q
   138 244 €    240  
   217 374 ü    250 š
   218  67 7     63 3


$ cmp -lb dmt-stripped-with-linux.exe dmt-stripped-with-mingw.exe
   137 145 e    354 ì
   138 244 €    230 M-^X
   217 374 ü    340 à
   218  67 7    363 ó
   257   0 ^@   224 M-^T
   258   0 ^@   345 å
   261   0 ^@   120 P
   346   0 ^@   340 à
   349   0 ^@   170 x
   350   0 ^@     1 ^A

It looks like strip NULLs a few more bytes than it should.

I'm willing to supply the exe files for testing, etc. Just let me know 
where to send them. Please Cc me in replies, as I am not on this list.

Sincerely,

-- 
Richard van den Berg, CISSP

Trust Factory B.V.      | http://www.trust-factory.com/
Bazarstraat 44a         | Phone: +31 70 3620684
NL-2518AK The Hague     | Fax  : +31 70 3603009
The Netherlands         |

Re: Strip 2.13 corrupts exe file

[Nick Clifton]

Hi Richard,

> I'm willing to supply the exe files for testing, etc. Just let me know
> where to send them. Please Cc me in replies, as I am not on this list.

If you can send the pre- and post- strip files to me I will have a
look.

Cheers
        Nick

Re: Strip 2.13 corrupts exe file

[Nick Clifton]

Hi Richard,

  I think that I have now traced down the cause of this problem.
  Strip is zeroing the $idata2 and $idata5 Data Dictionary entries
  (Import Directory and Import Address Table).  It is doing this
  because the code expects these entries to be reconstructed during a
  call to bfd_coff_final_link().

  Since, however a final link is not being performed, they are never
  reconstructed and so you end with a broken binary.  Please try
  applying the patch below to your sources and let me know if it fixes
  the problem.

Cheers
        Nick

2003-03-18  Nick Clifton  <nickc@redhat.com>

	* peXXigen.c (_bfd_XXi_swap_aouthdr_out): Initialse $idata2
	and $idata5 even though they will be recreated during a call
	to bfd_coff_final_link, since it cannot be relied upon that
	this function will be called.

Index: bfd/peXXigen.c
===================================================================
RCS file: /cvs/src/src/bfd/peXXigen.c,v
retrieving revision 1.12
diff -c -3 -p -w -r1.12 peXXigen.c
*** bfd/peXXigen.c	30 Nov 2002 08:39:40 -0000	1.12
--- bfd/peXXigen.c	18 Mar 2003 17:08:10 -0000
*************** _bfd_XXi_swap_aouthdr_out (abfd, in, out
*** 570,575 ****
--- 571,578 ----
    struct internal_extra_pe_aouthdr *extra = &pe->pe_opthdr;
    PEAOUTHDR *aouthdr_out = (PEAOUTHDR *) out;
    bfd_vma sa, fa, ib;
+   IMAGE_DATA_DIRECTORY idata2, idata5;
+ 
    
    if (pe->force_minimum_alignment)
      {
*************** _bfd_XXi_swap_aouthdr_out (abfd, in, out
*** 586,591 ****
--- 589,597 ----
    fa = extra->FileAlignment;
    ib = extra->ImageBase;
  
+   idata2 = pe->pe_opthdr.DataDirectory[1];
+   idata5 = pe->pe_opthdr.DataDirectory[12];
+   
    if (aouthdr_in->tsize)
      {
        aouthdr_in->text_start -= ib;
*************** _bfd_XXi_swap_aouthdr_out (abfd, in, out
*** 614,641 ****
  #define SA(x) (((x) + sa -1 ) & (- sa))
  
    /* We like to have the sizes aligned.  */
- 
    aouthdr_in->bsize = FA (aouthdr_in->bsize);
  
    extra->NumberOfRvaAndSizes = IMAGE_NUMBEROF_DIRECTORY_ENTRIES;
  
!   /* first null out all data directory entries ..  */
    memset (extra->DataDirectory, 0, sizeof (extra->DataDirectory));
  
    add_data_entry (abfd, extra, 0, ".edata", ib);
- 
-   /* Don't call add_data_entry for .idata$2 or .idata$5.  It's done in
-      bfd_coff_final_link where all the required information is
-      available.  */
- 
-   /* However, until other .idata fixes are made (pending patch), the
-      entry for .idata is needed for backwards compatability.  FIXME.  */
-   add_data_entry (abfd, extra, 1, ".idata", ib);
- 
    add_data_entry (abfd, extra, 2, ".rsrc", ib);
- 
    add_data_entry (abfd, extra, 3, ".pdata", ib);
  
    /* For some reason, the virtual size (which is what's set by
       add_data_entry) for .reloc is not the same as the size recorded
       in this slot by MSVC; it doesn't seem to cause problems (so far),
--- 620,653 ----
  #define SA(x) (((x) + sa -1 ) & (- sa))
  
    /* We like to have the sizes aligned.  */
    aouthdr_in->bsize = FA (aouthdr_in->bsize);
  
    extra->NumberOfRvaAndSizes = IMAGE_NUMBEROF_DIRECTORY_ENTRIES;
  
!   /* First null out all data directory entries.  */
    memset (extra->DataDirectory, 0, sizeof (extra->DataDirectory));
  
    add_data_entry (abfd, extra, 0, ".edata", ib);
    add_data_entry (abfd, extra, 2, ".rsrc", ib);
    add_data_entry (abfd, extra, 3, ".pdata", ib);
  
+   /* In theory we do not need to call add_data_entry for .idata$2 or
+      .idata$5.  It will be done in bfd_coff_final_link where all the
+      required information is available.  If however, we are not going
+      to perform a final link, eg because we have been invoked by objcopy
+      or strip, then we need to make sure that these Data Directory
+      entries are initialised properly.
+ 
+      So - we copy the input values into the output values, and then, if
+      a final link is going to be performed, it can overwrite them.  */
+   extra->DataDirectory[1]  = idata2;
+   extra->DataDirectory[12] = idata5;
+ 
+   if (extra->DataDirectory[1].VirtualAddress == 0)
+     /* Until other .idata fixes are made (pending patch), the entry for
+        .idata is needed for backwards compatability.  FIXME.  */
+     add_data_entry (abfd, extra, 1, ".idata", ib);
+     
    /* For some reason, the virtual size (which is what's set by
       add_data_entry) for .reloc is not the same as the size recorded
       in this slot by MSVC; it doesn't seem to cause problems (so far),

Re: Strip 2.13 corrupts exe file

[Richard van den Berg]

Nick Clifton wrote:
>   I think that I have now traced down the cause of this problem.
>   Strip is zeroing the $idata2 and $idata5 Data Dictionary entries
>   (Import Directory and Import Address Table).  It is doing this
>   because the code expects these entries to be reconstructed during a
>   call to bfd_coff_final_link().
> 
>   Since, however a final link is not being performed, they are never
>   reconstructed and so you end with a broken binary.  Please try
>   applying the patch below to your sources and let me know if it fixes
>   the problem.

Yes! With the patch applied, the stipped exe file works as expected. 
Thanks a lot, this was driving us quite mad.

One more question: isn't it strange I am the only one stumbling upon 
this? Or are the internals of our executable very non-standard? (I am 
not too familiar with pei-i386 myself.)

Sincerely,

-- 
Richard van den Berg, CISSP

Trust Factory B.V.      | http://www.trust-factory.com/
Bazarstraat 44a         | Phone: +31 70 3620684
NL-2518AK The Hague     | Fax  : +31 70 3603009
The Netherlands         |

Re: Strip 2.13 corrupts exe file

[Nick Clifton]

Hi Richard,

> Yes! With the patch applied, the stipped exe file works as
> expected. Thanks a lot, this was driving us quite mad.

Excellent - I will apply it to the sources.

> One more question: isn't it strange I am the only one stumbling upon
> this? Or are the internals of our executable very non-standard? (I am
> not too familiar with pei-i386 myself.)

Well PE and PEI are standard formats, so that is not the problem.  I
guess that most people (who use them) are not stripping their
binaries.  The bug only affected strip (and objcopy), so if you did
not use those tools you would not encounter the problem.

Cheers
        Nick

Re: Strip 2.13 corrupts exe file

[Danny Smith]

 --- Nick Clifton <nickc@redhat.com> wrote: > Hi Richard,
> 
> > Yes! With the patch applied, the stipped exe file works as
> > expected. Thanks a lot, this was driving us quite mad.
> 
> Excellent - I will apply it to the sources.
> 
> > One more question: isn't it strange I am the only one stumbling upon
> > this? Or are the internals of our executable very non-standard? (I am
> > not too familiar with pei-i386 myself.)
> 
> Well PE and PEI are standard formats, so that is not the problem.  I
> guess that most people (who use them) are not stripping their
> binaries.  The bug only affected strip (and objcopy), so if you did
> not use those tools you would not encounter the problem.
> 

Huh, I use strip (built from CVS) quite a bit on mingw produced executables
(including daily builds of GCC) and have not seen any problems.

For many years, the memset in _bfd_XXi_swap_aouthdr_out was a no-op because the
arguments were reversed.

Then:

2002-07-16  Moritz Jodeit  <moritz@jodeit.org>

	* peXXigen.c (_bfd_XXi_swap_aouthdr_out): Correct order of memset args.

Donn Terry reported the problem with  nulling out all data directory entries in
August and suggested that the call to memset could just be eliminated.  I've
tried his suggestion and have seen no problems.  But then again I haven't seen
any problems when the memset is actually working either.  So I too find it
strange that no-one to my knowledge  on mingw or cygwin lists has complained
until now. 

Danny

Danny

> Cheers
>         Nick
>  

http://mobile.yahoo.com.au - Yahoo! Mobile
- Check & compose your email via SMS on your Telstra or Vodafone mobile.

Re: Strip 2.13 corrupts exe file

[Richard van den Berg]

Danny Smith wrote:
> Huh, I use strip (built from CVS) quite a bit on mingw produced executables
> (including daily builds of GCC) and have not seen any problems.

I must say I only noticed the problem when we moved from mingw to VC++. 
Like I said, I am not familiar with the PEI internals, so I don't know 
how both compilers differ in their output.

-- 
Richard van den Berg, CISSP

Trust Factory B.V.      | http://www.trust-factory.com/
Bazarstraat 44a         | Phone: +31 70 3620684
NL-2518AK The Hague     | Fax  : +31 70 3603009
The Netherlands         |

Re: Strip 2.13 corrupts exe file

[Danny Smith]

 --- Richard van den Berg <richard@trust-factory.com> wrote: > Danny Smith
wrote:
> > Huh, I use strip (built from CVS) quite a bit on mingw produced executables
> > (including daily builds of GCC) and have not seen any problems.
> 
> I must say I only noticed the problem when we moved from mingw to VC++. 

OK, now it doesn't seem so strange.  That also explains why Donn was so quick
to trip over the memset change.  

Danny


http://mobile.yahoo.com.au - Yahoo! Mobile
- Check & compose your email via SMS on your Telstra or Vodafone mobile.

Re: Strip 2.13 corrupts exe file

[Christian Groessler]

Hi,

On Thu, 3 Apr 2003, Nick Clifton wrote:

>   3. Create a ChangeLog entry.

Oops, sorry.

> Here is the patch that I have applied.

Thanks.

chris

Re: Strip 2.13 corrupts exe file

[Nick Clifton]

Hi Chris,

: I'm using the attached patch. It doesn't change the behaviour for
: exe files and fixes the handling of object files wrt linking with MS 
: linker.
: I *think* it should also work for the gnu linker, because objdump with
: the patch applied still reports correct bss and data sizes.
: 
: Maybe I can be considered for integration? (Although I can't remember
: right now why I've added the '|| !scnhdr_int->s_size' part. :-( )

I have applied a slight variation of your patch.  The changes I made
were:

  1. Extract the test to distinguish between a PE executable and a PE
     object file and put it into a macro.

  2. Remove the x86 specific nature of the change to
     ..._swap_scnhdr_out, and make it apply to all PE ports.

  3. Create a ChangeLog entry.

Here is the patch that I have applied.

Cheers
	Nick

2003-04-03  Nick Clifton  <nickc@redhat.com>

	* peXXigen.c (_bfd_XXi_swap_scnhdr_out): Compute ps and ss
	differently for object files and executables.
	* peicode.h (coff_swap_scnhdr_in): Only set the s_size field
	for object files or for executables who have not already
	initialised the field.
	* libpei.h (bfd_pe_executable_p): New macro.  Return true if
	the PE format bfd is an executable.

Index: bfd/peXXigen.c
===================================================================
RCS file: /cvs/src/src/bfd/peXXigen.c,v
retrieving revision 1.13
diff -c -3 -p -w -r1.13 peXXigen.c
*** bfd/peXXigen.c	20 Mar 2003 09:02:09 -0000	1.13
--- bfd/peXXigen.c	3 Apr 2003 11:08:29 -0000
*************** _bfd_XXi_swap_scnhdr_out (abfd, in, out)
*** 906,917 ****
--- 906,929 ----
       sometimes).  */
    if ((scnhdr_int->s_flags & IMAGE_SCN_CNT_UNINITIALIZED_DATA) != 0)
      {
+       if (bfd_pe_executable_p (abfd))
+ 	{
  	  ps = scnhdr_int->s_size;
  	  ss = 0;
  	}
        else
         {
+          ps = 0;
+          ss = scnhdr_int->s_size;
+        }
+     }
+   else
+     {
+       if (bfd_pe_executable_p (abfd))
  	ps = scnhdr_int->s_paddr;
+       else
+ 	ps = 0;
+ 
        ss = scnhdr_int->s_size;
      }
  
Index: bfd/peicode.h
===================================================================
RCS file: /cvs/src/src/bfd/peicode.h,v
retrieving revision 1.35
diff -c -3 -p -w -r1.35 peicode.h
*** bfd/peicode.h	30 Nov 2002 08:39:40 -0000	1.35
--- bfd/peicode.h	3 Apr 2003 11:08:30 -0000
*************** coff_swap_scnhdr_in (abfd, ext, in)
*** 264,270 ****
--- 261,271 ----
    if ((scnhdr_int->s_flags & IMAGE_SCN_CNT_UNINITIALIZED_DATA) != 0
        && (scnhdr_int->s_paddr > 0))
      {
+      /* Always set it for non pe-obj files, and don't overwrite it
+         if it's zero for object files.  */
+      if (! bfd_pe_executable_p (abfd) || !scnhdr_int->s_size)
         scnhdr_int->s_size = scnhdr_int->s_paddr;
+ 
        /* This code used to set scnhdr_int->s_paddr to 0.  However,
           coff_set_alignment_hook stores s_paddr in virt_size, which
           only works if it correctly holds the virtual size of the

Index: bfd/libpei.h
===================================================================
RCS file: /cvs/src/src/bfd/libpei.h,v
retrieving revision 1.9
diff -c -3 -p -w -r1.9 libpei.h
*** bfd/libpei.h	30 Nov 2002 08:39:40 -0000	1.9
--- bfd/libpei.h	3 Apr 2003 11:08:30 -0000
*************** unsigned int _bfd_XX_only_swap_filehdr_o
*** 333,335 ****
--- 330,335 ----
  unsigned int _bfd_XXi_only_swap_filehdr_out PARAMS ((bfd*, PTR, PTR));
  bfd_boolean _bfd_XX_bfd_copy_private_section_data
    PARAMS ((bfd *, asection *, bfd *, asection *));
+ 
+ /* Macro: Returns true if the bfd is a PE executable as opposed to a PE object file.  */
+ #define bfd_pe_executable_p(abfd)  (strncmp ((abfd)->xvec->name, "pei-", 4) == 0)

Re: Strip 2.13 corrupts exe file

[Christian Groessler]

Hi,

On Fri, 21 Mar 2003, Richard van den Berg wrote:

> Christian Groessler wrote:
> > There are also problems mixing vc++ and binutils object files, see
> > http://sources.redhat.com/ml/binutils/2001-12/msg00327.html
>
> That link talks about a problem with linking binutils object file using
> the VC++ linker. I haven't tried that myself. I did link a VC++ object
> file with gcc object files using binutils. The resulting executable was
> not working as expected. I experienced all sorts of weird buffer
> overflows. This is the reason we are now moving to VC++ for all object
> files.
>
> Thanks for the info, as I could see myself coming across this situation
> in the near future. I take it, it has not been resolved?

I'm using the attached patch. It doesn't change the behaviour for exe
files and fixes the handling of object files wrt linking with MS
linker.
I *think* it should also work for the gnu linker, because objdump with
the patch applied still reports correct bss and data sizes.

Maybe I can be considered for integration? (Although I can't remember
right now why I've added the '|| !scnhdr_int->s_size' part. :-( )

regards,
chris

--------------------
Index: peXXigen.c
===================================================================
RCS file: /cvs/src/src/bfd/peXXigen.c,v
retrieving revision 1.13
diff -p -u -r1.13 peXXigen.c
--- peXXigen.c	20 Mar 2003 09:02:09 -0000	1.13
+++ peXXigen.c	27 Mar 2003 17:18:42 -0000
@@ -906,12 +906,27 @@ _bfd_XXi_swap_scnhdr_out (abfd, in, out)
      sometimes).  */
   if ((scnhdr_int->s_flags & IMAGE_SCN_CNT_UNINITIALIZED_DATA) != 0)
     {
-      ps = scnhdr_int->s_size;
-      ss = 0;
+      if (strcmp(abfd->xvec->name, "pe-i386") == 0)
+        {  /* object file.  */
+          ps = 0;
+          ss = scnhdr_int->s_size;
+        }
+      else
+        {  /* executable file.  */
+          ps = scnhdr_int->s_size;
+          ss = 0;
+        }
     }
   else
     {
-      ps = scnhdr_int->s_paddr;
+      if (strcmp(abfd->xvec->name, "pe-i386") == 0)
+        {  /* object file.  */
+          ps = 0;
+        }
+      else
+        {  /* executable file.  */
+          ps = scnhdr_int->s_paddr;
+        }
       ss = scnhdr_int->s_size;
     }
 
Index: peicode.h
===================================================================
RCS file: /cvs/src/src/bfd/peicode.h,v
retrieving revision 1.35
diff -p -u -r1.35 peicode.h
--- peicode.h	30 Nov 2002 08:39:40 -0000	1.35
+++ peicode.h	27 Mar 2003 17:18:42 -0000
@@ -264,7 +264,10 @@ coff_swap_scnhdr_in (abfd, ext, in)
   if ((scnhdr_int->s_flags & IMAGE_SCN_CNT_UNINITIALIZED_DATA) != 0
       && (scnhdr_int->s_paddr > 0))
     {
-      scnhdr_int->s_size = scnhdr_int->s_paddr;
+      /* Always set it for non pe-obj files,  and don't overwrite it
+         if it's zero for object files.  */
+      if (strncmp(abfd->xvec->name, "pe-", 3) != 0 || !scnhdr_int->s_size)
+        scnhdr_int->s_size = scnhdr_int->s_paddr;
       /* This code used to set scnhdr_int->s_paddr to 0.  However,
          coff_set_alignment_hook stores s_paddr in virt_size, which
          only works if it correctly holds the virtual size of the
--------------------

Re: Strip 2.13 corrupts exe file

[Richard van den Berg]

Christian Groessler wrote:
> There are also problems mixing vc++ and binutils object files, see
> http://sources.redhat.com/ml/binutils/2001-12/msg00327.html

That link talks about a problem with linking binutils object file using 
the VC++ linker. I haven't tried that myself. I did link a VC++ object 
file with gcc object files using binutils. The resulting executable was 
not working as expected. I experienced all sorts of weird buffer 
overflows. This is the reason we are now moving to VC++ for all object 
files.

Thanks for the info, as I could see myself coming across this situation 
in the near future. I take it, it has not been resolved?

Sincerely,

-- 
Richard van den Berg, CISSP

Trust Factory B.V.      | http://www.trust-factory.com/
Bazarstraat 44a         | Phone: +31 70 3620684
NL-2518AK The Hague     | Fax  : +31 70 3603009
The Netherlands         |

Re: Strip 2.13 corrupts exe file

[Christian Groessler]

Hi,

On 03/20/2003 06:13:13 PM CET Richard van den Berg wrote:
>
>Danny Smith wrote:
>> Huh, I use strip (built from CVS) quite a bit on mingw produced
executables
>> (including daily builds of GCC) and have not seen any problems.
>
>I must say I only noticed the problem when we moved from mingw to VC++.
>Like I said, I am not familiar with the PEI internals, so I don't know
>how both compilers differ in their output.

There are also problems mixing vc++ and binutils object files, see
http://sources.redhat.com/ml/binutils/2001-12/msg00327.html

regards,
chris