Bug 25662 - objcopy sets invalid sh_offset for the first section in a no_contents segment containing program headers
Summary: objcopy sets invalid sh_offset for the first section in a no_contents segment...
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: binutils (show other bugs)
Version: 2.34
: P2 normal
Target Milestone: 2.35
Assignee: Alan Modra
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-03-12 14:24 UTC by Jozef Lawrynowicz
Modified: 2020-09-08 17:05 UTC (History)
0 users

See Also:
Host:
Target:
Build:
Last reconfirmed: 2020-03-25 00:00:00


Attachments
patch (625 bytes, patch)
2020-03-12 14:46 UTC, Jozef Lawrynowicz
Details | Diff
original executable (51.99 KB, application/x-executable)
2020-03-13 09:58 UTC, Jozef Lawrynowicz
Details
Alternate patch (849 bytes, patch)
2020-03-24 12:45 UTC, Alan Modra
Details | Diff
testcase (2.22 KB, patch)
2020-03-25 14:55 UTC, Jozef Lawrynowicz
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jozef Lawrynowicz 2020-03-12 14:24:13 UTC
The Binutils "simple objcopy of executable" test is failing for
msp430-elf/-mlarge since

  commit 30fe183248b2523ecff9da36853e2f893c4c4b91
  Author: Alan Modra <amodra@gmail.com>
  Date:   Wed Oct 23 17:40:51 2019 +1030

      PR4499, assign file positions assumes segment offsets increasing

This is similar to PR23595, where the fact that the first segment contains the
program headers and a section of type SHT_NOBITS causes problems.

This time, the difference between the original executable file, and the output
from copying that file using objcopy is the sh_offset of the .bss section, which
is the only section in the first segment, and also contains the program
headers.

Original executable:
  Section Headers:
    [Nr] Name  Type     Addr     Off    Size   ES Flg Lk Inf Al
    [ 5] .data PROGBITS 00000500 0001b0 00013e 00  WA  0   0  2
    [ 7] .bss  NOBITS   0000063e 000116 000028 00  WA  0   0  2
  ...
  Program Headers:
    Type   Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
    LOAD   0x000000 0x00000528 0x00000528 0x00114 0x0013e RW  0x4
  ...
   Section to Segment mapping:
    Segment Sections...
     00     .bss

Objcopy output executable:
  Section Headers:
    [Nr] Name  Type     Addr     Off    Size   ES Flg Lk Inf Al
    [ 5] .data PROGBITS 00000500 0001b0 00013e 00  WA  0   0  2
    [ 7] .bss  NOBITS   0000063e 000114 000028 00  WA  0   0  2

Note 0x116 sh_offset in the original, and 0x114 sh_offset in the objcopy output.

The problem is that copy_elf_program_header calculates a p_vaddr_offset of -2
for the segment, since .bss is offset 2 bytes from the end of the program
headers in that segment, due to the required alignment of the segment.

So p_vaddr is temporarily being calculated as 0x63c, which means in
assign_file_positions_for_load_sections segment aligment is not required
since (0x63c + 0x114) % 4 == 0.

Hence the .bss sh_offset is output incorrectly - (0x63e + 0x114) % 4 = 2.
Since .bss is the first loadable section in the segment, it must be aligned to
the segment align of 4 rather than the section alignment of 2, right?
Comment 1 Jozef Lawrynowicz 2020-03-12 14:46:11 UTC
Created attachment 12368 [details]
patch

The attached patch fixes the issue, although I haven't been able to totally
convince myself of the reasoning behind the change.

In this situation, assign_file_positions_for_load_sections will align the
first section in the segment to the required segment alignment, but then to
avoid wasting space, the offset of the next segment will be adjusted back to the
end of the no_contents segment, even if that offset overlaps the SHT_NOBITS
section.

So in a similar manner we can disregard any "padding" between the start of the
no_contents segment and the first SHT_NOBITS section, by keeping
p_vaddr_offset set to 0. The gap between the segment and section start
is not used as padding in the normal sense to adjust the address of later
sections and segments, it is just there to align that first SHT_NOBITS section.

If that sounds about right I'll post the patch to the mailing list for approval.
Comment 2 Alan Modra 2020-03-13 08:39:11 UTC
Please attach the original executable.  I'd like to see all the program headers.
Comment 3 Jozef Lawrynowicz 2020-03-13 09:58:36 UTC
Created attachment 12370 [details]
original executable

Attached the original executable file.
Comment 4 Alan Modra 2020-03-24 12:45:30 UTC
Created attachment 12403 [details]
Alternate patch

This also fixes the testcase.  I'm inclined to go this way since it's really the lack of sh_offset adjustment for nobits sections that is breaking things for you.  What do you think?
Comment 5 Jozef Lawrynowicz 2020-03-24 21:54:04 UTC
(In reply to Alan Modra from comment #4)
> Created attachment 12403 [details]
> Alternate patch
> 
> This also fixes the testcase.  I'm inclined to go this way since it's really
> the lack of sh_offset adjustment for nobits sections that is breaking things
> for you.  What do you think?

Looks good to me, I prefer the reasoning behind your changes to what I originally suggested as well.

I successfully regtested your patch for msp430-elf in the -msmall and -mlarge configurations.

Thanks!
Comment 6 Sourceware Commits 2020-03-25 04:22:02 UTC
The master branch has been updated by Alan Modra <amodra@sourceware.org>:

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

commit d16e3d2e5b717cf83c1fe47a70e0a9f2c2024a44
Author: Alan Modra <amodra@gmail.com>
Date:   Tue Mar 24 10:42:45 2020 +1030

    PR25662, invalid sh_offset for first section in segment with phdrs
    
            PR 25662
            * elf.c (assign_file_positions_for_load_sections): Adjust offset
            for SHT_NOBITS section if first in segment.
Comment 7 Alan Modra 2020-03-25 04:41:22 UTC
Updated patch applied.  Jozef, perhaps you could add a testcase to ensure this doesn't regress.  Ideally one that doesn't require a C compiler.
Comment 8 Jozef Lawrynowicz 2020-03-25 14:55:45 UTC
Created attachment 12404 [details]
testcase

(In reply to Alan Modra from comment #7)
> Updated patch applied.  Jozef, perhaps you could add a testcase to ensure
> this doesn't regress.  Ideally one that doesn't require a C compiler.

How about the attached patch?
I tested that it fails on x86_64-pc-linux-gnu, msp430-elf and arm-eabi before your patch, and passes with the patch applied.
Comment 9 Alan Modra 2020-03-26 07:05:24 UTC
The testcase patch looks mostly good, thanks.  At first I thought you had the remote host file names wrong, but it doesn't seem too important what file names are used on the host.

Some other issues:
- The new test is using an ELF .type directive.  Either remove that or make the test ELF only.
- The test will error if ld isn't built, as is the case for a few targets, ia64-vms being an example.

Please post to the mailing list for any further discussion.
Comment 10 Jozef Lawrynowicz 2020-03-26 11:14:23 UTC
(In reply to Alan Modra from comment #9)
> The testcase patch looks mostly good, thanks.  At first I thought you had
> the remote host file names wrong, but it doesn't seem too important what
> file names are used on the host.
> 
> Some other issues:
> - The new test is using an ELF .type directive.  Either remove that or make
> the test ELF only.
> - The test will error if ld isn't built, as is the case for a few targets,
> ia64-vms being an example.
> 
> Please post to the mailing list for any further discussion.

I posted an updated patch here https://sourceware.org/pipermail/binutils/2020-March/110408.html.

BTW, the test fails for i386-pe, but as expected it's not fixed by your patch to elf.c. A quick comparison of objdump -x output shows that the "Time/Date" field has been reset to epoch 0.
Comment 11 Sourceware Commits 2020-03-27 10:56:10 UTC
The master branch has been updated by Jozef Lawrynowicz <jozefl@sourceware.org>:

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

commit 1fafefd59438f92206e65ea6b2022d82c5ccfe12
Author: Jozef Lawrynowicz <jozef.l@mittosystems.com>
Date:   Fri Mar 27 10:54:26 2020 +0000

    Add testcase for PR 25662 invalid sh_offset for section
    
    binutils/ChangeLog:
    
    2020-03-27  Jozef Lawrynowicz  <jozef.l@mittosystems.com>
    
            PR binutils/25662
            * testsuite/binutils-all/objcopy.exp (objcopy_test): Add argument to
            specify whether an object file or executable should be built and tested.
            Change test names to report whether an object file or executable is
            being tested.
            * testsuite/binutils-all/pr25662.ld: New test.
            * testsuite/binutils-all/pr25662.s: New test.
Comment 12 Sourceware Commits 2020-09-08 17:05:48 UTC
The master branch has been updated by H.J. Lu <hjl@sourceware.org>:

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

commit 790147a9e9ee05542c621a36669288413880c876
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Tue Sep 8 10:01:45 2020 -0700

    Pass --disable-reloc-section on PE targets for PR 25662 test
    
    Pass --disable-reloc-section on PE targets for PR 25662 test since
    
    commit 514b4e191d5f46de8e142fe216e677a35fa9c4bb
    
    Author: Jeremy Drake <sourceware-bugzilla@jdrake.com>
    Date:   Thu Aug 27 12:58:27 2020 +0100
    
        Change the default characteristics of DLLs built by the linker to more secure settings.
    
    defaulted to --enable-reloc-section.
    
            PR ld/26587
            * testsuite/binutils-all/objcopy.exp: Pass --disable-reloc-section
            to ld on PE targets for PR 25662 test.