[PATCH] Fix overlapping sections in ELF files when a memory segment has no contents

[Jozef Lawrynowicz <jozef dot l at mittosystems dot com>]

On targets which have small page size, or no concept of page size, memory
segments in the output file may require little or no alignment.
An optimization in bfd/elf.c:assign_file_positions_for_load_sections, to save
some space in the output file when a segment has no contents, can cause sections
to overlap on these targets.
This was exposed when running the "simple objcopy of executable" test for
msp430-elf with the -mlarge target flag, which failed due to different
section and segment offsets between the original ELF file and the ELF file
output from objcopy.

Here are the relevant parts of the "readelf -a" output of the linked executable
from the above test:

======
Section Headers:
  [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al
...
  [ 2] .rodata           PROGBITS        00002000 000134 000090 00   A  0   0  2
...
  [ 7] .bss              NOBITS          00000646 000136 00002c 00  WA  0   0  2
...
Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  LOAD           0x000000 0x00000510 0x00000510 0x00134 0x00162 RW  0x4
  LOAD           0x000134 0x00002000 0x00002000 0x00090 0x00090 R   0x4
...
 Section to Segment mapping:
  Segment Sections...
   00     .bss
   01     .rodata
======
The first segment contains only .bss. .data has its VMA in this segment, but its
LMA is elsewhere, which is partly why this segment has size, despite its
sections having no contents. This segment also contains some ELF headers which
is where the rest of the file size comes from.

As can be seen in the above readelf output, the offset of the .bss section
(0x136), is not within the memory segment that's supposed to contain it. This
segment ends at 0x134. When objcopy copies this ELF file, the new ELF file has
both the .rodata section offset, and the file offset for the segment containing
.rodata, set to 0x138. File size for the segment containing .bss is also
incremented to 0x136.

The attached patch fixes the cause of the overlapping sections, by incrementing
the segment filesize, and not decrementing output file offset, if it appears the
next segment would start before the end of the current segment.

Testing shows no regressions from the binutils, gas and ld testsuites on
x86_64-pc-linux-gnu and msp430-elf. This patch fixes the "simple objcopy of
executable" binutils test for msp430-elf with the -mlarge target flag.

If the patch is acceptable, I would appreciate if someone would commit it for
me as I don't have write access.

>From b5319b34db32905d1c2ff761a8078477d3b596e0 Mon Sep 17 00:00:00 2001
From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
Date: Tue, 22 May 2018 18:15:31 +0100
Subject: [PATCH] Fix overlapping sections in ELF files when a memory segment
 has no contents

2018-05-23  Jozef Lawrynowicz  <jozef.l@mittosystems.com>

	* bfd/elf.c (max_sec_align_in_segment): New.
	  (assign_file_positions_for_load_sections): Add off_adjust to segment
	  filesize, and don't subtract off_adjust from off, if the next section
	  will have file offset less than the current section file offset.

---
 bfd/elf.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 58 insertions(+), 12 deletions(-)

diff --git a/bfd/elf.c b/bfd/elf.c
index 024b6cd..f91a3ac 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -5227,6 +5227,24 @@ write_zeros (bfd *abfd, file_ptr pos, bfd_size_type len)
   return ret;
 }
 
+static bfd_size_type
+max_sec_align_in_segment (bfd *abfd, struct elf_segment_map *m)
+{
+  asection **secpp;
+  unsigned int i;
+  unsigned int align_power = 0;
+  for (i = 0, secpp = m->sections; i < m->count; i++, secpp++)
+    {
+      unsigned int secalign;
+
+      secalign = bfd_get_section_alignment (abfd, *secpp);
+      if (secalign > align_power)
+	align_power = secalign;
+    }
+  return (bfd_size_type) 1 << align_power;
+}
+
+
 /* Assign file positions to the sections based on the mapping from
    sections to segments.  This function also sets up some fields in
    the file header.  */
@@ -5384,21 +5402,12 @@ assign_file_positions_for_load_sections (bfd *abfd,
 	  && m->count > 0)
 	{
 	  bfd_size_type align;
-	  unsigned int align_power = 0;
 
 	  if (m->p_align_valid)
 	    align = p->p_align;
 	  else
 	    {
-	      for (i = 0, secpp = m->sections; i < m->count; i++, secpp++)
-		{
-		  unsigned int secalign;
-
-		  secalign = bfd_get_section_alignment (abfd, *secpp);
-		  if (secalign > align_power)
-		    align_power = secalign;
-		}
-	      align = (bfd_size_type) 1 << align_power;
+	      align = max_sec_align_in_segment (abfd, m);
 	      if (align < maxpagesize)
 		align = maxpagesize;
 	    }
@@ -5527,10 +5536,47 @@ assign_file_positions_for_load_sections (bfd *abfd,
 	  else
 	    {
 	      file_ptr adjust;
+	      bfd_size_type next_align;
+	      bfd_vma next_off_adjust;
+	      file_ptr next_off;
+	      bfd_vma next_vma;
 
 	      adjust = off - (p->p_offset + p->p_filesz);
-	      if (!no_contents)
-		p->p_filesz += adjust;
+	      /* On some targets, segment alignment can be very small.
+		 If this is a no_contents segment, and "adjust" coincidentally
+		 aligns the next segment, sections in the next segment may have
+		 file offset less than the file offset of sections in the
+		 current segment.
+		 Furthermore, empty sections in the current segment may have
+		 file offset >= to the file offset of the next segment.
+		 In this case, p_filesz for the current segment needs to be
+		 incremented by adjust, so the empty section(s) in this segment
+		 start within it.
+		 Also set off_adjust to 0, so the next section's offset isn't
+		 less than the current section offset.
+		 If this is the last segment we can save some space since
+		 nothing comes after it.
+		 These next_* variables are accurate.  The value of the operands
+		 in these calculations won't change between this segment and
+		 the next, if this is a "no_contents" segment.  */
+	      next_align = (m->next != NULL
+			    ? max_sec_align_in_segment (abfd, m->next) : 1);
+	      if (next_align < maxpagesize)
+		next_align = maxpagesize;
+	      next_vma = (m->next != NULL && m->next->count > 0
+			  ? m->next->sections[0]->vma - m->next->p_vaddr_offset
+			  : 0);
+	      /* Set the value for next_off to what the offset would be if
+		 this adjustment isn't performed.  */
+	      next_off = off - off_adjust;
+	      next_off_adjust = vma_page_aligned_bias (next_vma, next_off,
+						       next_align);
+	      if (!no_contents || (off_adjust > next_off_adjust
+				   && m->next != NULL))
+		{
+		  p->p_filesz += adjust;
+		  off_adjust = 0;
+		}
 	      p->p_memsz += adjust;
 	    }
 	}
-- 
2.7.4