[CMake] fix runpath for ELF platforms

[finagolfin]

Remove the absolute path to the host toolchain's stdlib from the three Foundation shared libraries. Also, add it as a CMake BUILD_PATH to plutil, so that it's removed upon installation.

Otherwise, you see the following in the latest official 5.3 snapshot release for linux:

> readelf -d swift-5.3-DEVELOPMENT-SNAPSHOT-2020-05-04-a-ubuntu18.04/usr/lib/swift/linux/libFoundation*so swift-5.3-DEVELOPMENT-SNAPSHOT-2020-05-04-a-ubuntu18.04/usr/bin/plutil | ag "File:|runpath"
File: swift-5.3-DEVELOPMENT-SNAPSHOT-2020-05-04-a-ubuntu18.04/usr/lib/swift/linux/libFoundation.so
 0x000000000000001d (RUNPATH)            Library runpath: [/home/buildnode/jenkins/workspace/oss-swift-5.3-package-linux-ubuntu-18_04/build/buildbot_linux/swift-linux-x86_64/lib/swift/linux:$ORIGIN]
File: swift-5.3-DEVELOPMENT-SNAPSHOT-2020-05-04-a-ubuntu18.04/usr/lib/swift/linux/libFoundationNetworking.so
 0x000000000000001d (RUNPATH)            Library runpath: [/home/buildnode/jenkins/workspace/oss-swift-5.3-package-linux-ubuntu-18_04/build/buildbot_linux/swift-linux-x86_64/lib/swift/linux:$ORIGIN]
File: swift-5.3-DEVELOPMENT-SNAPSHOT-2020-05-04-a-ubuntu18.04/usr/lib/swift/linux/libFoundationXML.so
 0x000000000000001d (RUNPATH)            Library runpath: [/home/buildnode/jenkins/workspace/oss-swift-5.3-package-linux-ubuntu-18_04/build/buildbot_linux/swift-linux-x86_64/lib/swift/linux:$ORIGIN]
File: swift-5.3-DEVELOPMENT-SNAPSHOT-2020-05-04-a-ubuntu18.04/usr/bin/plutil
 0x000000000000001d (RUNPATH)            Library runpath: [/home/buildnode/jenkins/workspace/oss-swift-5.3-package-linux-ubuntu-18_04/build/buildbot_linux/swift-linux-x86_64/lib/swift/linux]

[finagolfin]

There's a plutil test that fails without this rpath to the host toolchain's stdlib, so add it back in as a BUILD_RPATH below, ie an rpath that's removed by CMake when the binary is installed.

[spevans]

@swift-ci test linux

[finagolfin]

Something's missing, I'll build again locally and take a look.

[finagolfin]

I'm unable to reproduce the single test failure on Arch linux x86_64 with this pull applied against latest master, so I've added another debugging commit to dump the BUILD_RPATH and make sure it's being extracted properly when run on the CI.

@spevans, could you run the linux CI again?

[spevans]

@swift-ci test linux

[finagolfin]

That wasn't it, it's picking up the runtime path from the Swift compiler fine. The only other thing I can think of is that DT_RUNPATH is being handled differently on these linux distros, so I now hardcode the CI toolchain rpath for libFoundation.so just to see if it fixes plutil, as that's the only one of the three Foundation libraries it depends on.

This is just a hack commit to see if it fixes the CI, will update to a proper fix if it works.

[finagolfin]

@spevans, let's see if this new commit fixes the problem.

[millenomi]

@swift-ci please test

[millenomi]

To note to find a general solution instead.

[finagolfin]

@millenomi, looks like the CI didn't run.

[finagolfin]

@spevans, could you trigger the linux CI once more?

[spevans]

@swift-ci please test

[finagolfin]

Hard-coding the BUILD_RPATH for libFoundation.so didn't work, as it built Swift at a different path on the CI this time, /home/buildnode/jenkins/workspace/swift-corelibs-foundation-PR-Linux@2/buildbot_linux/swift-linux-x86_64/, so I copy-pasted the CMake config to extract that path from the compiler in a new commit. If this works, I will move it to the main CMakeLists.txt, so it's only run once.

@spevans, let's see if this works now.

[spevans]

@swift-ci please test

[finagolfin]

@spevans, the CI got stuck? Only linux needs to be checked.

[spevans]

@swift-ci test linux

[finagolfin]

Alright, this is like shooting in the dark, I have no idea what's going wrong. I will spin up an Ubuntu 16.04 VPS and see if I can reproduce the problem myself.

[finagolfin]

Finally reproduced these strange failures by building on Ubuntu 16.04 myself, turns out it's related to the CI building and then using CMake 3.16, whereas I was testing locally using CMake 3.17. Apparently, there's a bug where BUILD_RPATH can't be set for executables using CMake 3.16, 3ea08fe, so I simply disabled that additional CMake config for plutil for now. Alternately, I could remove that part altogether and submit a pull to add it later, rather than a pull to enable it then, just let me know what's preferred.

@spevans, this should work now, though plutil will still have the extraneous rpath for the time being.

[spevans]

@swift-ci test

[spevans]

@buttaface Are you saying that a better fix is to upgrade to CMake 3.17? If so that might be an option.

[spevans]

@swift-ci please test

[finagolfin]

CMake 3.17 would be ideal, as it seems to fix various bugs, but given that would be applied across all the Swift repos, I wouldn't presume to push for it. I was just planning on waiting for it, and enabling that plutil CMake config whenever the CMake update happened.

[spevans]

It shouldn't be an issue to do an update, the whole reason that CMake is built as part of the build process was specifically to fix issues in the corelibs repos. Note its only updated on non-Apple platforms since the toolchain is still expected to build using older CMake on macOS (where the corelibs repos are not built).

@compnerd What do you think about upgrading to CMake 3.17? It seems to fix some RPATH issues which is better than working around them.

[compnerd]

Updating to 3.17 I don’t have problems with. It has additional improvements for swift as well. I think that it should be possible to provide compatibility shims though.

That said, the -no-stb-toolchain-rpath should be external to the build system, not hard coded into it. The CMAKE_Swift_FLAGS allow easy control over such flags.

[compnerd]

Externalize the RPATH handling

[finagolfin]

Externalize the RPATH handling

Why? By this, I presume that you mean that it should be passed to cmake on the command-line instead, like SwiftPM does with other Swift flags.

I think that would make sense if people sometimes need that default toolchain stdlib rpath. However, this pull and the others I've submitted shows that almost none of the core Swift repos need that rpath set to pass their tests. Once installed, of course that default rpath should be removed. So I see no scenario under which we would want to keep it, so I think this "hard coded" approach is best.

Perhaps I'm missing something, so let me know if I have.

[spevans]

I agree with @buttaface, I don't think there needs to be an external option for this, the absolute path to the build directory should never be added. The tests use LD_LIBRARY_PATH to run correctly and installing libraries with a hard coded path does present a vulnerability, as a library could be added into a /home/buildnode/jenkins/workspace/... directory and would be used in preference to the required one.

[compnerd]

Id say that if you want to change the default, change the default in the driver so that -no-toolchain-stdlib-rpath is the default for non-Apple platforms. Embedding the flag on all the repositories by default doesn't solve the problem.

[finagolfin]

Embedding the flag on all the repositories by default doesn't solve the problem.

I actually think this is the best approach: the Swift compiler adds the toolchain stdlib rpath by default for user libraries, but remove it by default for these corelibs. As SR-1967 lays out, the default setting of the stdlib rpath is very useful for the average user, whereas the flags added a year and a half ago to customize it are useful for the more advanced user, swiftlang/swift@383a885.

In other words, it should be the default for the stdlib rpath to be added for the vast majority of Swift libraries that are built and used locally, but it should be the default for the few libraries like these that are distributed in binary form to not have the host toolchain's stdlib rpath.

[compnerd]

In other words, it should be the default for the stdlib rpath to be added for the vast majority of Swift libraries that are built and used locally

This should be assumed to be the default mode of compilation for all targets in the CMake. That is what I am also suggesting. That is, the rpath should always be added.

distributed in binary form to not have the host toolchain's stdlib rpath

Since the default is the local builds, this should be treated as the advanced case, and we have the ability to grant the control to the user for this: -D CMAKE_Swift_FLAGS="-no-stdlib-rpath" when configuring the project will do what you need.

I could see the argument for not providing the toolchain rpath by default on non-Apple platforms because they are non-stable ABIs and not normally distributed on the system. In such a case, if we want the default to be that the toolchain RPATH is not present by default, then the right place to do this is in the driver.

The result of that is that it is best to not propagate this flag into every single individual project. You can pass this to the project when building it if you intend to distribute it yourself.

[finagolfin]

Since the default is the local builds, this should be treated as the advanced case, and we have the ability to grant the control to the user for this: -D CMAKE_Swift_FLAGS="-no-stdlib-rpath" when configuring the project will do what you need.

Not quite, I believe it will apply that flag to all Swift targets indiscriminately, and plutil and other Swift executables will need to add that rpath back in again, with the workaround you used for XDGTestHelper, 3ea08fe. So passing that flag into this repo doesn't work as well as this fine-grained approach.

I could see the argument for not providing the toolchain rpath by default on non-Apple platforms because they are non-stable ABIs and not normally distributed on the system.

No, this has nothing to do with the ABI or being "normally distributed on the system." The reason to remove it is simply what I said earlier, because these corelibs are almost always distributed and used in binary form and it makes no sense to keep the host toolchain's stdlib from where the library happened to be built in that distributed library's rpath, see my OP and Simon's earlier point about security.

if we want the default to be that the toolchain RPATH is not present by default, then the right place to do this is in the driver.

No, nobody is talking about that, I already said that should add the rpath. We're only talking about how to configure the default rpath for these corelibs.

it is best to not propagate this flag into every single individual project.

You've yet to make a case for this when it comes to these corelibs, what precisely are you worried about by removing the toolchain stdlib rpath by default? I'll turn it back on you and note that if you happen to need that rpath in some scenario, you can always add it back with your favored CMAKE_Swift_FLAGS. In fact, for most usage of ELF shared libraries that are linked into a Swift executable, the executable's toolchain stdlib rpath is applied transitively to the shared library too, so you don't even need the stdlib rpath embedded in "the vast majority of Swift libraries that are built and used locally" that I mentioned earlier.

I've shown with the CI passing on all my recent no-stdlib-rpath pulls to these Swift library repos that the stdlib rpath is unneeded. Choosing a default is a matter of judgement, and I don't see a need to pass this flag from the command-line instead. I could be wrong, but you have been unable to give a concrete scenario where that even might be the case.

[compnerd]

plutil and other Swift executables will need to add that rpath back in again

Why do they need to restore the RPATH? What is different about libraries and executables? The RPATH should apply equally to all the libraries and the executables.

corelibs are almost always distributed and used in binary form

Everything is distributed in binary form. There is nothing special about the corelibs in that sense.

it makes no sense to keep the host toolchain's stdlib

This only makes sense when cross-compiling. That is not the default - thus, what you are proposing should be an opt-in.

We're only talking about how to configure the default rpath for these corelibs.

Yes, the entire discussion has been about defaults. If you go and read my responses, they are all taking about the defaults.

what precisely are you worried about by removing the toolchain stdlib rpath by default

Im not worried about the removal, Im worried about how you are removing them. Im saying that you should be removing them by not having the driver inject them in the first place.

I don't see a need to pass this flag from the command-line instead.

Because the defaults of all the tools are for native compilation, not cross-compilation, and the RPATHs being added are valid on the host that the binaries are being built for, the flag should be specified by the user. If they are not valid by default then they shouldn't be injected in the first place, and thus the proper place to address the issue where the injection occurs - the driver.

[finagolfin]

Note that all of the below is for most ELF platforms, with this pull and other similar ones I've submitted only applying for ELF libraries and executables.

Why do they need to restore the RPATH? What is different about libraries and executables? The RPATH should apply equally to all the libraries and the executables.

It's simple, I already explained it above. plutil, an executable, won't work without the rpath to all the libraries it directly links against. Whereas libFoundation.so will usually work fine if the toolchain stdlib path is missing from its runpath, because that stdlib path from plutil or another Swift executable that links against Foundation is transitively applied to libFoundation.so too. It's probably why CMake has all of CMAKE_{SHARED,STATIC,EXE,MODULE}_LINKER_FLAGS, to handle all the differences.

Everything is distributed in binary form. There is nothing special about the corelibs in that sense.

No, SwiftPM packages on ELF platforms don't even support binary targets yet, with macOS support just added a couple months ago. Whereas these corelibs are almost never going to be built from source, almost everyone is only going to use them pre-compiled as part of a toolchain. How a project is commonly used should determine its defaults.

it makes no sense to keep the host toolchain's stdlib

This only makes sense when cross-compiling. That is not the default - thus, what you are proposing should be an opt-in.

Ah, this may be where the confusion lies, nothing I'm talking about has anything to do with cross-compiling. When I refer to the "host toolchain's stdlib," I'm talking about the way most people use Foundation on ELF, ie Apple compiles it on an Ubuntu x86_64 server with the "host toolchain" at /home/buildnode/jenkins/workspace/, and then I download that toolchain and run it on a linux x86_64 server where no such path exists, yet that rpath is now plastered across all the Swift libraries. Seriously, read my OP, I say exactly why I submitted these pulls.

Yes, the entire discussion has been about defaults. If you go and read my responses, they are all taking about the defaults.
Im saying that you should be removing them by not having the driver inject them in the first place.

Maybe there's some confusion here: what do you mean "by not having the driver inject them?" I take that to mean a certain place, but perhaps you mean someplace completely different.

the RPATHs being added are valid on the host that the binaries are being built for, the flag should be specified by the user

They are "valid on the host," but as my pulls show they are not needed on the host, and as I've explained, should definitely be removed before installation and distribution. As for specifying it using your preferred flag passed into CMake, I've already explained how that doesn't work because of how Swift executables work on ELF platforms.

[finagolfin]

Let's see if we can move forward on this, after three months of setting it aside.

The entire aim of this pull is to remove the extraneous RPATH of /home/build-user/build/buildbot_linux/swift-linux-x86_64/lib/swift/linux that is currently applied to libFoundation.so from the official 5.2.5 release for Ubuntu, and will soon be there for the final Swift 5.3 release too, which @spevans has correctly noted is a security vulnerability (I believe this pull should be backported to the 5.3 branch once merged). This pull has nothing to do with cross-compilation or my own personal use, as has been suggested above.

Saleem seems to agree that that RPATH should be removed, but simply disagrees with my implementation of using target_link_options within this project to do it:

Im not worried about the removal, Im worried about how you are removing them.

He proposes two alternative implementations instead:

1. Get the Swift compiler driver itself to stop adding this RPATH to all shared libraries on ELF platforms. While this would probably work for most use of shared libraries, SR-1967 that I linked above notes that the current default of adding it was chosen instead for user convenience. While this default RPATH could be removed for shared libraries that are added to a Swift executable's RPATH when it's compiled, it could not be removed for dynamically loaded shared libraries that are expected to have their own dependencies' RPATHs built in, so I don't believe changing the compiler's default is a workable solution.
2. Pass the -no-toolchain-stdlib-rpath flag in with CMAKE_Swift_FLAGS when building Foundation instead, presumably by opening a PR to the Swift compiler repo to add it in the right place in utils/build-script-impl instead. While this would work to remove the RPATH from the shared libraries, it would also remove it from executables like plutil in the build directory, breaking their use as explained above. That's because CMAKE_Swift_FLAGS is applied indiscriminately to all Swift shared libraries and executables in this repo.

Given these problems with other approaches, I chose this target_link_options approach as a fine-grained way to remove the extraneous RPATHs, but I'm open to an alternate approach.

I've summarized the discussion so far in this comment, I ask that @spevans and @compnerd choose one implementation approach, either one of the three mentioned so far or possibly some new approach or combo altogether, so we can move forward with removing this security vulnerability.

[finagolfin]

@drexin, you deal with a lot of linux issues, could you chime in on this pull? I know there's been a long discussion here, but I believe my last comment accurately summarizes it for you.

[drexin]

@buttaface Sorry for the delay. I talked to @gottesmm and looks like he already talked to you and suggested a way forward. Happy to reviews those changes.

[finagolfin]

@drexin, no, @gottesmm and I never talked about a solution for this pull, I think he got confused with another pull. Could you give your opinion on the right approach from my summary comment above, given you seem to deal with the linux port a lot and my comment talks a lot about how the ELF RPATH works?

[millenomi]

@swift-ci please test and merge

[finagolfin]

Submitted for the 5.3 branch too, #2865.

[finagolfin]

Looks like the CI command didn't go through.

[drexin]

@swift-ci test

[drexin]

Hm, not sure why the bot isn't working. /cc @shahmishal

[shahmishal]

@swift-ci test

[finagolfin]

Passes CI, ready to merge.