Page MenuHomePhabricator

[ELF] Shuffle .init_array/.fini_array with --shuffle-sections=
ClosedPublic

Authored by MaskRay on Feb 19 2020, 11:30 PM.

Details

Summary

Useful for detecting static initialization order fiasco.

Diff Detail

Event Timeline

MaskRay created this revision.Feb 19 2020, 11:30 PM
grimar added inline comments.Feb 20 2020, 1:20 AM
lld/test/ELF/shuffle-sections-init-fini.s
2

This test passes for me without any code changes.

12

I think this tests nothing. If you have the same result with different seeds, it probably means that
--shuffle-sections= does not work as expected.

What I think can be done to test it:
Run LLD without --shuffle-sections and then check that with this option, the result is just different.
e.g.:

# COMMON:       Hex dump of section '.init_array'
# WITHOUT-NEXT: ff0100
# WITH-NOT:     ff0100
...
18

You do not need "unique" here it seems.

MaskRay marked an inline comment as done.Feb 20 2020, 7:33 AM
MaskRay added inline comments.
lld/test/ELF/shuffle-sections-init-fini.s
12

Random funtion implementations are different across platforms. We may have to create a large number of sections to ensure the result will be different.

MaskRay updated this revision to Diff 245673.Feb 20 2020, 9:21 AM

Improve tests

respindola added inline comments.Feb 20 2020, 1:47 PM
lld/ELF/Writer.cpp
1364

This will break code that uses init_priority, no?

I think we can only shuffle sections of equal priority.

MaskRay marked 2 inline comments as done.Feb 20 2020, 1:55 PM
MaskRay added inline comments.
lld/ELF/Writer.cpp
1364

Priorities are respected. Sections with the same priority can be shuffled.

See the test below, .init_array.1 is the first.

# CHECK:      Hex dump of section '.init_array'
# CHECK-NEXT: 0x{{[0-9a-f]+}} ff
# ORDERED-SAME: 000102 03040506 0708090a 0b
# SHUFFLED-NOT: 000102 03040506 0708090a 0b
respindola added inline comments.Feb 20 2020, 4:31 PM
lld/ELF/Writer.cpp
1364

Oh, I see. I was somehow reading a return after sortISDBySectionOrder, sorry.
What happens is that we first shuffle and then depend on functions like sortInitFini producing a valid order. Sounds reasonable.

What about the script->hasSectionsCommand case? Don't we have to skip sortISDBySectionOrder in when it is true?

MaskRay updated this revision to Diff 245767.Feb 20 2020, 4:51 PM
MaskRay marked an inline comment as done.

Improve comment.
Add a test case for script->hasSectionsCommand case

MaskRay marked 3 inline comments as done.Feb 20 2020, 4:52 PM
MaskRay added inline comments.
lld/ELF/Writer.cpp
1364

I think we can also sort for the script->hasSectionsCommand case. All priorities are ignored. All SHT_INIT_ARRAY sections are shuffled together.

Added a test case.

MaskRay marked 2 inline comments as done.Feb 20 2020, 4:53 PM
respindola added inline comments.Feb 20 2020, 5:18 PM
lld/ELF/Writer.cpp
1364

Sounds reasonable. When using sections the linker script is probably what is responsible for producing the right order and we only sort inside each ISD.

This LGTM, but should probably also get approved by someone more recently active in lld.

MaskRay added a subscriber: psmith.Feb 20 2020, 5:32 PM
grimar accepted this revision.Feb 21 2020, 12:19 AM

LGTM

lld/test/ELF/shuffle-sections-init-fini.s
12

Yes, I think it is fine for now. As far I understand std::mt19937 (i.e. a random number engine based on Mersenne Twister algorithm) should produce the same result on different platforms, because implementation should base on the same algorithm.
As was said in the different thread, the problem is probably comes from std::shuffle, then if we one day get llvm's implementation of one,
it will be possible to simplify this test.
Anyways the number of sections you are adding in this test looks sufficient and not that large to me.

This revision is now accepted and ready to land.Feb 21 2020, 12:19 AM
MaskRay updated this revision to Diff 245867.Feb 21 2020, 8:15 AM

Update a comment

This revision was automatically updated to reflect the committed changes.