Commit: objcopy: do not set the 'share' flag on ELF binaries

Commit: objcopy: do not set the 'share' flag on ELF binaries

[Nick Clifton]

Hi Guys,

  It was recently pointed out to me that using objcopy to add the
  "share" section flag to a section in an ELF format binary would
  trigger an abort in the BFD library.  It turns out that objcopy uses
  the SEC_COFF_SHARE flag bit for "share", which just happens to have
  the same value as SEC_ELF_COMPRESS.  Trying to compress an ELF section
  just by changing its flag bit is a bad idea and this is what was
  triggering the abort.

  So I am checking in the patch below.  It replaces the abort with some
  more helpful error messages and a failure result.  It updates the
  binutils documentation to note that you cannot set the "share" flag
  bit on non-COFF sections.  Plus it fixes objcopy so that it will
  reject the share flag if the output file is in the ELF format.

Cheers
  Nick

binutils/ChangeLog
2020-03-06  Nick Clifton  <nickc@redhat.com>

	* objcopy.c (check_new_section_flags): New function.  Reject the
	SEC_COFF_SHARED flag if the target is not a COFF binary.
	(copy_object): Call check_new_section_flags.
	(setup_section): Likewise.
	* doc/binutils.texi (objcopy): Add a note that the 'share' section
	flag cannot be applied to ELF binaries.

bfd/ChangeLog
2020-03-06  Nick Clifton  <nickc@redhat.com>

	* elf.c (_bfd_elf_set_section_contents): Replace call to abort
	with error messages and failure return values.

diff --git a/bfd/elf.c b/bfd/elf.c
index 747d120101..e6db2ff64d 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -9181,20 +9180,47 @@ _bfd_elf_set_section_contents (bfd *abfd,
   hdr = &elf_section_data (section)->this_hdr;
   if (hdr->sh_offset == (file_ptr) -1)
     {
+      unsigned char *contents;
+
       if (bfd_section_is_ctf (section))
 	/* Nothing to do with this section: the contents are generated
 	   later.  */
 	return TRUE;
 
-      /* We must compress this section.  Write output to the buffer.  */
-      unsigned char *contents = hdr->contents;
-      if ((offset + count) > hdr->sh_size
-	  || (section->flags & SEC_ELF_COMPRESS) == 0
-	  || contents == NULL)
-	abort ();
+      if ((section->flags & SEC_ELF_COMPRESS) == 0)
+	{
+	  _bfd_error_handler
+	    (_("%pB:%pA: error: attempting to write into an unallocated compressed section"),
+	     abfd, section);
+	  bfd_set_error (bfd_error_invalid_operation);
+	  return FALSE;
+	}
+      
+      if ((offset + count) > hdr->sh_size)
+	{
+	  _bfd_error_handler
+	    (_("%pB:%pA: error: attempting to write over the end of the section"),
+	     abfd, section);
+
+	  bfd_set_error (bfd_error_invalid_operation);
+	  return FALSE;
+	}
+
+      contents = hdr->contents;
+      if (contents == NULL)
+	{
+	  _bfd_error_handler
+	    (_("%pB:%pA: error: attempting to write section into an empty buffer"),
+	     abfd, section);
+
+	  bfd_set_error (bfd_error_invalid_operation);
+	  return FALSE;
+	}
+
       memcpy (contents + offset, location, count);
       return TRUE;
     }
+
   pos = hdr->sh_offset + offset;
   if (bfd_seek (abfd, pos, SEEK_SET) != 0
       || bfd_bwrite (location, count, abfd) != count)
diff --git a/binutils/doc/binutils.texi b/binutils/doc/binutils.texi
index 3099e3f545..de3f1babb2 100644
--- a/binutils/doc/binutils.texi
+++ b/binutils/doc/binutils.texi
@@ -1648,7 +1648,9 @@ recognized names are @samp{alloc}, @samp{contents}, @samp{load},
 @samp{contents} flag for a section which does not have contents, but it
 is not meaningful to clear the @samp{contents} flag of a section which
 does have contents--just remove the section instead.  Not all flags are
-meaningful for all object file formats.
+meaningful for all object file formats.  In particular the
+@samp{share} flag is only meaningful for COFF format files and not for
+ELF format files.
 
 @item --set-section-alignment @var{sectionpattern}=@var{align}
 Set the alignment for any sections matching @var{sectionpattern}.
@@ -1704,7 +1706,8 @@ Rename a section from @var{oldname} to @var{newname}, optionally
 changing the section's flags to @var{flags} in the process.  This has
 the advantage over using a linker script to perform the rename in that
 the output stays as an object file and does not become a linked
-executable.
+executable.  This option accepts the same set of flags as the
+@option{--sect-section-flags} option.
 
 This option is particularly helpful when the input format is binary,
 since this will always create a section called .data.  If for example,
diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index 16affa9960..09facf0061 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -2562,6 +2562,23 @@ merge_gnu_build_notes (bfd *          abfd,
   return size;
 }
 
+static flagword
+check_new_section_flags (flagword flags, bfd * abfd, const char * secname)
+{
+  /* Only set the SEC_COFF_SHARED flag on COFF files.
+     The same bit value is used by ELF targets to indicate
+     compressed sections, and setting that flag here breaks
+     things.  */
+  if ((flags & SEC_COFF_SHARED)
+      && bfd_get_flavour (abfd) != bfd_target_coff_flavour)
+    {
+      non_fatal (_("%s[%s]: Note - dropping 'share' flag as output format is not COFF"),
+		 bfd_get_filename (abfd), secname);
+      flags &= ~ SEC_COFF_SHARED;
+    }
+  return flags;
+}
+
 /* Copy object file IBFD onto OBFD.
    Returns TRUE upon success, FALSE otherwise.  */
 
@@ -2810,7 +2827,10 @@ copy_object (bfd *ibfd, bfd *obfd, const bfd_arch_info_type *input_arch)
 	  pset = find_section_list (padd->name, FALSE,
 				    SECTION_CONTEXT_SET_FLAGS);
 	  if (pset != NULL)
-	    flags = pset->flags | SEC_HAS_CONTENTS;
+	    {	      
+	      flags = pset->flags | SEC_HAS_CONTENTS;
+	      flags = check_new_section_flags (flags, obfd, padd->name);
+	    }
 	  else
 	    flags = SEC_HAS_CONTENTS | SEC_READONLY | SEC_DATA;
 
@@ -3950,6 +3970,7 @@ setup_section (bfd *ibfd, sec_ptr isection, void *obfdarg)
   flagword flags;
   const char *err;
   const char * name;
+  const char * new_name;
   char *prefix = NULL;
   bfd_boolean make_nobits;
   unsigned int alignment;
@@ -3965,7 +3986,12 @@ setup_section (bfd *ibfd, sec_ptr isection, void *obfdarg)
       flags &= bfd_applicable_section_flags (ibfd);
       flags &= bfd_applicable_section_flags (obfd);
     }
-  name = find_section_rename (name, &flags);
+  new_name = find_section_rename (name, &flags);
+  if (new_name != name)
+    {
+      name = new_name;
+      flags = check_new_section_flags (flags, obfd, name);
+    }
 
   /* Prefix sections.  */
   if (prefix_alloc_sections_string
@@ -3989,7 +4015,10 @@ setup_section (bfd *ibfd, sec_ptr isection, void *obfdarg)
   p = find_section_list (bfd_section_name (isection), FALSE,
 			 SECTION_CONTEXT_SET_FLAGS);
   if (p != NULL)
-    flags = p->flags | (flags & (SEC_HAS_CONTENTS | SEC_RELOC));
+    {
+      flags = p->flags | (flags & (SEC_HAS_CONTENTS | SEC_RELOC));
+      flags = check_new_section_flags (flags, obfd, bfd_section_name (isection));
+    }
   else if (strip_symbols == STRIP_NONDEBUG
 	   && (flags & (SEC_ALLOC | SEC_GROUP)) != 0
 	   && !is_nondebug_keep_contents_section (ibfd, isection))