Bug 18452 - ld allows overlapping sections
Summary: ld allows overlapping sections
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: ld (show other bugs)
Version: 2.24
: P2 normal
Target Milestone: 2.27
Assignee: Alan Modra
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-24 22:41 UTC by Kenneth Almquist
Modified: 2016-04-29 22:44 UTC (History)
5 users (show)

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


Attachments
test data (490 bytes, application/gzip)
2015-05-24 22:41 UTC, Kenneth Almquist
Details
binutils LD change patch (1.35 KB, patch)
2016-03-27 06:03 UTC, Cristian Gavril Olar
Details | Diff
binutils LD individual section selection patch suggestion (3.37 KB, patch)
2016-03-28 22:46 UTC, Cristian Gavril Olar
Details | Diff
Additional possible suggestion with keyword in the linker script (4.32 KB, patch)
2016-03-29 00:20 UTC, Cristian Gavril Olar
Details | Diff
ADDING OVERLAP keyword with IGNORESECTION (4.34 KB, patch)
2016-03-29 01:02 UTC, Cristian Gavril Olar
Details | Diff
Quick thing to check (632 bytes, application/gzip)
2016-03-29 22:33 UTC, Cristian Gavril Olar
Details
Overlay BSS sections working (620 bytes, application/gzip)
2016-04-29 22:44 UTC, Cristian Gavril Olar
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Almquist 2015-05-24 22:41:44 UTC
Created attachment 8331 [details]
test data

According to section 3.10.5 (title "The Location Counter"):

    The location counter may not be moved backwards inside an output
    section, and may not be moved backwards outside of an output
    section if so doing creates areas with overlapping LMAs.

Contrary to the documentation, LD actually allows creating overlapping
sections, at least if one of the overlapping sections is the ".bss"
section.

The reason that this has the potential to be a serious problem is that
if a linker script uses fixed addresses for output sections, there
is a chance that changes to the program being linked will cause one
of the output sections to increase in size so that it overlaps the
section that follows it.  If LD doesn't treat this condition as an
error, the result is likely to be malfunctions which are difficult to
debug.  For example, if the .bss section overlaps the .text section,
then the calling one function could overwrite a portion of the code
of a completely unrelated function, and no problem would be evident
until the latter function was called.


To reproduce the problem, unpack the attached tar file into an empty
directory and run the command "sh mk".  That will assemble and link
an object file named "lib.out", and run objdump and nm on it.  The
expected behavior (if the bug has not been fixed) is that
1)  The ld command will not produce any messages.
2)  The objdump command will show that the .text and .bss segments
    overlap.
3)  The nm command will show that the linker has assigned the same
    address to both main (a function) and var2 (a variable).


The following command output describes the system I am using:
$ /usr/bin/ld -version
GNU ld (GNU Binutils for Ubuntu) 2.24
Copyright 2013 Free Software Foundation, Inc.
This program is free software; you may redistribute it under the terms of
the GNU General Public License version 3 or (at your option) a later version.
This program has absolutely no warranty.
$ ldd /usr/bin/ld
	linux-vdso.so.1 =>  (0x00007fff21b1e000)
	libbfd-2.24-system.so => /usr/lib/libbfd-2.24-system.so (0x00007fdf47641000)
	libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007fdf4743d000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fdf47077000)
	libz.so.1 => /lib/x86_64-linux-gnu/libz.so.1 (0x00007fdf46e5e000)
	/lib64/ld-linux-x86-64.so.2 (0x00007fdf47985000)
$ ls -l /lib/x86_64-linux-gnu/libdl.so.2
lrwxrwxrwx 1 root root 13 Feb 25 11:56 /lib/x86_64-linux-gnu/libdl.so.2 -> libdl-2.19.so
$ ls -l /lib/x86_64-linux-gnu/libc.so.6
lrwxrwxrwx 1 root root 12 Feb 25 11:56 /lib/x86_64-linux-gnu/libc.so.6 -> libc-2.19.so
$ ls -l /lib/x86_64-linux-gnu/libz.so.1
lrwxrwxrwx 1 root root 13 Aug 14  2014 /lib/x86_64-linux-gnu/libz.so.1 -> libz.so.1.2.8
$ uname --kernel-name
Linux
$ uname --kernel-release
3.13.0-36-generic
$ uname --kernel-version
#63-Ubuntu SMP Wed Sep 3 21:30:07 UTC 2014
$ uname --machine
x86_64
$ lsb_release -d
Description:	Ubuntu 14.04.2 LTS
$ # CPU is Intel i5-4690K (Haswell)
Comment 1 Alan Modra 2015-05-25 09:42:49 UTC
Confirmed.  This is where we update the region "current" location, which lacks any sort of check.
		if (output_section_statement == abs_output_section)
		  {
		    /* If we don't have an output section, then just adjust
		       the default memory address.  */
		    lang_memory_region_lookup (DEFAULT_MEMORY_REGION,
					       FALSE)->current = newdot;
		  }

The behaviour goes back forever as far as I can tell. This code was introduced with commit 27f7237e.
Comment 2 Matt Grochowalski 2016-03-17 17:46:41 UTC
https://sourceware.org/git/?p=binutils.git;a=commit;h=ec457111c02dd5a5e29ce7ce30e95a17ef470c78

The linker previously detected section conflicts involving the .bss section, but this commit in 2009 changed it to not check sections that's aren't loaded from the executable (I'm not clear what the reason for this was).

https://sourceware.org/git/?p=binutils.git;a=commit;h=ac3f93dc025a4dcf2c7dcf12039c46555b8a7ecd

It was changed shortly after that to ignore sections that aren't allocated as well.

"!(s->flags & SEC_LOAD) || !(s->flags & SEC_ALLOC)" should likely be "!(s->flags & SEC_LOAD) && !(s->flags & SEC_ALLOC)". Or just "!(s->flags & SEC_ALLOC)" (can a section be SEC_LOAD but not SEC_ALLOC?).
Comment 3 Alan Modra 2016-03-17 22:47:29 UTC
See https://sourceware.org/ml/binutils/2009-05/msg00330.html for why lang_check_section_addresses can't check bss.
Comment 4 Cristian Gavril Olar 2016-03-27 06:03:24 UTC
Created attachment 9132 [details]
binutils LD change patch

Like the link in the last comment shows, this behavior is sane because BSS sections have to be able to overlap between themselves and data sections for overlay reasons. But as one user of LD who both needs overlaps but at the same time has been bitten by longish debugging hours because of not realizing that I had incidentally created unwanted/accidental overlaps I suggest the following change:
1) Have LD inform by default if there are noload sections overlapping other sections
2) If someone saw the information, became aware of the overlap and consciously makes a choice to quiet down that information, provide a LD flag that would silence that extra verbosity

The attached patch is a suggestion of such a change. Basically, in a default run, LD would just print information of the sort of:
"legal section overlap of section .text loaded at [0000000000008010,000000000000801f] overlapping section .bss loaded at [0000000000008000,000000000000801f]. See https://sourceware.org/ml/binutils/2009-05/msg00330.html"
And if the user decides that they have become aware and that for their case this is a legal use, they could silence the information by using a "--no-check-overlap-noload-sections" flag sent to LD.

The patch is done against binutils 2.24.
Comment 5 Andrei Lupas 2016-03-28 08:30:24 UTC
(In reply to Alan Modra from comment #3)
> See https://sourceware.org/ml/binutils/2009-05/msg00330.html for why
> lang_check_section_addresses can't check bss.

The "relaxed" checks above (suitable for LMA mapping into ROM scenario) means hours of debug for other people (in my case ALLOC .bss section silently overlapped .text section; LMA = VMA = RAM addresses).

I don't think LD "can't check .bss" is an acceptable answer; I think LD should check (as before) ALLOC section overlap (by default or optionally).
Comment 6 Cristian Gavril Olar 2016-03-28 22:46:24 UTC
Created attachment 9135 [details]
binutils LD individual section selection patch suggestion

I've been thinking a little bit more about this and I realized in the same application one can have both the problem of accidental overlaps but at the same time, in the same application have a need to create overlapped noload sections.

A reference to this usecase was hinted I think previously when the text in the endnote of https://sourceware.org/ml/binutils/2009-05/msg00330.html .

As a linker script evolves, one may have additions or changes to the linker script(s) used. If at some point one had enabled a general option to not warn anymore, one may still get later in trouble if a new section is accidentally overlapped.

As a consequence I'm thinking that the previous suggestion should be changed a little bit to something of this sort:
1) By default LD should just not allow section overlapping of any sort but
2) During the checks, if a section overlapping is detected to be a NOLOAD section then LD, even if not linking, could display a message informing the user that they can consciously allow the overlap if they mark that particular NOLOAD section as possible to be overlapped
3) Provide an option to allow the overlap of a NOLOAD section as per the description at point "2)"

Thinking about this problem I had thought of maybe adding an extra flag to the bfd_section structure may be one solution, but the bits in the flagword are pretty expensive I think to burn for this, so instead I thought of keeping a separate list in the bfd alive which holds the names of desired sections to be overlapped.

In terms of how to specify which sections are possible to be overlapped, I thought of just overloading the use of "OVERLAY". But then I'm aware that OVERLAY is mostly just meant to be syntactic sugar (ftp://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_node/ld_22.html), and the way it is implemented it actually forced the sections to have the LOAD flag set. I think changing that may have too great an implication worldwide and I don't think it's worth it.

Considering all of these I think the remaining option is to build the list of accepted NOLOAD sections to be overlapped a list of options in as command line arguments. This patch is suggesting one way of doing that.

Additionally, if there is interest in this direction, I guess an additional improvement that could be done would be to add a ld script new option called "OVERLAP" or something similar that would allow the sections to be added to the accepted list in this fashion too.
Comment 7 Alan Modra 2016-03-29 00:12:39 UTC
Since there seems to be enough interest in this bug for people to actually write patches, here is what I think would work and be accepted:

In lang_size_sections_1, near the comment "A backwards move of dot", set a static global var whenever seeing "dot < last->vma".  This should flag all cases where overlays are used without false triggers.

Modify lang_check_section_addresses to check for vma overlap when no overlays are detected.
- Use IGNORE_SECTION again to choose interesting sections (the attached patch was wrong in treatment of .tbss)
- In the existing lma checking loop, ignore !SEC_LOAD sections.  Some rewriting might be necessary to elegantly handle the case of the first section being !SEC_LOAD.
- Write another loop to check vmas if overlays not present
Comment 8 Cristian Gavril Olar 2016-03-29 00:20:25 UTC
Created attachment 9136 [details]
Additional possible suggestion with keyword in the linker script

Another suggestion with similar structure as the one before, but with accepting an (OVERLAP) keyword to specify a section is acceptable to be overlapped via ld script syntax. Choice of the word or method could be changed if this ever gets adopted of course. Mostly here to suggest an idea.
Comment 9 Cristian Gavril Olar 2016-03-29 01:02:32 UTC
Created attachment 9137 [details]
ADDING OVERLAP keyword with IGNORESECTION

(In reply to Alan Modra from comment #7)
> Since there seems to be enough interest in this bug for people to actually
> write patches, here is what I think would work and be accepted:
> 
> In lang_size_sections_1, near the comment "A backwards move of dot", set a
> static global var whenever seeing "dot < last->vma".  This should flag all
> cases where overlays are used without false triggers.
> 

But this would detect all overlays. It still does not resolve the problem that some overlays are intentional and others are not. This depends on user choice. The fact that one section is or isn't !SEC_LOAD does not necessarily mean that that is a good case or a bad case. We'd be in the same situation as before the 2009 patch, would we not?

> Modify lang_check_section_addresses to check for vma overlap when no
> overlays are detected.
> - Use IGNORE_SECTION again to choose interesting sections (the attached
> patch was wrong in treatment of .tbss)

Point taken. Reattaching last patch with IGNORE_SECTION back in there. Saw the original 2.24 source was still leaving it out so I left it out too, although I had noticed the difference and was a little bit curious why that define is still left in there for apparently no particular reason.

> - In the existing lma checking loop, ignore !SEC_LOAD sections.  Some
> rewriting might be necessary to elegantly handle the case of the first
> section being !SEC_LOAD.

Not all !SEC_LOAD sections should be ignored either, or else we'd be in the same situation as we are today (well, almost). The problem is specifically marking which sections are intentional overlay vs which ones are not. The OVERLAY keyword is there, yes, but reading through the world knowledge base it has a different meaning right now so I would not recommend hijacking that keyword for this.

> - Write another loop to check vmas if overlays not present
This assumes that whoever has overlays intends to have his whole application accept all overlays, and only who does not have overlays at all accepts a clean application.

I think these suggestions would be OK if the case would be that it is a general flag that is required to be detected, as per the very first patch submitted, but thinking more about things, I do not think that is true. The same application can have valid overlays and invalid ones, I cannot see any way around this other than allowing the user to specifically mark which ones are valid.
Comment 10 Cristian Gavril Olar 2016-03-29 01:34:27 UTC
(In reply to Cristian Gavril Olar from comment #9)
> I think these suggestions would be OK if the case would be that it is a
> general flag that is required to be detected, as per the very first patch
> submitted, but thinking more about things, I do not think that is true. The
> same application can have valid overlays and invalid ones, I cannot see any
> way around this other than allowing the user to specifically mark which ones
> are valid.

But that being said, if there would be a way to use the existing ld script commands to infer somehow an intentional overlap of noload sections that can clearly show intention and could not happen by mistake in any way, it would also be my preference to reuse existing syntax/options completely it is just that I keep thinking about this and I cannot see any way with just the existing options and syntax.

As far as I've noticed:
- OVERLAY creates SEC_LOAD sections rather than just allocated (and it has too, in order to perform the function it needs with creation of the extra symbols)
- NOLOAD sections will not increment the LMA: and that is just, because otherwise it would start eating up load memory in potentially ROM code (undesirable), so one cannot force increments on NOLOAD sections. I'm mentioning this particular point because I was initially considering that a possibility of handling this would be to use the current syntax and specify something of this sort:
  /*Make sure on the data side of CMNlibs NOLOAD is used but addresses for data sections are virtual*/
  .some.bss 0x1C000000 (NOLOAD) : AT (0xE0000000) { 
        *(S.bss*)
        *(.__BSS__sect*)
  }
  .other.bss 0x1C000000 (NOLOAD) : AT ( LOADADDR (.some.bss) + SIZEOF (.some.bss) ) {      
        *(.other.S.bss*)
        *(.other.__BSS__sect*)
  }

In my initial opinion I thought this would be a good way to use the current syntax to express intention of overlaps, and then just do LMA checks. But the load address would not increment in this fashion. Initially I did not realize why but reading this ticket and the links in it I realized it makes sense for this to not work because NOLOAD sections cannot increment the LMA address, or else, as the note in https://sourceware.org/ml/binutils/2009-05/msg00330.html says: "Clearly we don't want an image of the bss section in ROM, and the .bss section does not cause the advancement of the allocation point (the LMA)."

So it makes sense that LMA does not increase. But I think, at the same time, this shows why with classic commands this problem of intentional vs accidental use cannot be resolved without creating some sort of reinterpretation of the commands which I think is not desirable.

But like I said: if it would help anyone, I'd be interested in trying to provide patches/suggestions for a resolution to this problem of: detecting accidental overlaps but allowing intentional ones with classic commands if there is any way of doing that.
Comment 11 Andrei Lupas 2016-03-29 10:14:31 UTC
(In reply to Alan Modra from comment #7)
> Since there seems to be enough interest in this bug for people to actually
> write patches, here is what I think would work and be accepted:
> 
> In lang_size_sections_1, near the comment "A backwards move of dot", set a
> static global var whenever seeing "dot < last->vma".  This should flag all
> cases where overlays are used without false triggers.
> 
> Modify lang_check_section_addresses to check for vma overlap when no
> overlays are detected.
> - Use IGNORE_SECTION again to choose interesting sections (the attached
> patch was wrong in treatment of .tbss)
> - In the existing lma checking loop, ignore !SEC_LOAD sections.  Some
> rewriting might be necessary to elegantly handle the case of the first
> section being !SEC_LOAD.
> - Write another loop to check vmas if overlays not present

This would be great (would definitely solve my problem; ld scripts I used didn't use OVERLAY command).
Comment 12 Cristian Gavril Olar 2016-03-29 22:33:35 UTC
Created attachment 9141 [details]
Quick thing to check

I'd like to quickly check something with you all. See this attached modified version of the first testcase. I just did a quick check, didn't yet make all of the changes suggested but just plaed some printfs like:

if (dot < last->vma
		    && os->bfd_section->size != 0
		    && dot + os->bfd_section->size <= last->vma)
...
			printf("Overlays detected!\n");

...
		  }
		else
		  {
			printf("Overlays not detected!\n");

I notice with this, as was my expectation, there are no overlays detected, so then, with no overlays detected, the !SEC_LOAD would pass. But the VMA address check would not. Still this is an intended overlay for my usecase in there. It is just that the intended overlap is of nolaod sections not load sections. Because in either way I do not want nor need memory eaten up by neither of the .bss or .otherbss sections.

This usecase is extremely valid and required for me. I'd like to understand the position on this. I would have preferred to stay in tune with vanilla binutils but if it must be, I guess that's the beauty of GPL :) -> we'll just have to fork binutils into something that accepts this use case.

If this usecase is accepted as needed (as it definitely is for my usecase) then I would be open to discuss further and help if possible with implementing support for it. Otherwise I wont' bother anymore and go with forking.
Comment 13 Cristian Gavril Olar 2016-03-29 22:35:37 UTC
Thank you for your patience and contributions to this thread so far. It was in any case very instructive.
Comment 14 Sourceware Commits 2016-03-30 07:35:23 UTC
The master branch has been updated by Alan Modra <amodra@sourceware.org>:

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

commit a87dd97a2098b7e18ff2574a4e81ae521ef7e6f2
Author: Alan Modra <amodra@gmail.com>
Date:   Wed Mar 30 17:35:14 2016 +1030

    PR18452, ld allows overlapping sections
    
    	PR 18452
    	* ldlang.c (maybe_overlays): New static var.
    	(lang_size_sections_1): Set it here.
    	(struct check_sec): New.
    	(sort_sections_by_lma): Adjust for array of structs.
    	(sort_sections_by_vma): New function.
    	(lang_check_section_addresses): Check both LMA and VMA for overlap.
    	* testsuite/ld-scripts/rgn-over7.d: Adjust.
Comment 15 Alan Modra 2016-03-30 07:38:10 UTC
I think the patch I've just committed should catch all the common cases of accidental overlap.  Please try it out.
Comment 16 Alan Modra 2016-04-08 05:00:59 UTC
As I was reminded in private email, the patch does not in fact fix the testcase.  I'd still like to detect overlays automatically, even ones not using the syntactic sugar of the OVERLAY keyword.  So...  Perhaps it is reasonably to detect overlays when the current section vma exactly matches the previous section vma.  That's what you get when using OVERLAY, and the example of overlays without using OVERLAY in ld.texinfo also starts each overlay section at the same address.
Comment 17 Sourceware Commits 2016-04-08 14:14:15 UTC
The master branch has been updated by Alan Modra <amodra@sourceware.org>:

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

commit 136a43b762ce7bc692645cc0d9d50c934f9aa392
Author: Alan Modra <amodra@gmail.com>
Date:   Fri Apr 8 17:52:03 2016 +0930

    PR18452, ld allows overlapping sections
    
    	PR 18452
    	* ldlang.c (maybe_overlays): Delete.
    	(lang_size_sections_1): Remove code setting maybe_overlays.
    	(lang_check_section_addresses): Instead detect overlays by
    	exact match of section VMAs here.  Fix memory leak.
Comment 18 Alan Modra 2016-04-08 15:05:48 UTC
Fixed
Comment 19 Cristian Gavril Olar 2016-04-29 22:44:17 UTC
Created attachment 9225 [details]
Overlay BSS sections working

Thank you for this fix. I had a chance to try it out today and yes, this fixes both the original testcase as well as allowing the functionality we require co-exist. Attaching additional testcase here with the additional functionality tested with the last PR which still works.

Not sure what's the process of getting from RESOLVED to CLOSED here but as far as I can tell this is a successfully verified fix.