Skip to content

[CMake] Don't add an extraneous rpath to the swift-frontend #64103

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.

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
May 24, 2023

Conversation

finagolfin
Copy link
Member

When checking rpaths for #63782, I just noticed these incorrect runpaths on linux:

> readelf -d swift*-DEVELOPMENT-SNAPSHOT-2023-0*-a-ubuntu20.04/usr/bin/swift-frontend | ag "File:|runpath"
File: swift-5.8-DEVELOPMENT-SNAPSHOT-2023-02-23-a-ubuntu20.04/usr/bin/swift-frontend
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN/../lib/swift/linux:$ORIGIN/../../lib/swift/linux]
File: swift-DEVELOPMENT-SNAPSHOT-2023-03-01-a-ubuntu20.04/usr/bin/swift-frontend
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN/../lib/swift/linux:$ORIGIN/../../lib/swift/linux]

It looks like this line was incorrectly added in #60943, then incorporated into #61426 and merged.

This line is unused on Darwin because the CI builds with BOOTSTRAPPING-WITH-HOSTLIBS, but the incorrect runpath should show up on macOS too if a full bootstrap were done.

@zoecarver and @DougGregor, let me know what you think.

@finagolfin
Copy link
Member Author

@rintaro, you spun out _add_swift_runtime_link_flags() last year in 9f4ce61, maybe you can chime in on whether it's properly used here.

@finagolfin
Copy link
Member Author

Pinging @ktoso, haven't gotten a response on this so far and it's about to ship with the 5.8 release.

@finagolfin
Copy link
Member Author

Ping @eeckstein, you have some experience with these rpath issues with your bootstrapping work.

@eeckstein
Copy link
Contributor

Sorry, I can't remember the details. What you should do is to test all affected bootstrapping modes and see if the RPATH in the binary is set correct and if the final swift-frontend is loading the correct libraries (with otool -L on macos)

@finagolfin
Copy link
Member Author

@eeckstein, nothing wrong with your bootstrapping work, the issue is this line that I'm removing in this pull, that was added by Doug and Zoe last year. It adds the incorrect second rpath shown in my OP for linux, and should add it incorrectly for macOS too for full bootstraps.

I'd simply like your opinion on if this line should be removed.

@eeckstein
Copy link
Contributor

I can't tell for sure without testing it.

@finagolfin
Copy link
Member Author

@edymtt, perhaps you can review?

@finagolfin
Copy link
Member Author

@CodaFi, would you take a look at this?

@finagolfin
Copy link
Member Author

@etcwilde, any input on this issue?

@edymtt
Copy link
Contributor

edymtt commented May 3, 2023

@buttaface apologies for acknowledging this late -- at the moment I am busy with other efforts, so cannot devote the time to reason on any impact of this change.

In the meantime, could you comment on the impact this has on the Linux build?

@finagolfin
Copy link
Member Author

@edymtt, I believe this is just a cleanup of an extra unneeded runpath added on linux and certain macOS configs. This could be a mild security issue in certain system install layouts and cause libraries to be loaded that shouldn't be, so it is better to clean it up.

If you would run the CI on this pull, that should give us an idea if this line was unneeded in the first place.

@finagolfin
Copy link
Member Author

@artemcm, would you run the CI on this?

@edymtt
Copy link
Contributor

edymtt commented May 5, 2023

@swift-ci please test

@finagolfin
Copy link
Member Author

@edymtt, passed CI, which heavily implies this line was extraneous all along, though the CI obviously doesn't test all configurations.

I'm fairly certain it is never needed for linux, I just can't speak for some obscure macOS config.

@finagolfin
Copy link
Member Author

@DougGregor, you committed this line, can you review?

@edymtt
Copy link
Contributor

edymtt commented May 22, 2023

Found some time to review this, and in fact I see that _add_swift_runtime_link_flags is called for the swift-frontend target twice -- once as part of add_swift_host_tool and once explicitly -- and that the former invocation causes the addition of a second rpath when building for Linux (that does not seem to happen for Darwin).

@finagolfin -- could you check if my understanding of your concern is correct?

(in the meantime I will check on the "obscure macOS configs")

@edymtt
Copy link
Contributor

edymtt commented May 22, 2023

@swift-ci please test

@edymtt
Copy link
Contributor

edymtt commented May 22, 2023

@swift-ci please build toolchain

@finagolfin
Copy link
Member Author

@edymtt, yep, that is the problem, you got it.

@finagolfin
Copy link
Member Author

Mac toolchain build failure is unrelated, some libcxx issue:

CMake Error at test/CMakeLists.txt:153 (message):
  Couldn't find libcxx build in
  '/Users/ec2-user/jenkins/workspace/swift-PR-toolchain-macos/branch-main/buildbot_osx/libcxx-macosx-x86_64'.
  To run the test-suite for a standalone LLDB build please build libcxx and
  point LLDB_TEST_LIBCXX_ROOT_DIR to it.

@edymtt
Copy link
Contributor

edymtt commented May 23, 2023

Agree, that seems related to this LLVM PR

@edymtt
Copy link
Contributor

edymtt commented May 24, 2023

Double checked that in the generated linux toolchain we have only one rpath

$ readelf -d usr/bin/swift-frontend | grep -E "File:|runpath"
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN/../lib/swift/linux]

Copy link
Contributor

@edymtt edymtt left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for noticing this!

@edymtt edymtt merged commit ccae422 into swiftlang:main May 24, 2023
@finagolfin
Copy link
Member Author

Thanks @edymtt, I will submit this for the release branches next.

edymtt pushed a commit that referenced this pull request May 29, 2023
Cherrypick of #64103

Explanation: The current 5.9 snapshot has an incorrect relative runpath added to the compiler on linux, the second one:

```
> readelf -d swift-5.9-DEVELOPMENT-SNAPSHOT-2023-05-22-a-ubuntu20.04/usr/bin/swift-frontend | ag runpath
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN/../lib/swift/linux:$ORIGIN/../../lib/swift/linux]
This pull fixes that, removing a mild security risk when running the compiler.
```

Scope: Only affects the compiler runpath on linux

Issue: None

Risk: low, as it only removes an unused runpath for the linux compiler

Testing: Passed all CI on trunk

Reviewer: @edymtt
edymtt added a commit that referenced this pull request Jun 14, 2023
)

Cherrypick of #64103

__Explanation__: The 5.8 release has an incorrect relative runpath added to the compiler on linux, the second one:

```
> readelf -d swift-5.8-RELEASE-ubuntu20.04/usr/bin/swift-frontend | ag runpath
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN/../lib/swift/linux:$ORIGIN/../../lib/swift/linux]
This pull fixes that, removing a mild security risk when running the compiler.
```

__Scope__: Only affects the compiler runpath on linux

__Issue__: None

__Risk__: low, as it only removes an unused runpath for the linux compiler

__Testing__: Passed all CI on trunk

__Reviewer__: @edymtt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants