Bug 20193 - Invalid executable after adding debuglink to an executable produced after merging PE resource sections
Summary: Invalid executable after adding debuglink to an executable produced after mer...
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: binutils (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-02 16:52 UTC by Jon Turney
Modified: 2016-11-25 10:00 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
Test case (1.21 KB, application/x-xz)
2016-06-02 16:52 UTC, Jon Turney
Details
Output from test case (14.83 KB, application/x-xz)
2016-06-03 10:55 UTC, Jon Turney
Details
Patch to update size of resources in DataDirectory (711 bytes, patch)
2016-06-03 11:00 UTC, Jon Turney
Details | Diff
Output from test case with patch applied (14.83 KB, application/x-xz)
2016-06-03 11:02 UTC, Jon Turney
Details
Proposed patch (251 bytes, patch)
2016-06-03 15:59 UTC, Nick Clifton
Details | Diff
Output from test case with 2nd patch applied (15.74 KB, application/x-xz)
2016-06-06 13:13 UTC, Jon Turney
Details
Don't adjust size of PE/COFF merged .rsrc section (903 bytes, patch)
2016-11-24 14:42 UTC, Jon Turney
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Turney 2016-06-02 16:52:39 UTC
Created attachment 9304 [details]
Test case

It looks like merging of resource sections which are non-trivial occasionally produces executables which are invalid after further manipulation, e.g. after stripping and adding a debuglink section

Attached testcase demonstrates this, on x86_64.

1.exe and 2.exe are valid, but Windows refuses to run 3.exe as an invalid executable.
Comment 1 Jon Turney 2016-06-02 17:03:14 UTC
I don't understand why only certain sizes of resource section seem to trigger 
this problem, but a couple of observations:

I guess that the fact that the merged resource section can be smaller than the sum of the sizes, and thus can move under a page-size boundary has something to do with this bug.

The size of the resources in the DataDirectory is not updated by rsrc_process_section()

Since rsrc_process_section() takes place after bfd_coff_compute_section_file_positions(), allowing the .rsrc section to shrink seems problematic (I'm not sure how this interacts with the "page align the .rsrc section" patch in #16807)
Comment 2 Nick Clifton 2016-06-03 10:15:10 UTC
Hi Jon,

  Please could upload the 1.exe, 2.exe, 3.exe and 3.dbg files ?  I do not have
  a mingw64 build system here...

  It would be really nice if Windows could tell us *why* it does not like 3.exe
  binary.  Is there a Windows tool that dumps resources and which might give a
  error message or two ?

> The size of the resources in the DataDirectory is not updated by 
>  rsrc_process_section()

  Hmm, I wonder if that is it.  Does this, completely untested, patch make a
  difference ?

Cheers
  Nick

diff --git a/bfd/peXXigen.c b/bfd/peXXigen.c
index c92c1ea..bb0e2c8 100644
--- a/bfd/peXXigen.c
+++ b/bfd/peXXigen.c
@@ -4320,6 +4320,8 @@ rsrc_process_section (bfd * abfd,
   bfd_set_section_contents (pfinfo->output_bfd, sec, new_data, 0, size);
   sec->size = sec->rawsize = size;
 
+  pe->pe_opthdr.DataDirectory[PE_RESOURCE_TABLE].Size = size;
+
  end:
   /* Step six: Free all the memory that we have used.  */
   /* FIXME: Free the resource tree, if we have one.  */
Comment 3 Jon Turney 2016-06-03 10:55:58 UTC
Created attachment 9308 [details]
Output from test case

(In reply to Nick Clifton from comment #2)
> Hi Jon,
> 
> Please could upload the 1.exe, 2.exe, 3.exe and 3.dbg files ?  I do not
> have a mingw64 build system here...

No problem, attached.

> It would be really nice if Windows could tell us *why* it does not like
> 3.exe binary.  Is there a Windows tool that dumps resources and which might
> give a error message or two ?

Yes, it's very annoying, but I have not found any tool to help verify the resources are loadable.
Comment 4 Jon Turney 2016-06-03 11:00:33 UTC
Created attachment 9309 [details]
Patch to update size of resources in DataDirectory

(In reply to Nick Clifton from comment #2)
> 
> > The size of the resources in the DataDirectory is not updated by 
> >  rsrc_process_section()
> 
>   Hmm, I wonder if that is it.  Does this, completely untested, patch make a
>   difference ?
> 
> Cheers
>   Nick
> 
> diff --git a/bfd/peXXigen.c b/bfd/peXXigen.c
> index c92c1ea..bb0e2c8 100644
> --- a/bfd/peXXigen.c
> +++ b/bfd/peXXigen.c
> @@ -4320,6 +4320,8 @@ rsrc_process_section (bfd * abfd,
>    bfd_set_section_contents (pfinfo->output_bfd, sec, new_data, 0, size);
>    sec->size = sec->rawsize = size;
>  
> +  pe->pe_opthdr.DataDirectory[PE_RESOURCE_TABLE].Size = size;
> +
>   end:
>    /* Step six: Free all the memory that we have used.  */
>    /* FIXME: Free the resource tree, if we have one.  */

This isn't quite enough to set the size of the DataDirectory correctly, I used the attached instead.

Unfortunately, with that patch applied, 1.exe is invalid, while 2.exe and 3.exe are good
Comment 5 Jon Turney 2016-06-03 11:02:38 UTC
Created attachment 9310 [details]
Output from test case with patch applied
Comment 6 Nick Clifton 2016-06-03 15:36:35 UTC
Hi Jon,

> Unfortunately, with that patch applied, 1.exe is invalid, while 2.exe and
> 3.exe are good

:-} So close...

Question: in the 1.exe post-patch file the MinorLinkerVersion field in the COFF
header shows 26, but in all the other .exe files, both pre- and post- patch, the version number is reported as 25.  Any idea why this might be ?

Cheers
  Nick
Comment 7 Nick Clifton 2016-06-03 15:59:15 UTC
Created attachment 9316 [details]
Proposed patch

Hi Jon,

  Does this variation on your patch make a difference ?

  The change is to set the virtual size to the size *before* the .rsrc section
  is padded out to a file alignment boundary.  This idea was based upon viewing
  this page:

http://pedump.me/ec63e8be0717bd92c0ffbf7a21749a54/#pe

  Where the .rsrc section is shown as having a virtual size of 0x3b00 and a 
  raw size of 0x3c00.

Cheers
  Nick
Comment 8 Jon Turney 2016-06-06 13:11:09 UTC
(In reply to Nick Clifton from comment #6)
> Question: in the 1.exe post-patch file the MinorLinkerVersion field in the
> COFF
> header shows 26, but in all the other .exe files, both pre- and post- patch,
> the version number is reported as 25.  Any idea why this might be ?

This is the binutils version number.

The current cygwin package is binutils 2.25.2

I only updated ld with a build from git master with the patch applied,
2.exe and 3.exe record the version number of objcopy used to make them?
Comment 9 Jon Turney 2016-06-06 13:13:54 UTC
Created attachment 9319 [details]
Output from test case with 2nd patch applied

(In reply to Nick Clifton from comment #7)
>   Does this variation on your patch make a difference ?
> 
>   The change is to set the virtual size to the size *before* the .rsrc
> section
>   is padded out to a file alignment boundary.

This doesn't seem to help.  Testing on x86_64, 1 is still invalid, 2 and 3 are valid.

I'm not sure this is working as intended though, as the .rsrc section size is also the reported as the unpadded size.
Comment 10 Nick Clifton 2016-06-14 12:52:32 UTC
Hi Jon,

> This doesn't seem to help.  Testing on x86_64, 1 is still invalid, 2 and 3
> are valid.

Darn - in which case I am not sure what else to try.  Any suggestions ?

Cheers
  Nick
Comment 11 Jon Turney 2016-11-21 17:50:39 UTC
(In reply to Nick Clifton from comment #7)

Lookng at this again, this is quite odd behaviour from the loader.  Using my own PE dumper, which shows the VirtSize, on the testcase above:

1.exe (valid)

Name                      VirtSize   VMA        RawSize    Offset     Flags
   .text                  00000758   00001000   00000800   00000600   60500060
   .data                  00000068   00002000   00000200   00000e00   c0600040
  .rdata                  00000370   00003000   00000400   00001000   40500040
.buildid                  00000035   00004000   00000200   00001400   40300040
  .pdata                  000000d8   00005000   00000200   00001600   40300040
  .xdata                  0000007c   00006000   00000200   00001800   40300040
    .bss                  000001c0   00007000   00000000   00000000   c0600080
  .idata                  00000268   00008000   00000400   00001a00   c0300040
   .rsrc                  000010e0   00009000   00000c00   00001e00   c0300040
      /4 .debug_aranges   00000230   0000b000   00000400   00003000   42100040
     /19 .debug_info      000066a2   0000c000   00006800   00003400   42100040
     /31 .debug_abbrev    00000afc   00013000   00000c00   00009c00   42100040
     /45 .debug_line      00000e37   00014000   00001000   0000a800   42100040
     /57 .debug_frame     000002a0   00015000   00000400   0000b800   42400040
     /70 .debug_str       00000102   00016000   00000200   0000bc00   42100040
     /81 .debug_loc       00000745   00017000   00000800   0000be00   42100040
     /92 .debug_ranges    00000030   00018000   00000200   0000c600   42100040

2.exe (valid)

Name                      VirtSize   VMA        RawSize    Offset     Flags
   .text                  00000758   00001000   00000800   00000400   60500060
   .data                  00000068   00002000   00000200   00000c00   c0600040
  .rdata                  00000370   00003000   00000400   00000e00   40500040
.buildid                  00000035   00004000   00000200   00001200   40300040
  .pdata                  000000d8   00005000   00000200   00001400   40300040
  .xdata                  0000007c   00006000   00000200   00001600   40300040
    .bss                  000001c0   00007000   00000000   00000000   c0600080
  .idata                  00000268   00008000   00000400   00001800   c0300040
   .rsrc                  000010e0   00009000   00000c00   00001c00   c0300040

3.exe (invalid)

Name                      VirtSize   VMA        RawSize    Offset     Flags
   .text                  00000758   00001000   00000800   00000400   60500060
   .data                  00000068   00002000   00000200   00000c00   c0600040
  .rdata                  00000370   00003000   00000400   00000e00   40500040
.buildid                  00000035   00004000   00000200   00001200   40300040
  .pdata                  000000d8   00005000   00000200   00001400   40300040
  .xdata                  0000007c   00006000   00000200   00001600   40300040
    .bss                  000001c0   00007000   00000000   00000000   c0600080
  .idata                  00000268   00008000   00000400   00001800   c0300040
   .rsrc                  000010e0   00009000   00000c00   00001c00   c0300040
      /4 .gnu_debuglink   0000000c   0000a000   00000200   00002800   42300040

The problem seems to manifest when the .rsrc section has a VirtSize greater than it's RawSize (which I believe should just mean that the loaded section is null padded) and the following section is the .gnu_debuglink, but not when there's no following section, or it's a .debug section.

Playing around with the size of the .rsrc section, the problem first occurs when the VirtSize crosses the page alignement boundary (i.e. VirtSize = 0x1000)

> Created attachment 9316 [details]
> Proposed patch

Anyhow, we can avoid this problem by giving the  .rsrc section the right VirtSize, so this proposed patch seem the correct fix.
(although I'm not quite sure if the virt_size should be set in pinfo->output_bfd rather than pinfo->abfd?)

Unfortunately, as mentioned previously, this makes 1.exe invalid, but I think I can now see why that is:

1.exe (with patch, invalid)

Name                      VirtSize   VMA        RawSize    Offset     Flags
   .text                  00000758   00001000   00000800   00000600   60500060
   .data                  00000068   00002000   00000200   00000e00   c0600040
  .rdata                  00000370   00003000   00000400   00001000   40500040
.buildid                  00000035   00004000   00000200   00001400   40300040
  .pdata                  000000d8   00005000   00000200   00001600   40300040
  .xdata                  0000007c   00006000   00000200   00001800   40300040
    .bss                  000001a0   00007000   00000000   00000000   c0600080
  .idata                  00000268   00008000   00000400   00001a00   c0300040
   .rsrc                  00000bf8   00009000   00000c00   00001e00   c0300040
      /4 .debug_aranges   00000230   0000b000   00000400   00003000   42100040
     /19 .debug_info      000066a2   0000c000   00006800   00003400   42100040
     /31 .debug_abbrev    00000afc   00013000   00000c00   00009c00   42100040
     /45 .debug_line      00000e37   00014000   00001000   0000a800   42100040
     /57 .debug_frame     000002a0   00015000   00000400   0000b800   42400040
     /70 .debug_str       00000102   00016000   00000200   0000bc00   42100040
     /81 .debug_loc       00000745   00017000   00000800   0000be00   42100040
     /92 .debug_ranges    00000030   00018000   00000200   0000c600   42100040

This .exe is invalid because the section VMAs aren't contiguous.  .debug_aranges should have a VMA of 0000a000.

This occurs because rsrc_process_section(), which now updates the .rsrc section VirtSize, is happening after compute_section_file_positions(), so the VMAs aren't computed correctly if the .rsrc section shrinks under a page boundary.

So, another fix is needed here.  I'm trying to puzzle out where to move rsrc_process_section() to, but if you have any pointers, that would be most helpful.
Comment 12 Nick Clifton 2016-11-23 09:39:47 UTC
Hi Jon,

> This occurs because rsrc_process_section(), which now updates the .rsrc section
> VirtSize, is happening after compute_section_file_positions(), so the VMAs
> aren't computed correctly if the .rsrc section shrinks under a page boundary.
> 
> So, another fix is needed here.  I'm trying to puzzle out where to move
> rsrc_process_section() to, but if you have any pointers, that would be most
> helpful.

Hmm, tricky.  The start of coffcode.h:coff_write_object_contents() would be my 
first choice, but I have no idea what sort pf problems you might encounter...

Cheers
  Nick
Comment 13 Jon Turney 2016-11-24 14:40:38 UTC
(In reply to Nick Clifton from comment #12)
> > 
> > So, another fix is needed here.  I'm trying to puzzle out where to move
> > rsrc_process_section() to, but if you have any pointers, that would be most
> > helpful.
> 
> Hmm, tricky.  The start of coffcode.h:coff_write_object_contents() would be
> my 
> first choice, but I have no idea what sort pf problems you might encounter...

_bfd_coff_final_link() seems to be the right place.  I tried adding a hook which runs at the start of that before compute_section_file_positions() is called, but that's no good, I think because the contents of the output sections aren't actually determined at that point, so this seems a bit intractable without changing lots of stuff.
Comment 14 Jon Turney 2016-11-24 14:42:52 UTC
Created attachment 9666 [details]
Don't adjust size of PE/COFF merged .rsrc section

Here's the alternate approach, of not allowing the .rsrc section to shrink.

This seems to work ok (all the output .exe in my test case are valid), but this can waste a page in the output file (with the default manifest, potentially more if non-trivial resource sections were merged)
Comment 15 cvs-commit@gcc.gnu.org 2016-11-25 09:48:49 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=ec8f76882145c71bef81a9cadf0bf51ff9fa5b35

commit ec8f76882145c71bef81a9cadf0bf51ff9fa5b35
Author: Jon Turney <jon.turney@dronecode.org.uk>
Date:   Fri Nov 25 09:47:31 2016 +0000

    Prevent problems with section alignment by not shrinking the .rsrc section.
    
    	PR ld/20193
    	* peXXigen.c (rsrc_process_section): Do not shrink the merged
    	.rsrc section.
Comment 16 Nick Clifton 2016-11-25 10:00:39 UTC
Hi JOo,

> Here's the alternate approach, of not allowing the .rsrc section to shrink.
> 
> This seems to work ok (all the output .exe in my test case are valid), but
> this can waste a page in the output file (with the default manifest,
> potentially more if non-trivial resource sections were merged)

Wasting a page seems like a small price to pay in order to gain working
executables.  So I have checked your patch in.

Cheers
  Nick