This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Do not crash on invalid section alignment.
ClosedPublic

Authored by grimar on Sep 30 2016, 2:14 AM.

Details

Summary

Case was revealed by id_000010,sig_08,src_000000,op_havoc,rep_4 from PR30540.

Out implementation uses uint32 for storing section alignment value,
what seems reasonable, though if value exceeds 32 bits bounds we have
truncation and final value of 0.

afl-min was applied to input.

Diff Detail

Event Timeline

grimar updated this revision to Diff 73012.Sep 30 2016, 2:14 AM
grimar retitled this revision from to [ELF] - Do not crash on invalid section alignment..
grimar updated this object.
grimar added reviewers: rafael, ruiu, davide.
grimar added subscribers: llvm-commits, grimar, evgeny777.
evgeny777 added inline comments.Sep 30 2016, 2:17 AM
ELF/InputSection.cpp
48

This doesn't make any sense for ELF32, does it? What about using MaxPageSize?

grimar added inline comments.Sep 30 2016, 5:54 AM
ELF/InputSection.cpp
48

For ELF32 it does not, I see no problem here.
About MaxPageSize - it seems there is no dependency in GNU linkers between section alignment and MaxPageSize,
why do you think this would be correct ?

evgeny777 added inline comments.Sep 30 2016, 6:45 AM
ELF/InputSection.cpp
48

Just checked: both gas and llvm-mc require alignment to be 32-bit value which is a power of 2. So better check for alignment can be:

bool NotPower2 = Header->sh_addralign & (Header->sh_addralign - 1);
if (Header->sh_addralign > 0x80000000 || NotPower2)
  error("...");
grimar added inline comments.Sep 30 2016, 6:51 AM
ELF/InputSection.cpp
48

It is not just gas/llvm-mc, it is requirement for this field: "Only 0 and positive integral powers of 2 are currently allowed as values for this field. A value of 0 or 1 indicates no address alignment constraints.",

though I do not know do we want to check such details in linker or not. As far I understand main aim to fix wrong inputs is "just please do not crash" and not to verify if the ELF object is correct. As you mentioned, llvm-mc already do this check for us.

rafael accepted this revision.Sep 30 2016, 11:04 AM
rafael edited edge metadata.

This is fine since it fixes a crash and we can always improve on it afterwards.

Can you write the test with yaml2obj or it too requires the value to be 32 bits?

This revision is now accepted and ready to land.Sep 30 2016, 11:04 AM
grimar added a comment.Oct 3 2016, 3:11 AM

This is fine since it fixes a crash and we can always improve on it afterwards.

Can you write the test with yaml2obj or it too requires the value to be 32 bits?

It is possible to write test using yapl2obj for this, will commit in a while.

grimar closed this revision.Oct 3 2016, 3:15 AM

r283097