Bug 18087 - objcopy --compress-debug-sections can produce broken debug sections in PE binaries
Summary: objcopy --compress-debug-sections can produce broken debug sections in PE bin...
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: binutils (show other bugs)
Version: 2.26
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-06 12:01 UTC by Jon Turney
Modified: 2016-06-02 14:25 UTC (History)
4 users (show)

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


Attachments
Update section virtual size when it's compressed or decompressed (800 bytes, application/mbox)
2015-03-06 12:01 UTC, Jon Turney
Details
Don't compress section if it doesn't get smaller (1.33 KB, patch)
2015-03-07 22:08 UTC, Jon Turney
Details | Diff
Extended version of Jon's patch (1.43 KB, patch)
2015-03-17 17:43 UTC, Nick Clifton
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Turney 2015-03-06 12:01:27 UTC
Created attachment 8177 [details]
Update section virtual size when it's compressed or decompressed

PE binary support for objcopy --compress-debug-sections added in bug #14067 (commit a29a8af8) seems to have a problem when compression makes sections larger.

Examining the unstripped XWin.exe for which the problem was reported [1] (using my own PE dumper as objdump -h transparently decompresses the compressed sections for you.)

Section Name          Virtual Size   VMA        RawSize File Offset   Characteristics
   .text                  0017df74   00001000   0017e000   00000600   60500060
   .data                  00004184   0017f000   00004200   0017e600   c0700040
  .rdata                  000321e0   00184000   00032200   00182800   40700040
.buildid                  00000035   001b7000   00000200   001b4a00   40300040
      /4        .eh_frame 00049240   001b8000   00049400   001b4c00   40300040
    .bss                  0000eae0   00202000   00000000   00000000   c0700080
  .edata                  0002a0a5   00211000   0002a200   001fe000   40300040
  .idata                  00004c70   0023c000   00004e00   00228200   c0300040
   .rsrc                  00007800   00241000   00007400   0022d000   c0300040
  .reloc                  0000d1bc   00249000   0000d200   00234800   42300040
     /14   .debug_aranges 00002fb0   00257000   00003000   00241a00   42400040
     /29      .debug_info 007c423e   0025a000   007c4400   00244a00   42100040
     /41    .debug_abbrev 00056487   00a1f000   00056600   00a08e00   42100040
     /55      .debug_line 0009576a   00a76000   00095800   00a5f400   42100040
     /67     .debug_frame 00000038   00b0c000   00000200   00af4c00   42300040
     /80       .debug_str 00026a8b   00b0d000   00026c00   00af4e00   42100040
     /91       .debug_loc 001799f0   00b34000   00179a00   00b1ba00   42100040
    /102    .debug_ranges 00038b88   00cae000   00038c00   00c95400   42100040

Compare with this after it's compressed using objcopy -compressed-debug-sections

Section Name          Virtual Size   VMA        RawSize File Offset   Characteristics
   .text                  0017df74   00001000   0017e000   00000600   60500060
   .data                  00004184   0017f000   00004200   0017e600   c0700040
  .rdata                  000321e0   00184000   00032200   00182800   40700040
.buildid                  00000035   001b7000   00000200   001b4a00   40300040
      /4        .eh_frame 00049240   001b8000   00049400   001b4c00   40300040
    .bss                  0000eae0   00202000   00000000   00000000   c0700080
  .edata                  0002a0a5   00211000   0002a200   001fe000   40300040
  .idata                  00004c70   0023c000   00004e00   00228200   c0300040
   .rsrc                  00007800   00241000   00007400   0022d000   c0300040
  .reloc                  0000d1bc   00249000   0000d200   00234400   42300040
     /14  .zdebug_aranges 00002fb0   00257000   00001200   00241600   42400040
     /30     .zdebug_info 007c423e   0025a000   00356200   00242800   42100040
     /43   .zdebug_abbrev 00056487   00a1f000   0000d600   00598a00   42100040
     /58     .zdebug_line 0009576a   00a76000   00040600   005a6000   42100040
     /71    .zdebug_frame 00000038   00b0c000   00000200   005e6600   42300040
     /85      .zdebug_str 00026a8b   00b0d000   00005c00   005e6800   42100040
     /97      .zdebug_loc 001799f0   00b34000   0007de00   005ec400   42100040
    /109   .zdebug_ranges 00038b88   00cae000   00013000   0066a200   42100040

It can be seen that the virtual size of the compressed sections is not updated, although the raw size has decreased.

Normally this is not a problem, as nothing is accessing the section contents after the raw size.

However, if the compressor has made the data bigger, it is truncated to the virtual size and decompression fails.

Unfortunately, small .debug_frame sections seem to be quite normal on x86.

Attached is a patch which updates the virtual size, which seems to fix this issue.

Section Name          Virtual Size   VMA        RawSize File Offset   Characteristics
   .text                  0017df74   00001000   0017e000   00000600   60500060
   .data                  00004184   0017f000   00004200   0017e600   c0700040
  .rdata                  000321e0   00184000   00032200   00182800   40700040
.buildid                  00000035   001b7000   00000200   001b4a00   40300040
      /4        .eh_frame 00049240   001b8000   00049400   001b4c00   40300040
    .bss                  0000eae0   00202000   00000000   00000000   c0700080
  .edata                  0002a0a5   00211000   0002a200   001fe000   40300040
  .idata                  00004c70   0023c000   00004e00   00228200   c0300040
   .rsrc                  00007800   00241000   00007400   0022d000   c0300040
  .reloc                  0000d1bc   00249000   0000d200   00234400   42300040
     /14  .zdebug_aranges 0000103e   00257000   00001200   00241600   42400040
     /30     .zdebug_info 003561b2   0025a000   00356200   00242800   42100040
     /43   .zdebug_abbrev 0000d5aa   00a1f000   0000d600   00598a00   42100040
     /58     .zdebug_line 0004049b   00a76000   00040600   005a6000   42100040
     /71    .zdebug_frame 00000045   00b0c000   00000200   005e6600   42300040
     /85      .zdebug_str 00005b42   00b0d000   00005c00   005e6800   42100040
     /97      .zdebug_loc 0007dc89   00b34000   0007de00   005ec400   42100040
    /109   .zdebug_ranges 00012e79   00cae000   00013000   0066a200   42100040

Possibly related to bug #15350

[1] https://cygwin.com/ml/cygwin/2015-03/msg00080.html
Comment 1 Kai Tietz 2015-03-06 12:39:12 UTC
Yes, it is likely that we have here an issue in updating section size, if compression actually increases, instead of decreasing size.

IMO the saner approach would be to reject compression, if content gets larger as before
Comment 2 Jon Turney 2015-03-06 13:58:25 UTC
(In reply to Kai Tietz from comment #1)
> Yes, it is likely that we have here an issue in updating section size, if
> compression actually increases, instead of decreasing size.
> 
> IMO the saner approach would be to reject compression, if content gets
> larger as before

I think my patch is no good because an output PE which is unstripped but has compressed debug sections is not valid, because it breaks the "sections must have contiguous VMAs" constraint.

So not compressing if the section gets larger sounds like a better approach.  I'll have a go at implementing that.
Comment 3 Jon Turney 2015-03-07 22:08:47 UTC
Created attachment 8178 [details]
Don't compress section if it doesn't get smaller
Comment 4 Jon Turney 2015-03-07 22:11:32 UTC
(In reply to Jon TURNEY from comment #2)
> So not compressing if the section gets larger sounds like a better approach.
> I'll have a go at implementing that.

This turns out to be not quite so simple due to the bfd interface for dealing
with compressed sections, but an attempt is attached.

With this patch, objcopy --compress-debug-sections on the same test binary produces:

Name                      VirtSize   VMA        RawSize    Offset     Flags
   .text                  0017df74   00001000   0017e000   00000600   60500060
   .data                  00004184   0017f000   00004200   0017e600   c0700040
  .rdata                  000321e0   00184000   00032200   00182800   40700040
.buildid                  00000035   001b7000   00000200   001b4a00   40300040
      /4 .eh_frame        00049240   001b8000   00049400   001b4c00   40300040
    .bss                  0000eae0   00202000   00000000   00000000   c0700080
  .edata                  0002a0a5   00211000   0002a200   001fe000   40300040
  .idata                  00004c70   0023c000   00004e00   00228200   c0300040
   .rsrc                  00007800   00241000   00007400   0022d000   c0300040
  .reloc                  0000d1bc   00249000   0000d200   00234400   42300040
     /14 .zdebug_aranges  00002fb0   00257000   00001200   00241600   42400040
     /30 .zdebug_info     007c423e   0025a000   00356200   00242800   42100040
     /43 .zdebug_abbrev   00056487   00a1f000   0000d600   00598a00   42100040
     /58 .zdebug_line     0009576a   00a76000   00040600   005a6000   42100040
     /71 .debug_frame     00000038   00b0c000   00000200   005e6600   42300040
     /84 .zdebug_str      00026a8b   00b0d000   00005c00   005e6800   42100040
     /96 .zdebug_loc      001799f0   00b34000   0007de00   005ec400   42100040
    /108 .zdebug_ranges   00038b88   00cae000   00013000   0066a200   42100040

Note that the .debug_frame section didn't get compressed.

The unstripped output of objcopy can also successfully be executed.
Comment 5 Nick Clifton 2015-03-17 17:43:23 UTC
Created attachment 8192 [details]
Extended version of Jon's patch

Hi Jon,

  Your patch is good, but it misses a couple of points:

  * A change similar to the one in coffgen.c is needed in elf.c for ELF based targets.

  * The assembler needs to be updated as well so that it's --compress-debug-sections option only compresses when it is effective.

   * The documentation needs to be updated to explicitly mention that compression only happens when it is worthwhile.


I have uploaded an extended version of your patch which should handle these points.  Please could you try it out and let me know if it works for you.

Cheers
  Nick
Comment 6 Jon Turney 2015-03-17 19:06:01 UTC
(In reply to Nick Clifton from comment #5)

Thanks for taking a look.
 
>   Your patch is good, but it misses a couple of points:
> 
>   * A change similar to the one in coffgen.c is needed in elf.c for ELF
> based targets.

I thought I had done that.
 
>   * The assembler needs to be updated as well so that it's
> --compress-debug-sections option only compresses when it is effective.
>
>    * The documentation needs to be updated to explicitly mention that
> compression only happens when it is worthwhile.
> 
> 
> I have uploaded an extended version of your patch which should handle these
> points.  Please could you try it out and let me know if it works for you.

This seems to work fine.
Comment 7 H.J. Lu 2015-03-17 19:45:43 UTC
(In reply to Nick Clifton from comment #5)
> 
> I have uploaded an extended version of your patch which should handle these
> points.  Please could you try it out and let me know if it works for you.
> 

The testcase is missing in the patch.
Comment 8 cvs-commit@gcc.gnu.org 2015-03-18 15:49:15 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

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

commit 273a49858fa9c8d73de87167618ef99d70f9731a
Author: Jon Turney <jon.turney@dronecode.org.uk>
Date:   Wed Mar 18 15:47:13 2015 +0000

    Fix debug section compression so that it is only performed if it would make the section smaller.
    
    	PR binutils/18087
    gas	* doc/as.texinfo: Note that when gas compresses debug sections the
    	compression is only performed if it makes the section smaller.
    	* write.c (compress_debug): Do not compress a debug section if
    	doing so would make it larger.
    
    tests	* gas/i386/dw2-compress-1.d: Do not expect the .debug_abbrev or
    	.debug_info sections to be compressed.
    
    binu	* doc/binutils.texi: Note that when objcopy compresses debug
    	sections the compression is only performed if it makes the section
    	smaller.
    
    bfd	* coffgen.c (make_a_section_from_file): Only prepend a z to a
    	debug section's name if the section was actually compressed.
    	* elf.c (_bfd_elf_make_section_from_shdr): Likewise.
    	* compress.c (bfd_init_section_compress_status): Do not compress
    	the section if doing so would make it bigger.  In such cases leave
    	the section alone and return COMPRESS_SECTION_NONE.
Comment 9 Nick Clifton 2015-03-18 15:50:17 UTC
Patch committed, with an addition of a fix to the gas testsuite for the x86 targets as pointed out by H.J.
Comment 10 cvs-commit@gcc.gnu.org 2015-03-18 16:39:59 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=d94077e27d279c4ff0ee26bad786f89c350e2aea

commit d94077e27d279c4ff0ee26bad786f89c350e2aea
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Wed Mar 18 09:20:38 2015 -0700

    Add a testcase for PR gas/18087
    
    	PR gas/18087
    	* gas/i386/dw2-compress-1.d: Revert the last change.
    	* gas/i386/dw2-compress-3.d: New.
    	* gas/i386/dw2-compress-3.s: Likewise.
    	* gas/i386/i386.exp: Run dw2-compress-3 for ELF targets.
Comment 11 Jon Turney 2015-03-18 20:46:14 UTC
(In reply to Nick Clifton from comment #9)
> Patch committed, with an addition of a fix to the gas testsuite for the x86
> targets as pointed out by H.J.

Thanks.
Comment 12 cvs-commit@gcc.gnu.org 2015-03-19 12:16:58 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

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

commit e2575e05e73c3b2f08a8b5f579a504ac6a45ad60
Author: Nick Clifton <nickc@redhat.com>
Date:   Thu Mar 19 12:14:56 2015 +0000

    Fix building and testing dwarf debug section compression feature when zlib is not available.
    
    	PR gas/18087
    gas/test	* gas/i386/dw2-compress-1.d: Allow the test to pass regardless of
    	whether the .debug_info section was compressed on not.
    
    bfd	* compress.c (bfd_compress_section_contents): Do not define this
    	function if it is not used.
Comment 13 cvs-commit@gcc.gnu.org 2015-03-24 20:30:13 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=8d00121477371cfd1596118af062fe6ff4e263b7

commit 8d00121477371cfd1596118af062fe6ff4e263b7
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Tue Mar 24 13:27:52 2015 -0700

    Don't write the zlib header if not used
    
    No need to write the zlib header if compression didn't make the section
    smaller.
    
    	PR binutils/18087
    	* compress.c (bfd_compress_section_contents): Don't write the
    	zlib header and set contents as well as compress_status if
    	compression didn't make the section smaller.
    	(bfd_init_section_compress_status): Don't check compression
    	size here.
Comment 14 cvs-commit@gcc.gnu.org 2015-03-25 02:08:10 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=b422eb499be2858969fb7723b4e4e08cab20fcdc

commit b422eb499be2858969fb7723b4e4e08cab20fcdc
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Tue Mar 24 19:06:22 2015 -0700

    Don't write the zlib header if not used
    
    No need to write the zlib header if compression didn't make the section
    smaller.
    
    	PR gas/18087
    	* write.c (compress_debug): Don't write the zlib header if
    	compression didn't make the section smaller.
Comment 15 Jon Turney 2016-06-02 14:25:35 UTC
I believe this is resolved.