Page MenuHomePhabricator

[COFF] Bounds check relocations
ClosedPublic

Authored by rnk on Jul 13 2017, 10:48 AM.

Details

Summary

This would have caught the invalid object file I used in my test case in
r307726. The OOB was only caught by ASan later, which is slow and
doesn't work on some platforms. LLD should do some basic input
validation itself. This check isn't perfect, so relocations can reach
OOB by up to seven bytes, but it's better than what we had and probably
cheap.

Event Timeline

rnk created this revision.Jul 13 2017, 10:48 AM
ruiu edited edge metadata.Jul 13 2017, 10:53 AM

This adds one virtual function call for each relocation. I believe it's negligible, but can you run the linker with and without this patch to see if this is fine?

grandinj added inline comments.
lld/COFF/Chunks.cpp
219

could hoist getSize() outside the loop?

rnk added a comment.Jul 13 2017, 12:53 PM
In D35371#808377, @ruiu wrote:

This adds one virtual function call for each relocation. I believe it's negligible, but can you run the linker with and without this patch to see if this is fine?

Hm, I hadn't noticed that. We know we're dealing with a SectionChunk, so the call doesn't have to be virtual. Maybe we should mark SectionChunk final to get that benefit elsewhere.

lld/COFF/Chunks.cpp
219

We should just do that. Even if we inlined the implementation, it's a load.

rnk updated this revision to Diff 106498.Jul 13 2017, 12:55 PM
  • Mark SectionChunk final
  • Hoist loop-invariant getSize call
ruiu accepted this revision.Jul 13 2017, 1:05 PM

LGTM

This revision is now accepted and ready to land.Jul 13 2017, 1:05 PM
This revision was automatically updated to reflect the committed changes.