629635 - Still some problems with GNU_REL_RO segments with elfhack

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Bengt-Erik Soderstrom]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0b10pre) Gecko/20110121 Firefox/4.0b10pre SeaMonkey/2.1b2pre
Build Identifier: 20110128084136

After the fix for bug 628988 I get this:

==
=== If you get failures below, please file a bug describing the error
=== and your environment (compiler and linker versions), and use
=== --disable-elf-hack until this is fixed.
===
/home/bengt-erik/mozilla-central/src/ff-opt/build/unix/elfhack/elfhack -b test.so
elfhack: /home/bengt-erik/mozilla-central/src/build/unix/elfhack/elf.cpp:285: Elf::Elf(std::ifstream&): Assertion `segment->getFileSize() == phdr.p_filesz' failed.
make[6]: *** [test.so] Avbruten (SIGABRT)
make[6]: *** Tar bort filen "test.so"
make[6]: Lämnar katalogen "/home/bengt-erik/mozilla-central/src/ff-opt/build/unix/elfhack"
make[5]: *** [libs] Fel 2



Reproducible: Always




Using 64-bit Ubuntu 10.10  (gcc-4.4.real (Ubuntu/Linaro 4.4.4-14ubuntu5) 4.4.5) 
with the Gold Linker (link (GNU coreutils) 8.5.) Disabling elfhack works, however

Comment 1

[Mike Hommey [:glandium]]

Could you attach your test.so ?

Comment 2

[Bengt-Erik Soderstrom]

(In reply to comment #1)
> Could you attach your test.so ?

Sorry, I don't now how. test.so gets deleted...Please advice

Comment 3

[Mike Hommey [:glandium]]

(In reply to comment #2)
> (In reply to comment #1)
> > Could you attach your test.so ?
> 
> Sorry, I don't now how. test.so gets deleted...Please advice

Run the gcc command (the one that make displays) by hand.

Comment 4

[Bengt-Erik Soderstrom]

Attachment: test.so

OK, attached test.so as requested

Comment 5

[Mike Hommey [:glandium]]

Okay, so this is due to the GNU_RELRO section being weirdly defined. Its end is not page aligned as it is supposed to be (and elfhack expects it to be), and it ends in the middle of the .got.plt section.

Comment 6

[Bengt-Erik Soderstrom]

So, for the time being we make it without elfhack (ac_add_options --disable-elf-hack).

Comment 7

[Bengt-Erik Soderstrom]

I filed this bug to http://sourceware.org/bugzilla/ as well. Let's see what the Gold people has to say

Comment 8

[Bengt-Erik Soderstrom]

I can now confirm that in order to use the Gold linker working here you need to have binutils version 2.21 installed.
This bug can now be closed.

Comment 9

[Mike Hommey [:glandium]]

I'll leave this bug open, because there are some GNU_REL_RO issues with elfhack that still need to be addressed.

Comment 10

[Justin Lebar (not reading bugmail)]

Ubuntu 10.10 currently ships with binutils 2.20, so this is pretty unfortunate.

It might be worth hooking into configure or something so this errors out in a more user-friendly manner.

Comment 12

[Mike Hommey [:glandium]]

(In reply to comment #10)
> Ubuntu 10.10 currently ships with binutils 2.20, so this is pretty unfortunate.
> 
> It might be worth hooking into configure or something so this errors out in a
> more user-friendly manner.

An alternative, considering the number of other possible problems, could be to turn it off by default and enable it on mozilla buildbots. Who would make the call?

Comment 13

[Mike Hommey [:glandium]]

Yet another alternative would be to strip the GNU_REL_RO segment if it exists, since a lot of gold versions seem to fail to create this segment properly...

Comment 14

[Mike Hommey [:glandium]]

Attachment: Make elfhack fail if different blocks need to be mapped to the same address

Comment 15

[Mike Hommey [:glandium]]

Attachment: Don't allow elfhack to move the .dynamic section

This makes complications when gold puts it as first writeable section, creating 4 PT_LOAD segments where 3 would be enough.

Comment 16

[Mike Hommey [:glandium]]

Attachment: Add ElfSegment::getOffset and ElfSegment::getAddr functions

This patch doesn't change anything, except it separates this void change from the actual change in next patch.

Comment 17

[Mike Hommey [:glandium]]

Attachment: Make elfhack keep PT_GNU_RELRO segments as they were originally

This should solve most if not all GNU_REL_RO related problems. It should be safe because none of the sections it contains should be moved.

Comment 18

[(dormant account)]

Comment on attachment 511123 [details] [diff] [review]
Make elfhack fail if different blocks need to be mapped to the same address


>+    // Two subsequent sections can't be mapped in the same page in memory
>+    // if they aren't in the same 4K block on disk.
>+    if ((getType() != SHT_NOBITS) && getAddr()) {
>+        if (((offset & ~4095) != (previous->getOffset() & ~4095)) &&
>+            ((getAddr() & ~4095) == (previous->getAddr() & ~4095)))
>+            throw std::runtime_error("Moving section would require overlapping segments");
>+    }
>+

use less convoluted math as per irc

Comment 19

[Mike Hommey [:glandium]]

Attachment: Make elfhack fail if different blocks need to be mapped to the same address

Comment 20

[Mike Hommey [:glandium]]

"Don't allow elfhack to move the .dynamic section" and "Make elfhack fail if different blocks need to be mapped to the same address" both make elfhack more robust in that it's not going to make unexpected broken changes but explicitly fail instead.

"Add ElfSegment::getOffset and ElfSegment::getAddr functions" changes nothing but prepares for the last patch.

"Make elfhack keep PT_GNU_RELRO segments as they were originally" should fix most issues with PT_GNU_RELRO segments, and allow to properly build with elfhack enabled on Ubuntu.

I think it would be better to have these patches in next beta.

Comment 21

[Mike Hommey [:glandium]]

Note this breaks build:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1297287495.1297288823.25170.gz

I think I know what's going on and will work on a patch tomorrow.

Comment 22

[Mike Hommey [:glandium]]

Attachment: Gracefully abort elfhack when injected code+data is bigger than original relocation section

When injected code+data is bigger than the old relocation section, there are good chances that following sections end up triggering the protection from attachment 511140 [details] [diff] [review] when getting the size of the whole file, and that's what was happening on try.

Successful try with this patch: 7889c5324651

Comment 23

[Mike Hommey [:glandium]]

Attachment: Gracefully abort elfhack when injected code+data is bigger than original relocation section

We still need to check if the overall size is reduced, otherwise we see things like this:
./libnssckbi.so: Reduced by -112 bytes
./libxul.so: Reduced by 1417104 bytes
./libxpcom.so: No gain. Aborting
./libmozsqlite3.so: Reduced by -116 bytes
./libplds4.so: No gain. Aborting
./libsmime3.so: Reduced by -116 bytes
./libnssutil3.so: No gain. Aborting
./libnssdbm3.so: Reduced by -116 bytes
./components/libdbusservice.so: No gain. Aborting
./components/libmozgnome.so: Reduced by -116 bytes
./components/libbrowsercomps.so: Reduced by -112 bytes
./components/libnkgnomevfs.so: Reduced by -116 bytes
./libplc4.so: No gain. Aborting
./libnspr4.so: Reduced by -116 bytes
./libsoftokn3.so: Reduced by -116 bytes
./libmozalloc.so: No gain. Aborting
./libfreebl3.so: Reduced by -112 bytes
./libssl3.so: Reduced by -112 bytes
./libnss3.so: Reduced by -112 bytes

which is mostly due to the growing of the section headers and sections string table.

Comment 25

[Mike Hommey [:glandium]]

http://hg.mozilla.org/mozilla-central/rev/acd473c662fb
http://hg.mozilla.org/mozilla-central/rev/8a96724c2a8d
http://hg.mozilla.org/mozilla-central/rev/c40b2aa98f90
http://hg.mozilla.org/mozilla-central/rev/d79e30ee7a15
http://hg.mozilla.org/mozilla-central/rev/19f13dea4d4a