[lld][ELF] Fix crash when relocations proceed relocated section #156354
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-lld Author: None (mykouHW) ChangesFix the error generated during the linking process when the relocation section is placed before the relocated section and the relocated section is not defined in the linker script. Issue Cause: If the However, during the construction and registration of the Solution: Full diff: https://github.com/llvm/llvm-project/pull/156354.diff 2 Files Affected:
diff --git a/lld/ELF/LinkerScript.cpp b/lld/ELF/LinkerScript.cpp
index 921128dae2bdb..067abbc42a13d 100644
--- a/lld/ELF/LinkerScript.cpp
+++ b/lld/ELF/LinkerScript.cpp
@@ -1037,10 +1037,14 @@ void LinkerScript::addOrphanSections() {
if (ctx.arg.relocatable && (isec->flags & SHF_LINK_ORDER))
continue;
- if (auto *sec = dyn_cast<InputSection>(isec))
- if (InputSectionBase *rel = sec->getRelocatedSection())
+ if (auto *sec = dyn_cast<InputSection>(isec)){
+ if (InputSectionBase *rel = sec->getRelocatedSection()){
+ if (auto *relIS = dyn_cast_or_null<InputSectionBase>(rel))
+ add(relIS);
if (auto *relIS = dyn_cast_or_null<InputSectionBase>(rel->parent))
add(relIS);
+ }
+ }
add(isec);
if (ctx.arg.relocatable)
for (InputSectionBase *depSec : isec->dependentSections)
diff --git a/lld/test/ELF/linkerscript/orphan-sections-init.s b/lld/test/ELF/linkerscript/orphan-sections-init.s
new file mode 100644
index 0000000000000..1701336f098e2
--- /dev/null
+++ b/lld/test/ELF/linkerscript/orphan-sections-init.s
@@ -0,0 +1,30 @@
+# REQUIRES: x86
+# RUN: rm -rf %t && mkdir -p %t
+# RUN: split-file %s %t && cd %t
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64 foo.s -o foo.o
+
+# RUN: ld.lld -r foo.o -T script.ld -o foo_mc.o
+
+# RUN: llvm-objcopy --rename-section .text=.com.text foo_mc.o foo_mc.o
+# RUN: llvm-objcopy --rename-section .rela.text=.rela.com.text foo_mc.o foo_mc.o
+
+# RUN: ld.lld -r foo_mc.o -T script.ld -o foo_mc_after.o
+
+#--- foo.s
+ .text
+ .globl foo
+ .p2align 4
+ .type foo,@function
+foo:
+ mov $bar, %rax
+
+
+
+#--- script.ld
+SECTIONS
+{
+ .rela.text 0 : { *(.rela.text) }
+ .text 0 : { *(.text) }
+}
+
|
|
@llvm/pr-subscribers-lld-elf Author: None (mykouHW) ChangesFix the error generated during the linking process when the relocation section is placed before the relocated section and the relocated section is not defined in the linker script. Issue Cause: If the However, during the construction and registration of the Solution: Full diff: https://github.com/llvm/llvm-project/pull/156354.diff 2 Files Affected:
diff --git a/lld/ELF/LinkerScript.cpp b/lld/ELF/LinkerScript.cpp
index 921128dae2bdb..067abbc42a13d 100644
--- a/lld/ELF/LinkerScript.cpp
+++ b/lld/ELF/LinkerScript.cpp
@@ -1037,10 +1037,14 @@ void LinkerScript::addOrphanSections() {
if (ctx.arg.relocatable && (isec->flags & SHF_LINK_ORDER))
continue;
- if (auto *sec = dyn_cast<InputSection>(isec))
- if (InputSectionBase *rel = sec->getRelocatedSection())
+ if (auto *sec = dyn_cast<InputSection>(isec)){
+ if (InputSectionBase *rel = sec->getRelocatedSection()){
+ if (auto *relIS = dyn_cast_or_null<InputSectionBase>(rel))
+ add(relIS);
if (auto *relIS = dyn_cast_or_null<InputSectionBase>(rel->parent))
add(relIS);
+ }
+ }
add(isec);
if (ctx.arg.relocatable)
for (InputSectionBase *depSec : isec->dependentSections)
diff --git a/lld/test/ELF/linkerscript/orphan-sections-init.s b/lld/test/ELF/linkerscript/orphan-sections-init.s
new file mode 100644
index 0000000000000..1701336f098e2
--- /dev/null
+++ b/lld/test/ELF/linkerscript/orphan-sections-init.s
@@ -0,0 +1,30 @@
+# REQUIRES: x86
+# RUN: rm -rf %t && mkdir -p %t
+# RUN: split-file %s %t && cd %t
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64 foo.s -o foo.o
+
+# RUN: ld.lld -r foo.o -T script.ld -o foo_mc.o
+
+# RUN: llvm-objcopy --rename-section .text=.com.text foo_mc.o foo_mc.o
+# RUN: llvm-objcopy --rename-section .rela.text=.rela.com.text foo_mc.o foo_mc.o
+
+# RUN: ld.lld -r foo_mc.o -T script.ld -o foo_mc_after.o
+
+#--- foo.s
+ .text
+ .globl foo
+ .p2align 4
+ .type foo,@function
+foo:
+ mov $bar, %rax
+
+
+
+#--- script.ld
+SECTIONS
+{
+ .rela.text 0 : { *(.rela.text) }
+ .text 0 : { *(.text) }
+}
+
|
There was a problem hiding this comment.
I recommend not using the word vulnerability in titles and description as this is usually reserved for security bugs.
I suggest something like `[lld][ELF] Fix crash when relocations proceed relocated section.
I also recommend putting the analysis in the linked issue into the description as this will form the commit message. It is much easier to find and search for changes with git log when the commit message has all the details.
lld/ELF/LinkerScript.cpp
Outdated
| if (auto *sec = dyn_cast<InputSection>(isec)) | ||
| if (InputSectionBase *rel = sec->getRelocatedSection()) | ||
| if (auto *sec = dyn_cast<InputSection>(isec)){ | ||
| if (InputSectionBase *rel = sec->getRelocatedSection()){ |
There was a problem hiding this comment.
could rel be renamed to relocated here?
Not part of your change, but I think it would be an improvement. When reviewing just the diff, it was difficult to tell from the name rel whether we are referring to the relocation section, or the section being relocated.
lld/ELF/LinkerScript.cpp
Outdated
| @@ -1037,10 +1037,14 @@ void LinkerScript::addOrphanSections() { | |||
| if (ctx.arg.relocatable && (isec->flags & SHF_LINK_ORDER)) | |||
| continue; | |||
|
|
|||
| if (auto *sec = dyn_cast<InputSection>(isec)) | |||
| if (InputSectionBase *rel = sec->getRelocatedSection()) | |||
| if (auto *sec = dyn_cast<InputSection>(isec)){ | |||
There was a problem hiding this comment.
Can you git clang-format the patch? I would expect a space between ){ i.e. (isec)) {
lld/ELF/LinkerScript.cpp
Outdated
| if (InputSectionBase *rel = sec->getRelocatedSection()) | ||
| if (auto *sec = dyn_cast<InputSection>(isec)){ | ||
| if (InputSectionBase *rel = sec->getRelocatedSection()){ | ||
| if (auto *relIS = dyn_cast_or_null<InputSectionBase>(rel)) |
There was a problem hiding this comment.
Could we add a comment before the if statement like
// Ensure creation of OutputSection for relocated section before relocation section.
|
|
||
| # RUN: ld.lld -r foo.o -T script.ld -o foo_mc.o | ||
|
|
||
| # RUN: llvm-objcopy --rename-section .text=.com.text foo_mc.o foo_mc.o |
There was a problem hiding this comment.
You can merge do both -rename-section commands with a single instance of llvm-objcopy.
| foo: | ||
| mov $bar, %rax | ||
|
|
||
|
|
There was a problem hiding this comment.
I recommend a CHECK of the llvm-readelf --sections output to make sure lld has created the link from the relocations correctly.
I've seen some scripts been run to find tests with no FileCheck output before so it is worth putting at least one in.
| # REQUIRES: x86 | ||
| # RUN: rm -rf %t && mkdir -p %t | ||
| # RUN: split-file %s %t && cd %t | ||
|
|
There was a problem hiding this comment.
Could you add a comment to explain the test. Without context the steps look strange. LLD uses an additional comment character to distinguish comments from lit commands, so comment lines would start ##
For example:
Test that lld's orphan section placement can handle a relocatable link where the relocation section is seen before the relocated section. To create a test case we need to first need to create a relocatable object with
the relocations before the relocated section. Then we rename these to make these orphans.
I tried to see if I could create the test case without llvm-objcopy but it seemed like I needed the linker script to have .rela.text as the pattern to get it to place first.
Fix the error generated during the linking process when the relocation section is placed before the relocated section and the relocated section is not defined in the linker script.
fb38a50 to
3cc58a3
Compare
fixes #156417
When the relocation section is placed before the relocated section and the relocated section is not defined in the linker script, an error will occur during the linking process.
Issue Cause:
In the judgment logic,
addOrphanSectionsassumes that theRelocatedSectionmust be processed before theRelocationSection. Under this assumption, theOutputSectionfor theRelocatedSectionhas already been constructed, and theparentrelationship associated with theInputSectionBasehas been established.If the
RelocationSectionis processed before theRelocatedSection, this assumption is violated. As a result, the conditionrel->parentevaluates to null, causingadd(relIS)to not execute. This skips the registration and construction process of theRelocatedSection, since itscreateOutputSectionandrecordSectionmethods have not yet been called at this point.However, during the construction and registration of the
RelocationSectionin theaddInputSecfunction, theRelocatedSectionis accessed. Since theRelocatedSectionhas not been constructed yet, attempting to access it results in a null pointer error.Solution:
Before processing the
RelocationSection, ensure that theOutputSectionfor theRelocatedSectionis created and registered. The creation and registration logic is protected by theaddfunction, which prevents duplicate creation. However, it may result in duplicate establishment of theparentrelationship, which does not affect correctness.