[Bug binutils/23919] New: bfd doesn't handle ELF compressed data alignment

[mark at klomp dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=23919

Bug ID: 23919
Summary: bfd doesn't handle ELF compressed data alignment
Product: binutils
Version: unspecified
Status: NEW
Severity: normal
Priority: P2
Component: binutils
Assignee: unassigned at sourceware dot org
Reporter: mark at klomp dot org
CC: devurandom at gmx dot net, elfutils-devel at sourceware dot org,
mark at klomp dot org, slyfox at inbox dot ru,
unassigned at sourceware dot org
Depends on: 23916
Target Milestone: ---

+++ 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.


Referenced Bugs:

https://sourceware.org/bugzilla/show_bug.cgi?id=23916
[Bug 23916] [bisected] elifutils-0.175 broke kernel's objtool (elifutils-0.173
works)

[mark at klomp dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=23919

--- Comment #1 from Mark Wielaard <mark at klomp dot org> ---
Created attachment 11413
--> https://sourceware.org/bugzilla/attachment.cgi?id=11413&action=edit
Proposed patch to handle compressed section alignment correctly

The attached git format-patch resolved this issue by handling the
(de)compressed section alignment correctly.

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=23919

--- Comment #2 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Nick Clifton <***@sourceware.org>:

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

commit 4207142d6a5d2359170c5f9a140fc1a2351fbda9
Author: Mark Wielaard <***@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.

[nickc at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=23919

Nick Clifton <nickc at redhat dot com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
CC| |nickc at redhat dot com
Resolution|--- |FIXED

--- Comment #3 from Nick Clifton <nickc at redhat dot com> ---
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

[mark at klomp dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=23919

--- Comment #4 from Mark Wielaard <mark at klomp dot org> ---
(In reply to Nick Clifton from comment #3)

Post by nickc at redhat dot com
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!

Post by nickc at redhat dot com
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/