Bug 23919 - bfd doesn't handle ELF compressed data alignment
Summary: bfd doesn't handle ELF compressed data alignment
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: binutils (show other bugs)
Version: 2.32
: P2 normal
Target Milestone: 2.32
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on: 23916
Blocks:
  Show dependency treegraph
 
Reported: 2018-11-24 22:09 UTC by Mark Wielaard
Modified: 2019-02-26 20:14 UTC (History)
8 users (show)

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


Attachments
Proposed patch to handle compressed section alignment correctly (3.47 KB, patch)
2018-11-24 22:27 UTC, Mark Wielaard
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Wielaard 2018-11-24 22:09:41 UTC
+++ This bug was initially created as a clone of Bug #23916 +++

The ELF compression header has a field (ch_addralign) that is set to the alignment of the uncompressed section. This way the section itself can have a different alignment than the decompressed section. bfd (and readelf) however explicitly make the section sh_addralign and ch_addralign equal. It also always sets the alignment to 1 which is wrong when using a Elf32_Chdr (which has alignment of 4) or Elf64_Chdr (which has alignment of 8).

This shows up with tools that use elfutils libelf which sets up the alignment correctly.

First gas creates a compressed section with on alignment of 1.
Second libelf accepts this, but corrects the alignment when it
writes out the section.
Third bfd_check_compression_header sanity checks the section alignment,
but it checks that the compressed and decompressed alignment is equal?!?
I think it wanted to check that the alignment is a power of 2 instead.
Comment 1 Mark Wielaard 2018-11-24 22:27:39 UTC
Created attachment 11413 [details]
Proposed patch to handle compressed section alignment correctly

The attached git format-patch resolved this issue by handling the (de)compressed section alignment correctly.
Comment 2 cvs-commit@gcc.gnu.org 2018-11-27 12:02:53 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

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

commit 4207142d6a5d2359170c5f9a140fc1a2351fbda9
Author: Mark Wielaard <mark@klomp.org>
Date:   Tue Nov 27 11:59:10 2018 +0000

    Handle ELF compressed header alignment correctly by setting up the section alignment correctly for the Elf32_Chdr or Elf64_Chdr type and respect the ch_addralign field when decompressing the section data.
    
    	PR binutils/23919
    binutils* readelf.c (dump_sections_as_strings): Remove bogus addralign check.
    	(dump_sections_as_bytes): Likewise.
    	(load_specific_debug_sections): Likewise.
    	* testsuite/binutils-all/dw2-3.rS: Adjust alignment.
    	* testsuite/binutils-all/dw2-3.rt: Likewise.
    
    bfd	* bfd.c (bfd_update_compression_header): Explicitly set alignment.
    	(bfd_check_compression_header): Add uncompressed_alignment_power
    	argument. Check ch_addralign is a power of 2.
    	* bfd-in2.h: Regenerated.
    	* compress.c (bfd_compress_section_contents): Get and set
    	orig_uncompressed_alignment_pow if section is decompressed.
    	(bfd_is_section_compressed_with_header): Add and get
    	uncompressed_align_pow_p argument.
    	(bfd_is_section_compressed): Add uncompressed_align_power argument
    	to bfd_is_section_compressed_with_header call.
    	(bfd_init_section_decompress_status): Get and set
    	uncompressed_alignment_power.
    	* elf.c (_bfd_elf_make_section_from_shdr): Add
    	uncompressed_align_power argument to
    	bfd_is_section_compressed_with_header call.
Comment 3 Nick Clifton 2018-11-27 12:06:11 UTC
Hi Mark,

  Thanks very much for the bug report, and especially for a patch to fix it!
  I have applied the patch, so I hope that this problem is now resolved.

  One very minor point - in the future, would you mind providing the
  ChangeLog entries as plain text, rather than a context diff ?  It
  did not matter this time, but often the diff will not apply because
  the changelog has changed by the time that the patch is applied.

Cheers
  Nick
Comment 4 Mark Wielaard 2018-11-27 12:22:40 UTC
(In reply to Nick Clifton from comment #3)
>   Thanks very much for the bug report, and especially for a patch to fix it!
>   I have applied the patch, so I hope that this problem is now resolved.

Thanks!
 
>   One very minor point - in the future, would you mind providing the
>   ChangeLog entries as plain text, rather than a context diff ?  It
>   did not matter this time, but often the diff will not apply because
>   the changelog has changed by the time that the patch is applied.

OK, I will in the future.
Do you have some script that helps you handle separate ChangeLog entries?

Note that gnulib contains some helpers for merging ChangeLog entries that I have used somewhat successfully with mercurial and git. They work the other way around though, you explicitly do add the ChangeLog entries in the commit/ChangeLog file, but when a merge action takes place they handle the entry specially.

https://gnu.wildebeest.org/blog/mjw/2012/03/16/automagically-merging-changelog-files-with-mercurial-or-git/
Comment 5 H.J. Lu 2018-11-27 13:47:19 UTC
On Linux/x86-64, I got

/export/build/gnu/binutils/build-x86_64-linux/binutils/objcopy -O elf32-x86-64 --compress-debug-sections=zlib-gabi tmpdir/dw2-3.o tmpdir/debug_str.copy.o
Executing on host: /export/build/gnu/binutils/build-x86_64-linux/binutils/objcopy -O elf32-x86-64 --compress-debug-sections=zlib-gabi tmpdir/dw2-3.o tmpdir/debug_str.copy.o   (timeout = 300)
spawn -ignore SIGHUP /export/build/gnu/binutils/build-x86_64-linux/binutils/objcopy -O elf32-x86-64 --compress-debug-sections=zlib-gabi tmpdir/dw2-3.o tmpdir/debug_str.copy.o^M
/export/build/gnu/binutils/build-x86_64-linux/binutils/objcopy: tmpdir/debug_str.copy.o: error: alignment power 32767 of section `.zdebug_line' is too big^M
/export/build/gnu/binutils/build-x86_64-linux/binutils/objcopy:tmpdir/debug_str.copy.o[.text]: file in wrong format^M
/export/build/gnu/binutils/build-x86_64-linux/binutils/objcopy: tmpdir/debug_str.copy.o: error: alignment power 32767 of section `.zdebug_line' is too big
/export/build/gnu/binutils/build-x86_64-linux/binutils/objcopy:tmpdir/debug_str.copy.o[.text]: file in wrong format

FAIL: objcopy (Convert x86-64 object with zlib-gnu to x32 (2))
Comment 6 cvs-commit@gcc.gnu.org 2018-11-27 14:06:03 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=131a5a648d314cd15811158150573cb40eb3abd0

commit 131a5a648d314cd15811158150573cb40eb3abd0
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Tue Nov 27 06:02:36 2018 -0800

    Initialize *uncompressed_align_pow_p to 0
    
    Initialize *uncompressed_align_pow_p to 0 since *uncompressed_align_pow_p
    is passed to bfd_is_section_compressed_with_header as uninitialized,
    
    	PR binutils/23919
    	* compress.c (bfd_is_section_compressed_with_header): Initialize
    	*uncompressed_align_pow_p to 0.
Comment 7 H.J. Lu 2018-11-27 14:07:35 UTC
Fixed for 2.32.
Comment 8 Dmitry V. Levin 2018-11-28 13:55:03 UTC
When we backported these fixes to our 2_31 based branch,
we got a surprising regression in gold/testsuite:

$ cat gold/testsuite/debug_msg.sh.log
Did not find expected error in debug_msg_so.err:
   debug_msg.so: error: undefined reference to 'undef_fn1()'

Actual error output below:
gcctestdir/ld: internal error in read_header_prolog, at dwarf_reader.cc:1678
collect2: error: ld returned 1 exit status
FAIL debug_msg.sh (exit status: 1)
Comment 9 H.J. Lu 2018-11-28 14:42:46 UTC
This change triggered

gcctestdir/collect-ld: warning: gdb_index_test_cdebug_gabi.o: section .debug_str contains incorrectly aligned strings; the alignment of those strings won't be preserved
gcctestdir/collect-ld: warning: gdb_index_test_3.o: section .debug_str contains incorrectly aligned strings; the alignment of those strings won't be preserved
gcctestdir/collect-ld: error: treating warnings as errors
collect2: error: ld returned 1 exit status 
make[3]: *** [gdb_index_test_3] Error 1 

in gold tests.
Comment 10 Mark Wielaard 2018-11-28 14:57:17 UTC
(In reply to cvs-commit@gcc.gnu.org from comment #6)
> The master branch has been updated by H.J. Lu <hjl@sourceware.org>:
> 
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;
> h=131a5a648d314cd15811158150573cb40eb3abd0
> 
> commit 131a5a648d314cd15811158150573cb40eb3abd0
> Author: H.J. Lu <hjl.tools@gmail.com>
> Date:   Tue Nov 27 06:02:36 2018 -0800
> 
>     Initialize *uncompressed_align_pow_p to 0
>     
>     Initialize *uncompressed_align_pow_p to 0 since *uncompressed_align_pow_p
>     is passed to bfd_is_section_compressed_with_header as uninitialized,
>     
>     	PR binutils/23919
>     	* compress.c (bfd_is_section_compressed_with_header): Initialize
>     	*uncompressed_align_pow_p to 0.

I think this is correct, the uncompressed_align_pow_p wasn't set explicitly when the compressed section was gnu-zlib, in which case the alignment has to be assumed to be always 1.
Comment 11 Mark Wielaard 2018-11-28 15:02:57 UTC
(In reply to H.J. Lu from comment #9)
> This change triggered
> 
> gcctestdir/collect-ld: warning: gdb_index_test_cdebug_gabi.o: section
> .debug_str contains incorrectly aligned strings; the alignment of those
> strings won't be preserved
> gcctestdir/collect-ld: warning: gdb_index_test_3.o: section .debug_str
> contains incorrectly aligned strings; the alignment of those strings won't
> be preserved
> gcctestdir/collect-ld: error: treating warnings as errors
> collect2: error: ld returned 1 exit status 
> make[3]: *** [gdb_index_test_3] Error 1 
> 
> in gold tests.

I think this comes from do_add_input_section in gold/merge.cc.
It decompresses the section, but doesn't use ch_addralign, but this->addralign(), when calculating the alignment constraint.
Comment 12 cvs-commit@gcc.gnu.org 2018-12-02 13:46:34 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=5f6c22aee74f17393b82934a5682d985672e011a

commit 5f6c22aee74f17393b82934a5682d985672e011a
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Sun Dec 2 05:42:36 2018 -0800

    gold: Get alignment of uncompressed section from ch_addralign
    
    The ELF compression header has a field (ch_addralign) that is set to
    the alignment of the uncompressed section. This way the section itself
    can have a different alignment than the decompressed section.  Update
    decompress_input_section to get alignment of the decompressed section
    and use it when merging decompressed strings.
    
    	PR binutils/23919
    	* merge.cc (Output_merge_string<Char_type>::do_add_input_section):
    	Get addralign from decompressed_section_contents.
    	* object.cc (build_compressed_section_map): Set info.addralign.
    	(Object::decompressed_section_contents): Add a palign
    	argument and store p->second.addralign in *palign if it isn't
    	NULL.
    	* object.h (Compressed_section_info): Add addralign.
    	(section_is_compressed): Add a palign argument, default it
    	to NULL, store p->second.addralign in *palign if it isn't NULL.
    	(Object::decompressed_section_contents): Likewise.
    	* output.cc (Output_section::add_input_section): Get addralign
    	from section_is_compressed.
Comment 13 H.J. Lu 2018-12-02 13:52:21 UTC
Fixed for 2.32.
Comment 14 cvs-commit@gcc.gnu.org 2019-02-18 15:10:50 UTC
The binutils-2_31-branch branch has been updated by Nick Clifton <nickc@sourceware.org>:

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

commit 88739f776b733b0b84600b283417f862a010bb5d
Author: Nick Clifton <nickc@redhat.com>
Date:   Mon Feb 18 15:08:57 2019 +0000

    Import patch to fix PR23919 from the mainline.
    
    	PR binutils/23919
    bfd	* bfd.c (bfd_update_compression_header): Explicitly set alignment.
    	(bfd_check_compression_header): Add uncompressed_alignment_power
    	argument. Check ch_addralign is a power of 2.
    	* bfd-in2.h: Regenerated.
    	* compress.c (bfd_compress_section_contents): Get and set
    	orig_uncompressed_alignment_pow if section is decompressed.
    	(bfd_is_section_compressed_with_header): Add and get
    	uncompressed_align_pow_p argument.
    	(bfd_is_section_compressed): Add uncompressed_align_power argument
    	to bfd_is_section_compressed_with_header call.
    	(bfd_init_section_decompress_status): Get and set
    	uncompressed_alignment_power.
    	* elf.c (_bfd_elf_make_section_from_shdr): Add
    	uncompressed_align_power argument to
    	bfd_is_section_compressed_with_header call.
    
    	* compress.c (bfd_is_section_compressed_with_header): Initialize
    	* uncompressed_align_pow_p to 0.
    
    binutils* readelf.c (dump_sections_as_strings): Remove bogus addralign check.
    	(dump_sections_as_bytes): Likewise.
    	(load_specific_debug_sections): Likewise.
    	* testsuite/binutils-all/dw2-3.rS: Adjust alignment.
    	* testsuite/binutils-all/dw2-3.rt: Likewise.
    
    gold	* merge.cc (Output_merge_string<Char_type>::do_add_input_section):
    	Get addralign from decompressed_section_contents.
    	* object.cc (build_compressed_section_map): Set info.addralign.
    	(Object::decompressed_section_contents): Add a palign
    	argument and store p->second.addralign in *palign if it isn't
    	NULL.
    	* object.h (Compressed_section_info): Add addralign.
    	(section_is_compressed): Add a palign argument, default it
    	to NULL, store p->second.addralign in *palign if it isn't NULL.
    	(Object::decompressed_section_contents): Likewise.
    	* output.cc (Output_section::add_input_section): Get addralign
    	from section_is_compressed.
Comment 15 cvs-commit@gcc.gnu.org 2019-02-26 20:12:54 UTC
The upstream/gdb-8.2-branch branch has been updated by Pedro Alves <palves@sourceware.org>:

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

commit 6d33d996167a33552b68c036f1b1571a208ace42
Author: Nick Clifton <nickc@redhat.com>
Date:   Tue Feb 26 19:57:46 2019 +0000

    Import patch to fix PR23919 from the mainline.
    
    	PR binutils/23919
    bfd	* bfd.c (bfd_update_compression_header): Explicitly set alignment.
    	(bfd_check_compression_header): Add uncompressed_alignment_power
    	argument. Check ch_addralign is a power of 2.
    	* bfd-in2.h: Regenerated.
    	* compress.c (bfd_compress_section_contents): Get and set
    	orig_uncompressed_alignment_pow if section is decompressed.
    	(bfd_is_section_compressed_with_header): Add and get
    	uncompressed_align_pow_p argument.
    	(bfd_is_section_compressed): Add uncompressed_align_power argument
    	to bfd_is_section_compressed_with_header call.
    	(bfd_init_section_decompress_status): Get and set
    	uncompressed_alignment_power.
    	* elf.c (_bfd_elf_make_section_from_shdr): Add
    	uncompressed_align_power argument to
    	bfd_is_section_compressed_with_header call.
    
    	* compress.c (bfd_is_section_compressed_with_header): Initialize
    	* uncompressed_align_pow_p to 0.
    
    binutils* readelf.c (dump_sections_as_strings): Remove bogus addralign check.
    	(dump_sections_as_bytes): Likewise.
    	(load_specific_debug_sections): Likewise.
    	* testsuite/binutils-all/dw2-3.rS: Adjust alignment.
    	* testsuite/binutils-all/dw2-3.rt: Likewise.
    
    gold	* merge.cc (Output_merge_string<Char_type>::do_add_input_section):
    	Get addralign from decompressed_section_contents.
    	* object.cc (build_compressed_section_map): Set info.addralign.
    	(Object::decompressed_section_contents): Add a palign
    	argument and store p->second.addralign in *palign if it isn't
    	NULL.
    	* object.h (Compressed_section_info): Add addralign.
    	(section_is_compressed): Add a palign argument, default it
    	to NULL, store p->second.addralign in *palign if it isn't NULL.
    	(Object::decompressed_section_contents): Likewise.
    	* output.cc (Output_section::add_input_section): Get addralign
    	from section_is_compressed.
Comment 16 cvs-commit@gcc.gnu.org 2019-02-26 20:14:30 UTC
The gdb-8.2-branch branch has been updated by Pedro Alves <palves@sourceware.org>:

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

commit 6d33d996167a33552b68c036f1b1571a208ace42
Author: Nick Clifton <nickc@redhat.com>
Date:   Tue Feb 26 19:57:46 2019 +0000

    Import patch to fix PR23919 from the mainline.
    
    	PR binutils/23919
    bfd	* bfd.c (bfd_update_compression_header): Explicitly set alignment.
    	(bfd_check_compression_header): Add uncompressed_alignment_power
    	argument. Check ch_addralign is a power of 2.
    	* bfd-in2.h: Regenerated.
    	* compress.c (bfd_compress_section_contents): Get and set
    	orig_uncompressed_alignment_pow if section is decompressed.
    	(bfd_is_section_compressed_with_header): Add and get
    	uncompressed_align_pow_p argument.
    	(bfd_is_section_compressed): Add uncompressed_align_power argument
    	to bfd_is_section_compressed_with_header call.
    	(bfd_init_section_decompress_status): Get and set
    	uncompressed_alignment_power.
    	* elf.c (_bfd_elf_make_section_from_shdr): Add
    	uncompressed_align_power argument to
    	bfd_is_section_compressed_with_header call.
    
    	* compress.c (bfd_is_section_compressed_with_header): Initialize
    	* uncompressed_align_pow_p to 0.
    
    binutils* readelf.c (dump_sections_as_strings): Remove bogus addralign check.
    	(dump_sections_as_bytes): Likewise.
    	(load_specific_debug_sections): Likewise.
    	* testsuite/binutils-all/dw2-3.rS: Adjust alignment.
    	* testsuite/binutils-all/dw2-3.rt: Likewise.
    
    gold	* merge.cc (Output_merge_string<Char_type>::do_add_input_section):
    	Get addralign from decompressed_section_contents.
    	* object.cc (build_compressed_section_map): Set info.addralign.
    	(Object::decompressed_section_contents): Add a palign
    	argument and store p->second.addralign in *palign if it isn't
    	NULL.
    	* object.h (Compressed_section_info): Add addralign.
    	(section_is_compressed): Add a palign argument, default it
    	to NULL, store p->second.addralign in *palign if it isn't NULL.
    	(Object::decompressed_section_contents): Likewise.
    	* output.cc (Output_section::add_input_section): Get addralign
    	from section_is_compressed.