[2/3] bfd/PE: fix bigobj file corruption on objcopy

Message ID 20260928090146.4016188-3-tobias.deiminger@linutronix.de
State New
Headers
Series PE compression/corruption fixes with related test case |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed

Commit Message

Tobias Deiminger Sept. 28, 2026, 9:01 a.m. UTC
  Converting any linked executable to pe-bigobj-* silently produced
corrupted section headers. For example:

 $ gcc -g -o testprog testprog.c
 $ objcopy -I elf64-x86-64 -O pe-bigobj-x86-64 testprog testprog.bigobj
 $ objdump -b pe-bigobj-x86-64 -h testprog.bigobj | head
 [...]
 Idx Name          Size      VMA               LMA               File off  Algn
   0 ^K^B^B/       00001050  0000000000000000  0000000000000000  00001050  2**2

bigobj files can't have an OptionalHeader. coff_bigobj_swap_filehdr_in
therefore correctly hardcodes f_opthdr = 0 when reading bigobj into
memory.

However, bigobj flags wrongly allowed for EXEC_P which requests an
OptionalHeader to store a start address in multiple code paths. Further,
bigobj's _bfd_aoutsz was initialized > 0 and therefore miscalculated the
section header offset on write as if there was an OptionalHeader,
causing an invalid gap. If a correct reader reads the produced image, it
tries to read section headers but actually accesses undefined content
from the gap.

Fix it by ensuring bigobj formats have _bfd_aoutsz = 0 and EXEC_P unset,
notably also for special cases like auto-promotion. This causes minor
information loss (flag, start address) on some conversions, but it's
inevitable if the format just can't represent it and still better than
producing invalid output.

Signed-off-by: Tobias Deiminger <tobias.deiminger@linutronix.de>
---
Here are two more ways to trigger the bug.

File corruption during ld

 $ printf '.globl _entry\n.text\n_entry: ret\n' > entry.s
 $ x86_64-w64-mingw32-as -o entry.o entry.s
 $ ld -m i386pep --oformat pe-bigobj-x86-64 -e _entry -o linked.o entry.o
 $ objdump -h linked.o | head
 [...]
 Idx Name          Size      VMA               LMA               File off  Algn
   0 ^K^B^B/       00001000  0000000000000000  0000000000000000  00001000  2**2

File corruption during auto-promotion (more than 65279 sections)

 $ printf '.section .t%d,"dr"\n.byte 1\n' $(seq 0 65999) > many.s
 $ x86_64-w64-mingw32-as -mbig-obj -o many.o many.s
 $ objcopy -I pe-bigobj-x86-64 -O pe-x86-64 --set-start 0x1000 many.o bigmany.o
 $ objdump -h bigmany.o | head
 [...]
 Idx Name          Size      VMA               LMA               File off  Algn
   0 ^K^B^B/       00001000  0000000000000000  0000000000000000  00000000  2**2

 bfd/coff-aarch64.c |  6 +++---
 bfd/coff-i386.c    |  6 +++---
 bfd/coff-x86_64.c  |  6 +++---
 bfd/coffcode.h     | 23 ++++++++++++++++++-----
 4 files changed, 27 insertions(+), 14 deletions(-)
  

Comments

Jan Beulich Oct. 2, 2026, 12:46 p.m. UTC | #1
On 28.09.2026 11:01, Tobias Deiminger wrote:
> Converting any linked executable to pe-bigobj-* silently produced
> corrupted section headers. For example:
> 
>  $ gcc -g -o testprog testprog.c
>  $ objcopy -I elf64-x86-64 -O pe-bigobj-x86-64 testprog testprog.bigobj
>  $ objdump -b pe-bigobj-x86-64 -h testprog.bigobj | head
>  [...]
>  Idx Name          Size      VMA               LMA               File off  Algn
>    0 ^K^B^B/       00001050  0000000000000000  0000000000000000  00001050  2**2
> 
> bigobj files can't have an OptionalHeader. coff_bigobj_swap_filehdr_in
> therefore correctly hardcodes f_opthdr = 0 when reading bigobj into
> memory.
> 
> However, bigobj flags wrongly allowed for EXEC_P which requests an
> OptionalHeader to store a start address in multiple code paths. Further,
> bigobj's _bfd_aoutsz was initialized > 0 and therefore miscalculated the
> section header offset on write as if there was an OptionalHeader,
> causing an invalid gap. If a correct reader reads the produced image, it
> tries to read section headers but actually accesses undefined content
> from the gap.
> 
> Fix it by ensuring bigobj formats have _bfd_aoutsz = 0 and EXEC_P unset,
> notably also for special cases like auto-promotion. This causes minor
> information loss (flag, start address) on some conversions, but it's
> inevitable if the format just can't represent it and still better than
> producing invalid output.

While things read plausibly (assuming one is willing to accept that
converting a linked binary back into an object file is something that
wants permitting), I'm struggling with the solution. There shouldn't be
a difference between settings (EXEC_P, AOUTSZ) for bigobj vs "normal".
That difference is rather one between (COFF) objects and linked (PE)
binaries, isn't it?

That is, shouldn't e.g. pe-<arch>.c and pei-<arch>.c control
(differently) what coff-<arch>.c (aided by coffcode.h) puts in place?
(Really this may not need any change to pe{,i}-<arch>.c, but may be
possible to control via use of COFF_IMAGE_WITH_PE directly in
coff-<arch>.c / coffcode.h. And then uniformly for bigobj and "normal"
objects. Likely the !COFF_WITH_PE case also would need leaving alone
altogether, just to avoid breaking anything there.)

Jan
  
Tobias Deiminger Oct. 6, 2026, 7:44 a.m. UTC | #2
Am Freitag, 2. Oktober 2026, 14:46:43 Mitteleuropäische Sommerzeit schrieb Jan Beulich:
> On 28.09.2026 11:01, Tobias Deiminger wrote:
> > Converting any linked executable to pe-bigobj-* silently produced
> > corrupted section headers. For example:
> > 
> >  $ gcc -g -o testprog testprog.c
> >  $ objcopy -I elf64-x86-64 -O pe-bigobj-x86-64 testprog testprog.bigobj
> >  $ objdump -b pe-bigobj-x86-64 -h testprog.bigobj | head
> >  [...]
> >  Idx Name          Size      VMA               LMA               File off  Algn
> >    0 ^K^B^B/       00001050  0000000000000000  0000000000000000  00001050  2**2
> > 
> > bigobj files can't have an OptionalHeader. coff_bigobj_swap_filehdr_in
> > therefore correctly hardcodes f_opthdr = 0 when reading bigobj into
> > memory.
> > 
> > However, bigobj flags wrongly allowed for EXEC_P which requests an
> > OptionalHeader to store a start address in multiple code paths. Further,
> > bigobj's _bfd_aoutsz was initialized > 0 and therefore miscalculated the
> > section header offset on write as if there was an OptionalHeader,
> > causing an invalid gap. If a correct reader reads the produced image, it
> > tries to read section headers but actually accesses undefined content
> > from the gap.
> > 
> > Fix it by ensuring bigobj formats have _bfd_aoutsz = 0 and EXEC_P unset,
> > notably also for special cases like auto-promotion. This causes minor
> > information loss (flag, start address) on some conversions, but it's
> > inevitable if the format just can't represent it and still better than
> > producing invalid output.
> 
> While things read plausibly (assuming one is willing to accept that
> converting a linked binary back into an object file is something that
> wants permitting), I'm struggling with the solution. There shouldn't be
> a difference between settings (EXEC_P, AOUTSZ) for bigobj vs "normal".
> That difference is rather one between (COFF) objects and linked (PE)
> binaries, isn't it?

In my understanding it's like this:
- objects: *should not have* an optional header, but could
- bigobj: definitely *cannot have* an optional header
- EXEC_P only makes sense with an optional header, i.e. autosz > 0.

COFF File Header (Object and Image) says [1]

 SizeOfOptionalHeader ... This value should be zero for an object file.

The "should" leaves the odd possibility that an object could have a non-zero
optional header. objcopy can create such odd, yet valid objects today. Whether
it makes sense I don't know.

However, bigobj has a distinct file header

 // bigobj doesn't use COFF File Header, but a special one
 typedef struct ANON_OBJECT_HEADER_BIGOBJ {
    ...
    // no SizeOfOptionalHeader field here at all
    ...
 }

That means bigobj can definitely not have an optional header, because the
bigobj file header has no field to declare room for an optional header.

> That is, shouldn't e.g. pe-<arch>.c and pei-<arch>.c control
> (differently) what coff-<arch>.c (aided by coffcode.h) puts in place?
> (Really this may not need any change to pe{,i}-<arch>.c, but may be
> possible to control via use of COFF_IMAGE_WITH_PE directly in
> coff-<arch>.c / coffcode.h. And then uniformly for bigobj and "normal"
> objects. Likely the !COFF_WITH_PE case also would need leaving alone
> altogether, just to avoid breaking anything there.)

I wouldn't have been confident enough to change all PE formats in BFD so that
"should be zero" for us means "we don't support an optional header". Thus kept
the scope local to bigobj.

Do you still prefer a PE-wide change? If so I'll add it to v2.

Tobias

> Jan
>
  
Jan Beulich Oct. 6, 2026, 8:19 a.m. UTC | #3
On 06.10.2026 09:44, Tobias Deiminger wrote:
> Am Freitag, 2. Oktober 2026, 14:46:43 Mitteleuropäische Sommerzeit schrieb Jan Beulich:
>> On 28.09.2026 11:01, Tobias Deiminger wrote:
>>> Converting any linked executable to pe-bigobj-* silently produced
>>> corrupted section headers. For example:
>>>
>>>  $ gcc -g -o testprog testprog.c
>>>  $ objcopy -I elf64-x86-64 -O pe-bigobj-x86-64 testprog testprog.bigobj
>>>  $ objdump -b pe-bigobj-x86-64 -h testprog.bigobj | head
>>>  [...]
>>>  Idx Name          Size      VMA               LMA               File off  Algn
>>>    0 ^K^B^B/       00001050  0000000000000000  0000000000000000  00001050  2**2
>>>
>>> bigobj files can't have an OptionalHeader. coff_bigobj_swap_filehdr_in
>>> therefore correctly hardcodes f_opthdr = 0 when reading bigobj into
>>> memory.
>>>
>>> However, bigobj flags wrongly allowed for EXEC_P which requests an
>>> OptionalHeader to store a start address in multiple code paths. Further,
>>> bigobj's _bfd_aoutsz was initialized > 0 and therefore miscalculated the
>>> section header offset on write as if there was an OptionalHeader,
>>> causing an invalid gap. If a correct reader reads the produced image, it
>>> tries to read section headers but actually accesses undefined content
>>> from the gap.
>>>
>>> Fix it by ensuring bigobj formats have _bfd_aoutsz = 0 and EXEC_P unset,
>>> notably also for special cases like auto-promotion. This causes minor
>>> information loss (flag, start address) on some conversions, but it's
>>> inevitable if the format just can't represent it and still better than
>>> producing invalid output.
>>
>> While things read plausibly (assuming one is willing to accept that
>> converting a linked binary back into an object file is something that
>> wants permitting), I'm struggling with the solution. There shouldn't be
>> a difference between settings (EXEC_P, AOUTSZ) for bigobj vs "normal".
>> That difference is rather one between (COFF) objects and linked (PE)
>> binaries, isn't it?
> 
> In my understanding it's like this:
> - objects: *should not have* an optional header, but could
> - bigobj: definitely *cannot have* an optional header
> - EXEC_P only makes sense with an optional header, i.e. autosz > 0.
> 
> COFF File Header (Object and Image) says [1]
> 
>  SizeOfOptionalHeader ... This value should be zero for an object file.
> 
> The "should" leaves the odd possibility that an object could have a non-zero
> optional header. objcopy can create such odd, yet valid objects today. Whether
> it makes sense I don't know.

Imo such uses of "should" often are an attempt to be kind, i.e. to avoid
words like "must" or "has to". Hence I question whether object files with
the field non-zero actually are valid.

> However, bigobj has a distinct file header
> 
>  // bigobj doesn't use COFF File Header, but a special one
>  typedef struct ANON_OBJECT_HEADER_BIGOBJ {
>     ...
>     // no SizeOfOptionalHeader field here at all
>     ...
>  }
> 
> That means bigobj can definitely not have an optional header, because the
> bigobj file header has no field to declare room for an optional header.
> 
>> That is, shouldn't e.g. pe-<arch>.c and pei-<arch>.c control
>> (differently) what coff-<arch>.c (aided by coffcode.h) puts in place?
>> (Really this may not need any change to pe{,i}-<arch>.c, but may be
>> possible to control via use of COFF_IMAGE_WITH_PE directly in
>> coff-<arch>.c / coffcode.h. And then uniformly for bigobj and "normal"
>> objects. Likely the !COFF_WITH_PE case also would need leaving alone
>> altogether, just to avoid breaking anything there.)
> 
> I wouldn't have been confident enough to change all PE formats in BFD so that
> "should be zero" for us means "we don't support an optional header". Thus kept
> the scope local to bigobj.
> 
> Do you still prefer a PE-wide change? If so I'll add it to v2.

As per above - yes, I think we want PE-wide adjustments.

Jan
  
Tobias Deiminger Oct. 7, 2026, 10:11 a.m. UTC | #4
Am Dienstag, 6. Oktober 2026, 10:19:59 Mitteleuropäische Sommerzeit schrieb Jan Beulich:
> On 06.10.2026 09:44, Tobias Deiminger wrote:
> > Am Freitag, 2. Oktober 2026, 14:46:43 Mitteleuropäische Sommerzeit schrieb Jan Beulich:
> >> On 28.09.2026 11:01, Tobias Deiminger wrote:
> >>> ...
> > The "should" leaves the odd possibility that an object could have a non-zero
> > optional header. objcopy can create such odd, yet valid objects today. Whether
> > it makes sense I don't know.
> 
> Imo such uses of "should" often are an attempt to be kind, i.e. to avoid
> words like "must" or "has to". Hence I question whether object files with
> the field non-zero actually are valid.
> 
> > However, bigobj has a distinct file header
> > 
> >  // bigobj doesn't use COFF File Header, but a special one
> >  typedef struct ANON_OBJECT_HEADER_BIGOBJ {
> >     ...
> >     // no SizeOfOptionalHeader field here at all
> >     ...
> >  }
> > 
> > That means bigobj can definitely not have an optional header, because the
> > bigobj file header has no field to declare room for an optional header.
> > 
> >> That is, shouldn't e.g. pe-<arch>.c and pei-<arch>.c control
> >> (differently) what coff-<arch>.c (aided by coffcode.h) puts in place?
> >> (Really this may not need any change to pe{,i}-<arch>.c, but may be
> >> possible to control via use of COFF_IMAGE_WITH_PE directly in
> >> coff-<arch>.c / coffcode.h. And then uniformly for bigobj and "normal"
> >> objects. Likely the !COFF_WITH_PE case also would need leaving alone
> >> altogether, just to avoid breaking anything there.)
> > 
> > I wouldn't have been confident enough to change all PE formats in BFD so that
> > "should be zero" for us means "we don't support an optional header". Thus kept
> > the scope local to bigobj.
> > 
> > Do you still prefer a PE-wide change? If so I'll add it to v2.
> 
> As per above - yes, I think we want PE-wide adjustments.

Tried, works basically as you've sketched it.

I'd like to suggest a variant. How about controlling only EXEC_P generically
with COFF_WITH_PE/COFF_IMAGE_WITH_PE as you suggested, but keep AOUTSZ
format-specific?

This has the effect that existing odd PE objects with OptionalHeader remain
readable while bfd will no longer write any PE object with OptionalHeader,
avoiding the corruption.

Tobias
  
Jan Beulich Oct. 7, 2026, 10:53 a.m. UTC | #5
On 07.10.2026 12:11, Tobias Deiminger wrote:
> Am Dienstag, 6. Oktober 2026, 10:19:59 Mitteleuropäische Sommerzeit schrieb Jan Beulich:
>> On 06.10.2026 09:44, Tobias Deiminger wrote:
>>> Am Freitag, 2. Oktober 2026, 14:46:43 Mitteleuropäische Sommerzeit schrieb Jan Beulich:
>>>> On 28.09.2026 11:01, Tobias Deiminger wrote:
>>>>> ...
>>> The "should" leaves the odd possibility that an object could have a non-zero
>>> optional header. objcopy can create such odd, yet valid objects today. Whether
>>> it makes sense I don't know.
>>
>> Imo such uses of "should" often are an attempt to be kind, i.e. to avoid
>> words like "must" or "has to". Hence I question whether object files with
>> the field non-zero actually are valid.
>>
>>> However, bigobj has a distinct file header
>>>
>>>  // bigobj doesn't use COFF File Header, but a special one
>>>  typedef struct ANON_OBJECT_HEADER_BIGOBJ {
>>>     ...
>>>     // no SizeOfOptionalHeader field here at all
>>>     ...
>>>  }
>>>
>>> That means bigobj can definitely not have an optional header, because the
>>> bigobj file header has no field to declare room for an optional header.
>>>
>>>> That is, shouldn't e.g. pe-<arch>.c and pei-<arch>.c control
>>>> (differently) what coff-<arch>.c (aided by coffcode.h) puts in place?
>>>> (Really this may not need any change to pe{,i}-<arch>.c, but may be
>>>> possible to control via use of COFF_IMAGE_WITH_PE directly in
>>>> coff-<arch>.c / coffcode.h. And then uniformly for bigobj and "normal"
>>>> objects. Likely the !COFF_WITH_PE case also would need leaving alone
>>>> altogether, just to avoid breaking anything there.)
>>>
>>> I wouldn't have been confident enough to change all PE formats in BFD so that
>>> "should be zero" for us means "we don't support an optional header". Thus kept
>>> the scope local to bigobj.
>>>
>>> Do you still prefer a PE-wide change? If so I'll add it to v2.
>>
>> As per above - yes, I think we want PE-wide adjustments.
> 
> Tried, works basically as you've sketched it.
> 
> I'd like to suggest a variant. How about controlling only EXEC_P generically
> with COFF_WITH_PE/COFF_IMAGE_WITH_PE as you suggested, but keep AOUTSZ
> format-specific?
> 
> This has the effect that existing odd PE objects with OptionalHeader remain
> readable while bfd will no longer write any PE object with OptionalHeader,
> avoiding the corruption.

Why not, if suitably explained in the description.

Jan
  

Patch

diff --git a/bfd/coff-aarch64.c b/bfd/coff-aarch64.c
index d889359a6fc..50ba610ad46 100644
--- a/bfd/coff-aarch64.c
+++ b/bfd/coff-aarch64.c
@@ -1028,9 +1028,9 @@  const bfd_target
   BFD_ENDIAN_LITTLE,		/* Data byte order is little.  */
   BFD_ENDIAN_LITTLE,		/* Header byte order is little.  */
 
-  (HAS_RELOC | EXEC_P		/* Object flags.  */
-   | HAS_LINENO | HAS_DEBUG
-   | HAS_SYMS | HAS_LOCALS | WP_TEXT | D_PAGED | BFD_COMPRESS | BFD_DECOMPRESS),
+  (HAS_RELOC | HAS_LINENO		/* Object flags.  */
+   | HAS_DEBUG | HAS_SYMS
+   | HAS_LOCALS | WP_TEXT | D_PAGED | BFD_COMPRESS | BFD_DECOMPRESS),
 
   (SEC_HAS_CONTENTS | SEC_ALLOC | SEC_LOAD | SEC_RELOC /* Section flags.  */
 #ifdef COFF_WITH_PE
diff --git a/bfd/coff-i386.c b/bfd/coff-i386.c
index 0e296a3400b..3c4e24a5ca1 100644
--- a/bfd/coff-i386.c
+++ b/bfd/coff-i386.c
@@ -801,9 +801,9 @@  const bfd_target
   BFD_ENDIAN_LITTLE,		/* data byte order is little */
   BFD_ENDIAN_LITTLE,		/* header byte order is little */
 
-  (HAS_RELOC | EXEC_P |		/* object flags */
-   HAS_LINENO | HAS_DEBUG |
-   HAS_SYMS | HAS_LOCALS | WP_TEXT | D_PAGED | BFD_COMPRESS | BFD_DECOMPRESS ),
+  (HAS_RELOC | HAS_LINENO |	/* object flags */
+   HAS_DEBUG | HAS_SYMS |
+   HAS_LOCALS | WP_TEXT | D_PAGED | BFD_COMPRESS | BFD_DECOMPRESS ),
 
   (SEC_HAS_CONTENTS | SEC_ALLOC | SEC_LOAD | SEC_RELOC /* section flags */
 #ifdef COFF_WITH_PE
diff --git a/bfd/coff-x86_64.c b/bfd/coff-x86_64.c
index 2c434624052..d60ba6af04f 100644
--- a/bfd/coff-x86_64.c
+++ b/bfd/coff-x86_64.c
@@ -984,9 +984,9 @@  const bfd_target
   BFD_ENDIAN_LITTLE,		/* Data byte order is little.  */
   BFD_ENDIAN_LITTLE,		/* Header byte order is little.  */
 
-  (HAS_RELOC | EXEC_P		/* Object flags.  */
-   | HAS_LINENO | HAS_DEBUG
-   | HAS_SYMS | HAS_LOCALS | WP_TEXT | D_PAGED | BFD_COMPRESS | BFD_DECOMPRESS),
+  (HAS_RELOC | HAS_LINENO	/* Object flags.  */
+   | HAS_DEBUG | HAS_SYMS
+   | HAS_LOCALS | WP_TEXT | D_PAGED | BFD_COMPRESS | BFD_DECOMPRESS),
 
   (SEC_HAS_CONTENTS | SEC_ALLOC | SEC_LOAD | SEC_RELOC /* Section flags.  */
 #if defined(COFF_WITH_PE)
diff --git a/bfd/coffcode.h b/bfd/coffcode.h
index 784ed19f494..eb84f971833 100644
--- a/bfd/coffcode.h
+++ b/bfd/coffcode.h
@@ -3055,10 +3055,23 @@  coff_compute_section_file_positions (bfd * abfd)
     }
 #endif
 
-  if (bfd_get_start_address (abfd))
-    /*  A start address may have been added to the original file. In this
-	case it will need an optional header to record it.  */
-    abfd->flags |= EXEC_P;
+  if (bfd_applicable_file_flags (abfd) & EXEC_P)
+    {
+      /* A start address may have been added to the original file.  In this
+	 case it will need an optional header to record it.  */
+      if (bfd_get_start_address (abfd))
+	abfd->flags |= EXEC_P;
+    }
+  else if (abfd->flags & EXEC_P)
+    {
+      /* ld or auto-promotion may have requested the executable flag even if
+	 not supported by the target format.  */
+      _bfd_error_handler
+	/* xgettext:c-format */
+	(_("%pB: warning: %s cannot be executable or have a start address"),
+	 abfd, bfd_get_target (abfd));
+      abfd->flags &= ~EXEC_P;
+    }
 
   if (abfd->flags & EXEC_P)
     sofar += bfd_coff_aoutsz (abfd);
@@ -5940,7 +5953,7 @@  static const bfd_coff_backend_data bigobj_swap_table =
   coff_SWAP_lineno_out, coff_SWAP_reloc_out,
   coff_bigobj_swap_filehdr_out, coff_SWAP_aouthdr_out,
   coff_SWAP_scnhdr_out,
-  FILHSZ_BIGOBJ, AOUTSZ, SCNHSZ, SYMESZ_BIGOBJ, AUXESZ_BIGOBJ,
+  FILHSZ_BIGOBJ, 0, SCNHSZ, SYMESZ_BIGOBJ, AUXESZ_BIGOBJ,
    RELSZ, LINESZ, FILNMLEN_BIGOBJ,
   true,
   COFF_DEFAULT_LONG_SECTION_NAMES,