Page MenuHomePhabricator

[llvm-objcopy] - Check dynamic relocation sections for broken references.
ClosedPublic

Authored by grimar on Apr 17 2019, 7:05 AM.

Details

Summary

This is a fix for https://bugs.llvm.org/show_bug.cgi?id=41371.

Currently, it is possible to break the sh_link field of the dynamic relocation section
by removing the section it refers to. The patch fixes an issue and adds 2 test cases.

One of them shows that it does not seem possible to break the sh_info field.
That is explained in the comments. I added an assert to verify this.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Apr 17 2019, 7:05 AM
grimar marked an inline comment as done.Apr 17 2019, 7:06 AM
grimar added inline comments.
test/tools/llvm-objcopy/ELF/dynrelocsec-remove-shlink-reference.test
24 ↗(On Diff #195563)

This TODO can be removed if we land D60820.

So overall this looks good and literally yesterday I'd have just given a +2 but James noticed an edge case we should think about. If there are ever reasons to have a dynamic relocation without a symbol table (I don't know of any) we should hold off and account for the edge case. O think that concern only applies to relocatable files however.

So overall this looks good and literally yesterday I'd have just given a +2 but James noticed an edge case we should think about. If there are ever reasons to have a dynamic relocation without a symbol table (I don't know of any) we should hold off and account for the edge case. O think that concern only applies to relocatable files however.

In theory, I think R_X86_64_RELATIVE relocations should not need a symbol table since they just adjust a place by program base.
This will be easy to support after landing the D60324 which introduce the --allow-broken-links option.
We can just wait for it and I'll update this patch then or I could do that in a follow-up.

FYI, I've just landed D60324, and approved D60820. This looks good, except for the edge case mentioned by @jakehehrlich. The code should also implement --allow-broken-links now that that has landed.

test/tools/llvm-objcopy/ELF/dynrelocsec-remove-shinfo-reference.test
11 ↗(On Diff #195563)

I'm not too bothered, but I find it marginally easier to read the line as a continuation if the '|' is on the second line.

grimar updated this revision to Diff 195718.Apr 18 2019, 4:40 AM
grimar marked an inline comment as done.
  • Addressed comments, rebased, updated the test case.

Hmm...right now we don't parse the dynamic symbol table but parsing it just enough to check if no symbols are needed and then not throwing an error if no symbol table is needed seems like a better option than using that flag. That flag should of course also be respected here however.

Maybe we should land this and wait for someone to have that issue. Can someone file a bug about that issue now?

Maybe we should land this and wait for someone to have that issue. Can someone file a bug about that issue now?

Right now we already have a bug saying that "If you strip a section that is referred to by a dynamic relocation section (e.g. the .dynsym),
llvm-objcopy lets it happen assuming that there are no other references".
(https://bugs.llvm.org/show_bug.cgi?id=41371)

As far I understand, after changing the behavior, we might want to post a bug saying something like the following, right?
"Currently when --allow-broken-links is *not* given we always report a error when removing a symbol table used by a dynamic relocation section.
Though it should be possible to do that only in a case when we know that using symbols from dynamic symbol table."

And then we need to land this patch first (as PR41371 already covers the current behavior I think?).

jhenderson accepted this revision.Apr 29 2019, 8:32 AM

LGTM. I agree that inspecting the dynamic relocations to see if there are any references to symbols is a separate issue. Indeed, it technically also applies to regular relocations, not just dynamic ones. There is a potential performance issue here though, which I think might be a problem if we tried to pass the relocations to do this.

test/tools/llvm-objcopy/ELF/dynrelocsec-remove-shlink-reference.test
12 ↗(On Diff #195718)

Either "with --allow-broken-links" or "with the --allow-broken-links switch"

This revision is now accepted and ready to land.Apr 29 2019, 8:32 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2019, 4:00 AM