Page MenuHomePhabricator

[ELF] Fix copy relocation when two symbols share the same Symbol instance.
ClosedPublic

Authored by MaskRay on Jun 9 2018, 1:53 AM.

Details

Summary

In glibc libc.so.6, the multiple versions of sys_errlist share the same Symbol instance. When sys_errlist is copy relocated, we would replace SharedSymbol with Defined in the first iteration of the following loop:

for (SharedSymbol *Sym : getSymbolsAt<ELFT>(SS))

Then in the second iteration, we think the symbol (which has been changed to Defined) is still SharedSymbol and screw up (the address ends up in the Size field).

Event Timeline

MaskRay created this revision.Jun 9 2018, 1:53 AM

While you're doing that, maybe also fix "refer different places" in the comment on line 447 to be "refer to different places". :)

MaskRay updated this revision to Diff 150630.Jun 9 2018, 9:21 AM

Typo as pointed by Brooks Moses

MaskRay retitled this revision from [ELF] Fix copy relocation when two symbols share the same address. to [ELF] Fix copy relocation when two symbols share the same Symbol instance..Jun 9 2018, 9:27 AM
MaskRay edited the summary of this revision. (Show Details)
bkramer added inline comments.
ELF/Relocations.cpp
444

SmallSet of pointers doesn't have a stable iteration order. Does that matter here?

MaskRay marked an inline comment as done.Jun 11 2018, 9:41 AM
MaskRay added inline comments.
ELF/Relocations.cpp
444

The iteration order doesn't matter here. These SharedSymbol's are already in the symbol table. This function returns a set of pointers that will be changed to Defined's.

ruiu added a comment.Jun 11 2018, 11:30 AM

Could you add a test?

ELF/Relocations.cpp
443

I don't think you need this comment.

MaskRay updated this revision to Diff 150810.Jun 11 2018, 12:21 PM
MaskRay marked an inline comment as done.

Add test

MaskRay updated this revision to Diff 150811.Jun 11 2018, 12:21 PM

Remove a comment as suggested by ruiu@

MaskRay marked an inline comment as done.Jun 11 2018, 12:22 PM
Harbormaster completed remote builds in B19199: Diff 150811.
MaskRay updated this revision to Diff 150813.Jun 11 2018, 12:24 PM

CHECK -> CHECK-NEXT

MaskRay edited the summary of this revision. (Show Details)Jun 11 2018, 12:24 PM
ruiu accepted this revision.Jun 11 2018, 12:35 PM

LGTM

Thanks!

This revision is now accepted and ready to land.Jun 11 2018, 12:35 PM
MaskRay closed this revision.Jun 11 2018, 12:55 PM

Commited but somehow removed the
Differential revision: https://reviews.llvm.org/D47975
tag