Bug 25237 - Strip leaves file offset of empty PT_LOAD segment point past end of file
Summary: Strip leaves file offset of empty PT_LOAD segment point past end of file
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: binutils (show other bugs)
Version: 2.34
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL: https://bugs.launchpad.net/ubuntu/+so...
Keywords:
Depends on:
Blocks:
 
Reported: 2019-11-30 23:38 UTC by Balint Reczey
Modified: 2021-06-03 08:33 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2019-12-02 00:00:00


Attachments
Patch adjusting offset to stay in the file (816 bytes, patch)
2019-11-30 23:38 UTC, Balint Reczey
Details | Diff
Patch adjusting offset honoring page alignment (629 bytes, patch)
2019-12-03 13:50 UTC, Balint Reczey
Details | Diff
Updated patch bases on mailing list comments (626 bytes, patch)
2019-12-12 08:35 UTC, Balint Reczey
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Balint Reczey 2019-11-30 23:38:36 UTC
Created attachment 12095 [details]
Patch adjusting offset to stay in the file

This itself was tolerated for long time, but WSL's ELF loader crashes due to the offset and it seems to be easy to fix.
Comment 1 Alan Modra 2019-12-01 01:27:02 UTC
It looks to me like your fix ignores the following comment, and will therefore cause problems with other loaders.

	      /* We shouldn't need to align the segment on disk since
		 the segment doesn't need file space, but the gABI
		 arguably requires the alignment and glibc ld.so
		 checks it.  So to comply with the alignment
		 requirement but not waste file space, we adjust
		 p_offset for just this segment.
Comment 2 Balint Reczey 2019-12-01 11:12:43 UTC
It seemed to me that the changed adjustment later is still valid, honoring alignment.

Did I miss something?
Comment 3 Alan Modra 2019-12-02 01:33:44 UTC
(In reply to Balint Reczey from comment #2)
> Did I miss something?

You did.  By subtracting off_adjust from the value written to p_offset you potentially have p_offset mod page_size != p_vaddr mod page_size.

This will fail the glibc test in elf/dl-load.c resulting in "ELF load command address/offset not properly aligned"
Comment 4 Balint Reczey 2019-12-02 09:00:44 UTC
(In reply to Alan Modra from comment #3)
> (In reply to Balint Reczey from comment #2)
> > Did I miss something?
> 
> You did.  By subtracting off_adjust from the value written to p_offset you
> potentially have p_offset mod page_size != p_vaddr mod page_size.
> 
> This will fail the glibc test in elf/dl-load.c resulting in "ELF load
> command address/offset not properly aligned"

Thanks. Could you please consider accepting a fixed patch rather than closing the bug as won't fix?
While this is not an issue with traditional loaders, users of WSL would certainly appreciate strip not generating binaries crashing there.
Comment 5 Alan Modra 2019-12-02 13:24:35 UTC
Well, yes, but patches can be posted to binutils@sourceware.org without a bugzilla open.
Comment 6 Balint Reczey 2019-12-03 13:50:33 UTC
Created attachment 12101 [details]
Patch adjusting offset honoring page alignment

In my test with the previous patch p_offset mod page_size indeed did not match p_vaddr mod page_size. However the tested binary (gzip) worked ok, but I see that it is not guaranteed to work everywhere.

This newer patch ensures that p_offset's alignment is kept.
Comment 7 Balint Reczey 2019-12-12 08:35:08 UTC
Created attachment 12121 [details]
Updated patch bases on mailing list comments
Comment 8 cvs-commit@gcc.gnu.org 2019-12-13 11:37:18 UTC
The master branch has been updated by Alan Modra <amodra@sourceware.org>:

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

commit 0bc3450e220a4fb29f931ada84b546ce8993e85e
Author: Alan Modra <amodra@gmail.com>
Date:   Fri Dec 13 16:14:57 2019 +1030

    Set no file contents PT_LOAD p_offset to first page
    
    	PR 25237
    	* elf.c (assign_file_positions_for_load_sections): Attempt to
    	keep meaningless p_offset for PT_LOAD segments without file
    	contents within file size.
Comment 9 Fangrui Song 2020-02-24 02:28:00 UTC
Can be closed now.
Comment 10 Alan Modra 2020-02-24 05:14:37 UTC
Closing
Comment 11 Zena Hayes 2021-06-03 08:33:27 UTC Comment hidden (spam)