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.
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)
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. */
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.
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
Created attachment 9310 [details] Output from test case with patch applied
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
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
(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?
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.
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
(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.
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
(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.
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)
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.
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