Skip to content

[LLD] Fix crash on parsing ':ALIGN' in linker script #146723

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 6, 2025
Merged

[LLD] Fix crash on parsing ':ALIGN' in linker script #146723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lld/ELF/ScriptParser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1229,6 +1229,8 @@ SymbolAssignment *ScriptParser::readSymbolAssignment(StringRef name) {
// This is an operator-precedence parser to parse a linker
// script expression.
Expr ScriptParser::readExpr() {
if (atEOF())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will work, although I think it may be more idiomatic to use

if (errCount(ctx))
  return 0;

That way you don't need the comment to explain that atEOF will return true if there's an error. The return 0 comes from other places where an ErrAlways has occurred, such as

      ErrAlways(ctx) << loc << ": division by zero";
      return 0;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the CI this may have caused a problem with
FAIL: lld :: ELF/linkerscript/custom-section-type.s (1500 of 3105)

2025-07-02T15:45:44.9280956Z ld.lld: warning: section type mismatch for progbits
2025-07-02T15:45:44.9282076Z >>> /home/gha/actions-runner/_work/llvm-project/llvm-project/build/tools/lld/test/ELF/linkerscript/Output/custom-section-type.s.tmp/mismatch.o:(progbits): SHT_NOTE
2025-07-02T15:45:44.9283202Z >>> output section progbits: SHT_PROGBITS
2025-07-02T15:45:44.9283407Z 
2025-07-02T15:45:44.9283520Z ld.lld: warning: section type mismatch for expr
2025-07-02T15:45:44.9284363Z >>> /home/gha/actions-runner/_work/llvm-project/llvm-project/build/tools/lld/test/ELF/linkerscript/Output/custom-section-type.s.tmp/mismatch.o:(expr): Unknown
2025-07-02T15:45:44.9285740Z >>> output section expr: Unknown

Assuming this is related to the patch. It may be that we've terminated too early before enough context for the error message can be accumulated. Which may mean that the check needs to be put closer to the point where an infinite loop may occur. Or we need a different approach.

Copy link
Contributor Author

@parth-07 parth-07 Jul 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your inputs @smithp35.

although I think it may be more idiomatic to use

 if (errCount(ctx))
   return 0;

I agree that errCount(ctx) would work as well, however, I think it helps to make the code flow easier to understand and more intuitive if the behavior of parsing expression is consistent for both the error-case and the actual end-of-file case, and atEOF() covers both these cases. Please let me know your thoughts on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming this is related to the patch. It may be that we've terminated too early before enough context for the error message can be accumulated. Which may mean that the check needs to be put closer to the point where an infinite loop may occur. Or we need a different approach.

Yes, it was related to the patch. Thank you for sharing your thoughts. It turned out the error was due to the return value of getExpr() in the error path. The return value was an empty function object. It should be a 0-value equivalent of the lld::elf::Expr. I have fixed the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got a small suggestion that means we don't need the comment.

I agree that the comment was a little redundant. I have removed the comment.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates. This looks OK to me. I've added the maintainer MaskRay and MysteryMath, who has been involved in linker script parsing recently.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks ok to me as well. A few parser functions call atEOF() at the beginning, when they expect to consume at least one token.

return []() { return 0; };
// Our lexer is context-aware. Set the in-expression bit so that
// they apply different tokenization rules.
SaveAndRestore saved(lexState, State::Expr);
Expand Down
21 changes: 19 additions & 2 deletions lld/test/ELF/linkerscript/align-section.test
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,24 @@
# REQUIRES: x86
# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux /dev/null -o %t.o
# RUN: ld.lld -o %t --script %s %t.o -shared
# RUN: rm -rf %t && split-file %s %t && cd %t

# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux /dev/null -o a.o
# RUN: ld.lld --script a.t a.o -shared

# lld shouldn't crash.

#--- a.t
SECTIONS { .foo : ALIGN(2M) {} }

# RUN: not ld.lld --script b.t 2>&1 | FileCheck %s --match-full-lines --strict-whitespace

# lld should not crash and report the error properly.

# CHECK:{{.*}} error: b.t:3: malformed number: :
# CHECK:>>> S :ALIGN(4096) {}
# CHECK:>>> ^

#--- b.t
SECTIONS
{
S :ALIGN(4096) {}
}
Loading