Bug 25029 - Invalid PE file caused by discarded .rdata section
Summary: Invalid PE file caused by discarded .rdata section
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: ld (show other bugs)
Version: 2.32
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-09-23 14:37 UTC by Florin Saftoiu
Modified: 2019-12-05 13:59 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2019-10-10 00:00:00


Attachments
code to reproduce bug (538 bytes, text/plain)
2019-09-23 14:37 UTC, Florin Saftoiu
Details
Proposed patch (377 bytes, patch)
2019-10-11 11:03 UTC, Nick Clifton
Details | Diff
Proposed patch (272 bytes, patch)
2019-11-14 12:55 UTC, Nick Clifton
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Florin Saftoiu 2019-09-23 14:37:13 UTC
Created attachment 12000 [details]
code to reproduce bug

Hello,

I have attached a very simple NASM file that calls the Windows API to display a message box.
I assemble the file with nasm -f win64 test.asm -o test.o
I then link it with ld -o test_64.exe -L/c/msys64/mingw64/x86_64-w64-mingw32/lib test_64.o -lkernel32 -luser32

This used to work like a charm with ld 2.30, but produces an invalid file with ld 2.32. Specifically, the entry point address seems wrong.

I suspect this is because of this commit https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=73af69e74974eaa155eec89867e3ccc77ab39f6d

Since the .rdata section defines output symbols, it is not discarded and the . = ALIGN(4); instruction causes it to fill with 512 bytes of 0x00. However, this is not reflected in the resulting PE header
Comment 1 Florin Saftoiu 2019-09-23 16:05:22 UTC
Also, if I rename the .data section to .rdata it all works out fine.
Comment 2 Nick Clifton 2019-10-10 14:45:37 UTC
(In reply to Florin Saftoiu from comment #0)
Hi Florin,

> This used to work like a charm with ld 2.30, but produces an invalid file
> with ld 2.32. Specifically, the entry point address seems wrong.

What seems to be wrong with the entry point ?

I tried to reproduce the problem locally, but to me the entry point looks correct:

  % objdump -f test_64.exe
  [...]
  start address 0x0000000100401000

  % objdump -d test_64.exe
  [...]
  Disassembly of section .text:

  0000000100401000 <__rt_psrelocs_end>:
     100401000:	48 83 ec 08          	sub    $0x8,%rsp
     100401004:	48 83 ec 20          	sub    $0x20,%rsp

  0000000100401008 <_main.DisplayMessageBox>:
     100401008:	b9 00 00 00 00       	mov    $0x0,%ecx
  [...]

Please note, I am not a PE format expert, so there could well be something
simple that is wrong and I am just not seeing it.

Cheers
  Nick
Comment 3 Florin Saftoiu 2019-10-11 08:11:44 UTC
Hi Nick,

Thanks for your reply.

You're right about the entry point, it is correct. But Windows still wouldn't run my program so I took another look at the file.

The problem is in fact with the SizeOfImage and SizeOfHeaders fields.
If I call the data section .rdata, then I get a SizeOfImage of 0x4000 and a SizeOfHeaders of 0x200 which is correct.
But if I call the data section .data, then I get a SizeOfImage of 0x3000 and a SizeOfHeaders of 0x400.

Microsoft says (https://docs.microsoft.com/en-us/windows/win32/debug/pe-format?redirectedfrom=MSDN#optional-header-image-only) that SizeOfImage should be "The size (in bytes) of the image, including all headers, as the image is loaded in memory. It must be a multiple of SectionAlignment." and SizeOfHeaders should be "The combined size of an MS-DOS stub, PE header, and section headers rounded up to a multiple of FileAlignment."

It seems that ld considers the empty 512 bytes, from the discarded .rdata section, as part of the header. I've no hypothesis about why the SizeOfImage is messed up.

Hope this helps.

Best regards,
Florin
Comment 4 Florin Saftoiu 2019-10-11 09:13:56 UTC
PS: I actually modified the .exe file to change SizeOfImage to 0x4000 and SizeOfHeaders to 0x200 and I can now execute it in Windows.
Comment 5 Nick Clifton 2019-10-11 11:02:52 UTC
Hi Florin,

Just checking...

> The problem is in fact with the SizeOfImage and SizeOfHeaders fields.
> If I call the data section .rdata, then I get a SizeOfImage of 0x4000 and a
> SizeOfHeaders of 0x200 which is correct.
> But if I call the data section .data, then I get a SizeOfImage of 0x3000 and
> a SizeOfHeaders of 0x400.

I assume that you mean that the SizeOfImage is 0x4000 with .data, not 0x3000 ?
The actual size of the executable does not change...

> It seems that ld considers the empty 512 bytes, from the discarded .rdata
> section, as part of the header.

Agreed - and I am uploading a patch which I think will fix the problem.
Please could you give it a try and let me know if it actually works ?

Cheers
  Nick
Comment 6 Nick Clifton 2019-10-11 11:03:32 UTC
Created attachment 12038 [details]
Proposed patch
Comment 7 Florin Saftoiu 2019-10-12 23:44:08 UTC
Hi Nick,

SizeOfImage is actually 0x3000 with the section called .data and 0x4000 with the section called .rdata.

Here's the output from objdump.exe -p test_64_data.exe :
...
SizeOfImage             00003000
SizeOfHeaders           00000400
...

test_64_data.exe does NOT work.

Here's the output from objdump.exe -p test_64_rdata.exe :
...
SizeOfImage             00004000
SizeOfHeaders           00000200
...

test_64_rdata.exe does work.

I've manually edited test_64_data.exe and got the following results :
- SizeOfImage 0x3000, SizeOfHeaders 0x200 => does NOT work
- SizeOfImage 0x4000, SizeOfHeaders 0x400 => works
- SizeOfImage 0x4000, SizeOfHeaders 0x200 => works

So it seems that the bigger problem is the SizeOfImage and not the SizeOfHeaders.

Best regards,
Florin
Comment 8 Nick Clifton 2019-10-14 11:23:48 UTC
Hi Florin,

(In reply to Florin Saftoiu from comment #7)
> SizeOfImage is actually 0x3000 with the section called .data and 0x4000 with
> the section called .rdata.

> So it seems that the bigger problem is the SizeOfImage and not the
> SizeOfHeaders.

Hmmm, there seems to be a problem here, because I am not seeing a
SizeOfImage value of 0x3000 even in the .data version of test_64:


  % objdump -p test_64.ok.exe test_64.bad.exe | grep SizeOfImage
  SizeOfImage		00004000
  SizeOfImage		00004000

So maybe this is related to the toolchain being used.  How did you
configure the binutils that you are using ?  I have been testing
using a toolchain configured as  --target=x86_64-pc-cygwin.

Cheers
  Nick
Comment 9 Florin Saftoiu 2019-10-14 12:00:41 UTC
Hello,

I'm using GCC 9.2.0 and Binutils 2.32 from MSYS2. No Cygwin installed.

The target seems to be x86_64-w64-mingw32

PS ...>gcc -dumpmachine
x86_64-w64-mingw32

Are your results before or after applying your proposed patch ?
Comment 10 Nick Clifton 2019-10-21 10:15:21 UTC
Hi Florin,

> The target seems to be x86_64-w64-mingw32

Darn - I get the same results (ie SizeOfImage == 0x4000 always) with this
configuration too.

> Are your results before or after applying your proposed patch ?

Both - I tried with and without the patch applied.

Maybe it is a host alignment issue.  Are you able to run the linker
under a debugger ?  If so, please could you put a breakpoint on 
_bfd_64i_swap_aouthdr_out() and step through to where the "isize"
variable is computed ?  (Near the end of the function).  It will
probably be computed three times, but it is the last one which
should be generating the 0x4000 value.  If you can find out why
0x3000 is being generated instead that would be really helpful.

Cheers
  Nick
Comment 11 Florin Saftoiu 2019-11-13 08:54:09 UTC
Hi Nick,

Sorry it took so long, I finally got some time, so I compiled the msys2 binutils-2.32 package from source and ran it in GDB. Here's my GDB session :

$ gdb --args /c/work/projects/soft-to-you/osdev/gcc/src/MINGW-packages/mingw-w64-binutils/pkg/mingw-w64-x86_64-binutils/mingw64/bin/ld.exe -o test_64_broken.exe -L/mingw64/x86_64-w64-mingw32/lib test_64_broken.o -lkernel32 -luser32
GNU gdb (GDB) 8.3
Copyright (C) 2019 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Type "show copying" and "show warranty" for details.
This GDB was configured as "x86_64-w64-mingw32".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
    <http://www.gnu.org/software/gdb/documentation/>.

For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from C:/work/projects/soft-to-you/osdev/gcc/src/MINGW-packages/mingw-w64-binutils/pkg/mingw-w64-x86_64-binutils/mingw64/bin/ld.exe...
(gdb) dir ../gcc/src/MINGW-packages/mingw-w64-binutils/src/build-x86_64-w64-mingw32/bfd
Source directories searched: C:\work\projects\soft-to-you\osdev\src/../gcc/src/MINGW-packages/mingw-w64-binutils/src/build-x86_64-w64-mingw32/bfd;$cdir;$cwd
(gdb) b _bfd_pex64i_swap_aouthdr_out
Breakpoint 1 at 0x471b39: file pex64igen.c, line 631.
(gdb) run
Starting program: C:\work\projects\soft-to-you\osdev\gcc\src\MINGW-packages\mingw-w64-binutils\pkg\mingw-w64-x86_64-binutils\mingw64\bin\ld.exe -o test_64_broken.exe -LC:/msys64/mingw64/x86_64-w64-mingw32/lib test_64_broken.o -lkernel32 -luser32
[New Thread 37688.0x8ab4]
[New Thread 37688.0x74e0]
[New Thread 37688.0xb8a0]

Thread 1 hit Breakpoint 1, _bfd_pex64i_swap_aouthdr_out (abfd=0x5ade490,
    in=0x1c8f130, out=0x5b50920) at pex64igen.c:631
warning: Source file is more recent than executable.
631       struct internal_aouthdr *aouthdr_in = (struct internal_aouthdr *) in;
(gdb) b 739
Breakpoint 2 at 0x471e73: file pex64igen.c, line 739.
(gdb) c
Continuing.

Thread 1 hit Breakpoint 2, _bfd_pex64i_swap_aouthdr_out (abfd=0x5ade490,
    in=0x1c8f130, out=0x5b50920) at pex64igen.c:739
739               isize = (sec->vma - extra->ImageBase
(gdb) print sec->name
$1 = 0x432bfa0 ".text"
(gdb) print isize
$2 = 0
(gdb) c
Continuing.

Thread 1 hit Breakpoint 2, _bfd_pex64i_swap_aouthdr_out (abfd=0x5ade490,
    in=0x1c8f130, out=0x5b50920) at pex64igen.c:739
739               isize = (sec->vma - extra->ImageBase
(gdb) print sec->name
$3 = 0x5ad34b0 ".data"
(gdb) print isize
$4 = 8192
(gdb) c
Continuing.

Thread 1 hit Breakpoint 2, _bfd_pex64i_swap_aouthdr_out (abfd=0x5ade490,
    in=0x1c8f130, out=0x5b50920) at pex64igen.c:739
739               isize = (sec->vma - extra->ImageBase
(gdb) print sec->name
$5 = 0x5ad41e0 ".idata"
(gdb) print isize
$6 = 12288
(gdb) c
Continuing.

Thread 1 hit Breakpoint 2, _bfd_pex64i_swap_aouthdr_out (abfd=0x5ade490,
    in=0x1c8f130, out=0x5b50920) at pex64igen.c:739
739               isize = (sec->vma - extra->ImageBase
(gdb) print sec->name
$7 = 0x5ad3dd0 ".rdata"
(gdb) print isize
$8 = 16384
(gdb) print sec->vma
$9 = 4206592
(gdb) print extra->ImageBase
$10 = 4194304
(gdb) next
740                        + SA (FA (pei_section_data (abfd, sec)->virt_size)));
(gdb) next
739               isize = (sec->vma - extra->ImageBase
(gdb) next
715         for (sec = abfd->sections; sec; sec = sec->next)
(gdb) print isize
$11 = 12288
(gdb) c
Continuing.
[Thread 37688.0xb8a0 exited with code 0]
[Thread 37688.0x74e0 exited with code 0]
[Thread 37688.0x8ab4 exited with code 0]
[Inferior 1 (process 37688) exited normally]

As far as I can tell the problem comes from the .rdata section which appears in the image before the .idata section but is here considered last.

Hope this helps.

Best regards,
Florin
Comment 12 Nick Clifton 2019-11-14 12:55:41 UTC
Created attachment 12073 [details]
Proposed patch

Hi Florian,

  You are right - the sections are in a different order for me (.text, .data. .rdata, .idata).  I do not know why this is happening, but it has given me an idea for a potential patch.

  Please could you try out the patch attached to this update - along with the original patch - and see if this solves the problem.  Thanks.

Cheers
  Nick
Comment 13 Florin Saftoiu 2019-11-14 22:11:37 UTC
Hi Nick,

I've tested with both patches applied and the executable is valid and works as expected. SizeOfHeaders is 0x200 and SizeOfImage is 0x4000 as expected. So I guess the patches are OK.

Weird about the order of the sections though, I'm guessing it could cause other problems, since it's the offset of the last section in the ld script that determines the size of the image.

Let me know if I can help further.

Best regards,
Florin
Comment 14 cvs-commit@gcc.gnu.org 2019-12-05 13:57:15 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

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

commit a23e9ba17f6ab8bef1f2cc02686e8567bdc728ca
Author: Nick Clifton <nickc@redhat.com>
Date:   Thu Dec 5 13:56:07 2019 +0000

    Fix a problem computing the size fields in the PE format header.
    
    	PR 25029
    	* peXXigen.c (_bfd_XXi_swap_aouthdr_out): Ignore empty sections
    	when computing the sizes stored in the headers.
Comment 15 Nick Clifton 2019-12-05 13:59:44 UTC
Patch applied.