[PATCH] objcopy: check input flavor before setting PE/COFF section alignment

[PATCH] objcopy: check input flavor before setting PE/COFF section alignment

[Jan Beulich]

coff_section_data() and elf_section_data() use the same underlying
field. The pointer being non-NULL therefore isn't sufficient to know
that pei_section_data() can validly be used on the incoming object.
Apparently in 64-bit-host builds the resulting memory corruption is
benign, whereas in 32-bit-host builds a segmentation fault occurs upon
de-referencing pei_section_data()'s return value.
---
Of course the value (first) being set on the input bfd is suspicious
in the first place: When copying e.g. ELF to PE/COFF, the option ought
to be similarly respected, yet clearly it can't be set like this then on
the incoming object. The change here is merely to address the testsuite
failures observed for Arm64 and RISC-V ("Check if efi app format is
recognized") as well as the (latent) memory corruption.

--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -4310,6 +4310,7 @@ setup_section (bfd *ibfd, sec_ptr isecti
   if (p != NULL)
     alignment = p->alignment;
   else if (pe_section_alignment != (bfd_vma) -1
+	   && bfd_get_flavour (ibfd) == bfd_target_coff_flavour
 	   && bfd_get_flavour (obfd) == bfd_target_coff_flavour)
     {
       alignment = power_of_two (pe_section_alignment);

Re: [PATCH] objcopy: check input flavor before setting PE/COFF section alignment

[Nick Clifton]

Hi Jan,

> coff_section_data() and elf_section_data() use the same underlying
> field. The pointer being non-NULL therefore isn't sufficient to know
> that pei_section_data() can validly be used on the incoming object.
> Apparently in 64-bit-host builds the resulting memory corruption is
> benign, whereas in 32-bit-host builds a segmentation fault occurs upon
> de-referencing pei_section_data()'s return value.
> ---
> Of course the value (first) being set on the input bfd is suspicious
> in the first place: When copying e.g. ELF to PE/COFF, the option ought
> to be similarly respected, yet clearly it can't be set like this then on
> the incoming object. The change here is merely to address the testsuite
> failures observed for Arm64 and RISC-V ("Check if efi app format is
> recognized") as well as the (latent) memory corruption.

Thanks for fixing my oversight!

Cheers
   Nick

Re: [PATCH] objcopy: check input flavor before setting PE/COFF section alignment

[Jan Beulich]

On 19.04.2024 12:37, Nick Clifton wrote:
>> coff_section_data() and elf_section_data() use the same underlying
>> field. The pointer being non-NULL therefore isn't sufficient to know
>> that pei_section_data() can validly be used on the incoming object.
>> Apparently in 64-bit-host builds the resulting memory corruption is
>> benign, whereas in 32-bit-host builds a segmentation fault occurs upon
>> de-referencing pei_section_data()'s return value.
>> ---
>> Of course the value (first) being set on the input bfd is suspicious
>> in the first place: When copying e.g. ELF to PE/COFF, the option ought
>> to be similarly respected, yet clearly it can't be set like this then on
>> the incoming object. The change here is merely to address the testsuite
>> failures observed for Arm64 and RISC-V ("Check if efi app format is
>> recognized") as well as the (latent) memory corruption.
> 
> Thanks for fixing my oversight!

Well, before putting it in - any thoughts on the post-commit-message remark
above? Is it really meant to stay the way of the input bfd's data is being
altered, rather than keeping that intact and fiddling only with the output?
And thus - afaict - rendering the command line option (silently) useless
when copying ELF to PE?

Jan

Re: [PATCH] objcopy: check input flavor before setting PE/COFF section alignment

[Nick Clifton]

Hi Jan,

>>> Of course the value (first) being set on the input bfd is suspicious
>>> in the first place: When copying e.g. ELF to PE/COFF, the option ought
>>> to be similarly respected, yet clearly it can't be set like this then on
>>> the incoming object. The change here is merely to address the testsuite
>>> failures observed for Arm64 and RISC-V ("Check if efi app format is
>>> recognized") as well as the (latent) memory corruption.

> Well, before putting it in - any thoughts on the post-commit-message remark
> above?

Sorry.  Well in the first place converting from ELF to PE is always going to
be a difficult process.  So trying to combine it with adjustments to other
properties, such as alignment, is just asking for trouble.  My feeling therefore
is that we ought to be have a warning in the documentation telling users
to be careful and maybe take things one step at a time.

> Is it really meant to stay the way of the input bfd's data is being
> altered, rather than keeping that intact and fiddling only with the output?
> And thus - afaict - rendering the command line option (silently) useless
> when copying ELF to PE?

You are right.  For this particular case maybe we should add a test for
ELF->PE format changing and refuse to adjust the alignments.

Cheers
   Nick