982014 - scan_relocs_for_code in elfhack.cpp may not work as intended

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Tooru Fujisawa [:arai]]

Attachment: build log, test-array.so in original and modified tree, objdump and readelf result

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140306171728

Steps to reproduce:

in build/unix/elfhack/elfhack.cpp:221, scan_relocs_for_code
>     template <typename Rel_Type>
>     void scan_relocs_for_code(ElfRel_Section<Rel_Type> *rel)
>     {
>         ElfSymtab_Section *symtab = (ElfSymtab_Section *)rel->getLink();
>         for (auto r = rel->rels.begin(); r != rel->rels.end(); r++) {
>             ElfSection *section = symtab->syms[ELF32_R_SYM(r->r_info)].value.getSection();
>             if (section) {
>                 for (ElfSection *s = elf->getSection(1); s != nullptr; s = s->getNext()) {
>                     if (section == s)
>                         section = nullptr;
>                         break;
>                 }
>             }
>             add_code_section(section);
>         }
>     }

"break" is executed regardless of if-condition, and inner for-loop runs only once,
all sections except first one are not checked.

I modified it to following, and built it:
>                 for (ElfSection *s = elf->getSection(1); s != nullptr; s = s->getNext()) {
>                     if (section == s) {
>                         section = nullptr;
>                         break;
>                     }
>                 }

However, test fails with following error:
> 47:51.22 ===
> 47:51.23 === If you get failures below, please file a bug describing the error
> 47:51.23 === and your environment (compiler and linker versions), and use
> 47:51.23 === --disable-elf-hack until this is fixed.
> 47:51.23 ===
> 47:51.23 # Fail if the library doesn't have INIT_ARRAY .dynamic info
> 47:51.23 readelf -d test-array.so | grep '(INIT_ARRAY)'
> 47:51.23  0x00000019 (INIT_ARRAY)                 0x5ae8
> 47:51.23 /home/arai/projects/gecko-dev/obj-elf/build/unix/elfhack/elfhack -b -f test-array.so
> 47:51.24 test-array.so: Reduced by 3948 bytes
> 47:51.24 # Fail if the backup file doesn't exist
> 47:51.24 [ -f 'test-array.so.bak' ]
> 47:51.24 # Fail if the new library doesn't contain less relocations
> 47:51.24 [ $(objdump -R test-array.so.bak | wc -l) -gt $(objdump -R test-array.so | wc -l) ]
> ...
> 47:51.28 gcc -o dummy dummy.o -lpthread  -Wl,-z,noexecstack -Wl,-z,text -Wl,--build-id -B /home/arai/projects/gecko-dev/obj-elf/build/unix/gold    -Wl,-rpath-link,/home/arai/projects/gecko-dev/obj-elf/dist/bin -Wl,-rpath-link,/usr/local/lib
> 47:51.30 # Will either crash or return exit code 1 if elfhack is broken
> 47:51.30 LD_PRELOAD=/home/arai/projects/gecko-dev/obj-elf/build/unix/elfhack/test-array.so /home/arai/projects/gecko-dev/obj-elf/build/unix/elfhack/dummy
> 47:51.30 Segmentation fault

I compared test-array.so in original tree and modified one, with objdump/readelf,
but I'm not sure what's wrong with it.

[objdump]
original
> 000003bc <.elfhack.text.v0>:
> 3bc:  56                      push   %esi
> 3bd:  53                      push   %ebx
> 3be:  e8 67 00 00 00          call   42a <bar+0x42a>
> 3c3:  81 c3 3d fc ff ff       add    $0xfffffc3d,%ebx
> ...
> 429:  c3                      ret
> 42a:  8b 1c 24                mov    (%esp),%ebx
> 42d:  c3                      ret
modified
> 000003bc <.elfhack.text.v0>:
> 3bc:   56                      push   %esi
> 3bd:   53                      push   %ebx
> 3be:   e8 3d fc ff ff          call   0 <bar>
> 3c3:   81 c3 3d fc ff ff       add    $0xfffffc3d,%ebx
> ...
> 429:   c3                      ret

[readelf]
original
>  [ 7] .gnu.version_r    VERNEED         0000039c 00039c 000020 00   A  3   1  4
>  [ 8] .elfhack.text.v0  PROGBITS        000003bc 0003bc 000072 00  AX  0   0  1
>  [ 9] .elfhack.data.v0  PROGBITS        00000430 000430 000010 08   A  0   0  8
modified
>  [ 7] .gnu.version_r    VERNEED         0000039c 00039c 000020 00   A  3   1  4
>  [ 8] .elfhack.text.v0  PROGBITS        000003bc 0003bc 00006e 00  AX  0   0  1
>  [ 9] .elfhack.data.v0  PROGBITS        00000430 000430 000010 08   A  0   0  8

The crash might be caused by my mistake, because I run it on chrooted 32bit env on 64bit env.
Anyway, the code may have a problem in either its indentation or logic.

Could anyone verify it?

Comment 1

[Mike Hommey [:glandium]]

I need to look at it again with fresh eyes, bug I think this is how to fix it:

@@ -225,10 +226,11 @@
         for (auto r = rel->rels.begin(); r != rel->rels.end(); r++) {
             ElfSection *section = symtab->syms[ELF32_R_SYM(r->r_info)].value.getSection();
             if (section) {
-                for (ElfSection *s = elf->getSection(1); s != nullptr; s = s->getNext()) {
-                    if (section == s)
+                for (auto s = code.begin(); s != code.end(); ++s) {
+                    if (section == *s) {
                         section = nullptr;
                         break;
+                    }
                 }
             }
             add_code_section(section);

Comment 2

[Mike Hommey [:glandium]]

Attachment: Fix what sections are copied from injection object after bug 932737

This fixes two issues with the current code, and makes things clearer.

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8399817 [details] [diff] [review]
Fix what sections are copied from injection object after bug 932737

Review of attachment 8399817 [details] [diff] [review]:
-----------------------------------------------------------------

Hooray for C's lack of fine-grained loop control structures.

Comment 4

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a06c8ddc5a78

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a06c8ddc5a78