[object] Fix reproducer crash when input file has no contents#1166
[object] Fix reproducer crash when input file has no contents#1166deepakshirkem wants to merge 1 commit into
Conversation
| return true; | ||
| if (!Input->getSize()) | ||
| if (!Input->getSize()) { | ||
| ThisConfig.raise(Diag::input_file_has_zero_size) << Input->decoratedPath(); |
There was a problem hiding this comment.
Are we emitting this diagnostic for non-existing file? If so, that is incorrect. Also with this modification, the link will fail for existing but 0-sized files. Is this an intended behavior change?
There was a problem hiding this comment.
Hi @parth-07, I think you are right in both cases. I will update the checks.
| # Verify that ELD does not crash when a linker script references a | ||
| # file that cannot be read (e.g. not found in sysroot). | ||
| # ELD should error out gracefully instead of segfaulting. | ||
| # Fix for issue #818. |
There was a problem hiding this comment.
github specific references can probably be left out.
There was a problem hiding this comment.
Hi @quic-areg, Thank You ::)). Will remove it.
ea77759 to
02f9cdf
Compare
|
Hi @parth-07, I have updated the fix. Here is the verified behavior Hi @quic-areg , Updated the test. Please go one more time. |
02f9cdf to
b8fb315
Compare
|
Hi @Parth / @quic-areg / @quic-seaswara , Updated the suggested test. Please review when get a chance and also the run pipeline. |
| RUN: %clang %clangopts -o %t1.o %p/Inputs/1.c -c | ||
| RUN: touch %t1.empty.o | ||
| RUN: llvm-ar rcs %t1.a %t1.o %t1.empty.o | ||
| RUN: llvm-ar rcsT %t1.thin.a %t1.o %t1.empty.o |
There was a problem hiding this comment.
Use the lit variables, do we know if this will work on windows ?
| RUN: %clang %clangopts -o %t1.o %p/Inputs/main.c -c | ||
| RUN: %not %link %linkopts %t1.o -T %p/Inputs/script.t 2>&1 | %filecheck %s | ||
| #END_TEST | ||
| CHECK: cannot read file |
b8fb315 to
fdfcc03
Compare
|
Hi @quic-seaswara, addressed your comments. |
fdfcc03 to
62a09af
Compare
|
Hi @quic-seaswara, Can you re-run the pipeline one more time. |
62a09af to
937fa19
Compare
|
Hi @quic-seaswara, I am not sure why the CI failed for that test case, as it is passing locally for me. I think it might be because my branch was out of date. I have now rebased it with the latest top/main branch. Could you please run the CI one more time? Sorry |
| // 2. File does not exist (MemArea = null): | ||
| // fatal_cannot_read_input already raised. Return false to | ||
| // prevent crash in downstream processing. | ||
| if (Input->getMemArea()) |
There was a problem hiding this comment.
We should not reach readAndProcessInput for an invalid Input *. We resolve the input path and create the MemoryArea for the input in Input::resolvePath(). This function should report the error and return false if the file does not exist.
There was a problem hiding this comment.
You were right. I found that the root cause is not in resolvePath(), but in resolvePathMappingFile(). The issue is that createMemoryArea() returns nullptr for the missing file, but setMemArea(nullptr) is still called and the function returns true, incorrectly signaling success to the caller. Added GDB logs to the comment as proof.
Really appreciate your review and pointing me in the right direction. Please take a look when you get a chance, and let me know if you have any additional test cases in mind.
937fa19 to
c49fa11
Compare
|
Hi @quic-areg / @quic-seaswara / @parth-07 , Here is the GDB log confirming the root cause. Observation The fix is in |
c49fa11 to
549f429
Compare
|
Hi @quic-seaswara / @quic-areg / @parth-07 , Does this need changes or can it be merged? |
parth-07
left a comment
There was a problem hiding this comment.
Please change the commit message and PR description to: Fix reproducer crash when input file has no contents.
Done. |
|
@deepakshirkem I do not see any update to the commit title. |
549f429 to
0caae32
Compare
Done! Changes have been pushed. Thank You ::(( |
| @@ -0,0 +1,11 @@ | |||
| #---SysrootScriptCrash.test----------- Executable -----------------# | |||
There was a problem hiding this comment.
@deepakshirkem This test passes even without the changes of this patch, right? Why did we add this test with this patch?
ELD crashes with a segfault when a reproduce tarball contains a linker script that references a file not found in the sysroot. The crash occurs because Input::resolvePathMappingFile() calls createMemoryArea() which returns nullptr when the file does not exist, but then calls setMemArea(nullptr) and returns true — incorrectly signaling success. This allows a null MemArea input to reach readAndProcessInput(), which eventually dereferences the null pointer inside ScriptLexer. Fixes qualcomm#818. Signed-off-by: deepakshirkem <deepakshirke509@gmail.com>
0caae32 to
a912861
Compare

Problem
ELD crashes with a segfault when a reproduce tarball contains a linker script that references a file not found in the sysroot.
Testing
Added test
SysrootScriptCrashthat verifies ELD errors out gracefully instead of segfaulting when a linker script references an unreadable file.Screenshot
Fixes #818
cc @quic-seaswara , @parth-07 , @quic-areg