fix: Error on R_X86_64_PC32 to DSO symbols in non-executable sections in PIE - #2132
Conversation
97a6e57 to
dfecb06
Compare
dfecb06 to
3838b73
Compare
|
@davidlattimore Please review when you get a chance. |
| let flags = layout.flags_for_symbol(local_symbol_id); | ||
| if layout.symbol_db.output_kind.is_position_independent() | ||
| && (flags.is_interposable() || flags.is_dynamic()) | ||
| && !section_info.section_flags.contains(shf::EXECINSTR) |
There was a problem hiding this comment.
It seems to me that there might be 4 different scenarios:
- code references code
- data references code
- code references data
- data references data
My guess is that the first two are likely OK, while the second two are likely not OK. It seems that you're checking whether the section to which the relocation is applied is executable. But, what matters is probably not where the reference is from, but what it's a reference to. Is it possible to make tests for all 4 combinations?
3838b73 to
2283a75
Compare
| //#Shared:pie-pc32-dso-shared-fn.s | ||
| //#LinkArgs:-pie | ||
| //#RunEnabled:false | ||
| //#SkipLinker:ld |
There was a problem hiding this comment.
I'm guessing that GNU ld just issues a warning. Are we matching lld's behaviour for all these cases? It'd be good to enable lld so that we can verify that we are.
There was a problem hiding this comment.
@davidlattimore Added EnableLinker:lld for cases 1, 2 and 4.
For case 3, lld accepts R_X86_64_PC32 to a FUNC DSO symbol via canonical PLT, so it doesn't error only Wild errors in that case, hence SkipLinker:lld there.
There was a problem hiding this comment.
The comment in the test says:
// SkipLinker:lld because the test framework passes -pie to the shared object build,
// which conflicts with lld's -shared flag. Wild correctly errors on this case.
It's hard for me to see that this is the same reason. Can you clarify?
Also, I think we have a way to override the linker flags used when linking a shared object. If we don't, then we should. I think you can list them after a ':' on the #Shared line.
I'm fuzzy on where we currently stand with canonical PLT entries. I know I don't like them (much like copy relocations), but I thought we did them.
There was a problem hiding this comment.
The comment in the test says:
// SkipLinker:lld because the test framework passes -pie to the shared object build,
// which conflicts with lld's -shared flag. Wild correctly errors on this case.
It's hard for me to see that this is the same reason. Can you clarify?
Also, I think we have a way to override the linker flags used when linking a shared object. If we don't, then we should. I think you can list them after a ':' on the #Shared line.
The comment was misleading the real reason for SkipLinker:lld is that lld creates a canonical PLT entry for R_X86_64_PC32 to STT_FUNC DSO symbols and succeeds, while Wild doesn't support canonical PLT and errors instead. Also found LinkSoArgs directive which can override shared object linker flags will use that to clean up the test once canonical PLT behavior is decided.
I'm fuzzy on where we currently stand with canonical PLT entries. I know I don't like them (much like copy relocations), but I thought we did them.
After checking, Wild does not implement canonical PLT entries. For case 3 (R_X86_64_PC32 to STT_FUNC DSO symbol from .text), lld succeeds via canonical PLT while Wild errors. For now I've kept Wild erroring since it's safer than silently producing a broken binary. If canonical PLT support is needed, I can file a separate issue and fix it in a follow-up PR.
a63dbd6 to
7c7650e
Compare
| //#Shared:pie-pc32-dso-shared-fn.s | ||
| //#LinkArgs:-pie | ||
| //#RunEnabled:false | ||
| //#SkipLinker:ld |
There was a problem hiding this comment.
The comment in the test says:
// SkipLinker:lld because the test framework passes -pie to the shared object build,
// which conflicts with lld's -shared flag. Wild correctly errors on this case.
It's hard for me to see that this is the same reason. Can you clarify?
Also, I think we have a way to override the linker flags used when linking a shared object. If we don't, then we should. I think you can list them after a ':' on the #Shared line.
I'm fuzzy on where we currently stand with canonical PLT entries. I know I don't like them (much like copy relocations), but I thought we did them.
There was a problem hiding this comment.
If it's not too much trouble, could we rename the tests to indicate which scenario they are - e.g. instead of numbered cases include "data-ref-data", "data-ref-code" etc in the name. Alternatively, just add a one line comment to the start of each test saying which scenario it is.
There was a problem hiding this comment.
Yeah, that makes more sense. I have updated the test names to be more descriptive and also added a one line comment at the beginning of each test indicating the scenario.
… in PIE R_X86_64_PC32 to a DSO symbol in a non-executable section (e.g. .data) is invalid in PIE output. Wild was silently producing a broken binary. R_X86_64_PC32 in executable sections (.text) remains valid as it goes via PLT. lld errors in this case; GNU ld only warns. Issue wild-linker#2114
7c7650e to
0793e33
Compare
R_X86_64_PC32 to a DSO symbol in a non-executable section (e.g. .data) is invalid in PIE output. Wild was silently producing a broken binary. R_X86_64_PC32 in executable sections (.text) remains valid as it goes via PLT.
lld errors in this case; GNU ld only warns.
Issue #2114