Skip to content

[linux] remove absolute rpath of /usr/lib/swift/linux added to many shared libraries #34303

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

Closed
wants to merge 1 commit into from

Conversation

finagolfin
Copy link
Member

@finagolfin finagolfin commented Oct 14, 2020

  • Explanation

This RPATH was presumably added as a backup, in case the libraries in a toolchain couldn't be found, but will not work well, so take it out.

This pull is a cherry-pick of #34023 to the 5.3 branch.

  • Scope

Since the stdlib and these other Swift shared libraries all have $ORIGIN set, the toolchain should be self-contained and never have to go to /usr/lib. In the rare occasion of a broken toolchain, eg libswiftCore.so is missing, the other libraries might find one in the system that works. That is such a rare scenario, that is as likely to break if a different Swift version is installed in the system, that it's better not to have this.

  • SR issue

At least partially resolves SR-5755, fully if he's not saying we should add CMAKE_INSTALL_PREFIX to the RPATH instead, which I don't think should be added by default either.

  • Risk

Low

  • Testing

I'm currently building SPM with the official Oct. 22 trunk snapshot build that had this pull merged and everything works fine.

  • Reviewer

I discussed these rpath issues in detail across the entire toolchain with @gottesmm and @compnerd.

@drexin, will a merge window for the 5.3.1 release be announced on the forums? I'd like to get this and swiftlang/swift-corelibs-xctest#308 in before the next patch release.

…hared libraries

This was presumably added as a backup, in case the libraries in a toolchain
couldn't be found, but will not work well, so take it out.
@finagolfin finagolfin requested a review from a team as a code owner October 14, 2020 08:07
@finagolfin
Copy link
Member Author

@gottesmm, this backports the rpath pull you merged today to the 5.3 branch.

@gottesmm
Copy link
Contributor

@swift-ci test

@finagolfin
Copy link
Member Author

Windows CI failure is unrelated.

@finagolfin
Copy link
Member Author

@drexin, there was no merge window announced for 5.3.1, doesn't look like there will be one for 5.3.2 either: could you get this pull and the approved swiftlang/swift-corelibs-xctest#308 into the 5.3 branch, as they're completely safe rpath changes for linux?

@drexin
Copy link
Contributor

drexin commented Dec 11, 2020

@buttaface I'm sorry, I can only merge during the merge windows.

@finagolfin
Copy link
Member Author

Alright, thanks for letting me know.

@finagolfin
Copy link
Member Author

finagolfin commented Dec 18, 2020

@tomerd, I'd like to finally get this in for 5.3.25.3.3.

@finagolfin
Copy link
Member Author

@drexin, would you run the CI again? We should be able to get this in now.

@drexin
Copy link
Contributor

drexin commented Jan 6, 2021

@swift-ci test

@tomerd
Copy link
Contributor

tomerd commented Jan 7, 2021

cc @airspeedswift to approve taking into 5.3.3

@tomerd
Copy link
Contributor

tomerd commented Jan 8, 2021

@buttaface @airspeedswift had some questions on urgency of this, i.e can it wait for the 5.4 release?

@finagolfin
Copy link
Member Author

On the one hand, yes, it has been this way on linux since almost the beginning, so it could be delayed just a couple months longer. On the other hand, it represents almost no risk of breakage and closes up a small security hole- the attacker would have to remove one of these affected Swift libraries like libswiftCore.so from the intended location and place a booby-trapped version in /usr/lib/swift/linux, both of which would be very difficult, though not impossible, without extensive access already- that I think it might as well be closed up now.

In the end, I'd be fine with waiting if the reviewers come to a different conclusion: just close the pull if you'd rather wait, as it's already in the 5.4 branch.

@tomerd
Copy link
Contributor

tomerd commented Jan 15, 2021

thanks @buttaface, we should probably just wait for 5.4

@tomerd tomerd closed this Jan 15, 2021
@finagolfin
Copy link
Member Author

No problem.

@finagolfin finagolfin deleted the linux-rpath branch January 15, 2021 05:22
@AnthonyLatsis AnthonyLatsis added the 🍒 release cherry pick Flag: Release branch cherry picks label May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍒 release cherry pick Flag: Release branch cherry picks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants