-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[CMake] fix runpath for ELF platforms #2783
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
Conversation
Sources/Tools/plutil/CMakeLists.txt
Outdated
# On ELF platforms, remove the absolute rpath to the host toolchain's stdlib, then add it | ||
# back temporarily as a BUILD_RPATH just for the tests. | ||
if(NOT CMAKE_SYSTEM_NAME MATCHES "Darwin|Windows") | ||
target_link_options(plutil PRIVATE "SHELL:-no-toolchain-stdlib-rpath") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@swift-ci test linux |
Something's missing, I'll build again locally and take a look. |
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 @spevans, could you run the linux CI again? |
@swift-ci test linux |
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 This is just a hack commit to see if it fixes the CI, will update to a proper fix if it works. |
@spevans, let's see if this new commit fixes the problem. |
@swift-ci please test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To note to find a general solution instead.
@millenomi, looks like the CI didn't run. |
@spevans, could you trigger the linux CI once more? |
@swift-ci please test |
Hard-coding the @spevans, let's see if this works now. |
@swift-ci please test |
@spevans, the CI got stuck? Only linux needs to be checked. |
@swift-ci test linux |
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. |
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 @spevans, this should work now, though |
@swift-ci test |
@buttaface Are you saying that a better fix is to upgrade to CMake 3.17? If so that might be an option. |
@swift-ci please test |
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 |
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 @compnerd What do you think about upgrading to CMake 3.17? It seems to fix some |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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 |
Id say that if you want to change the default, change the default in the driver so that |
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. |
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.
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: 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. |
Not quite, I believe it will apply that flag to all Swift targets indiscriminately, and
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.
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.
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 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. |
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.
Everything is distributed in binary form. There is nothing special about the corelibs in that sense.
This only makes sense when cross-compiling. That is not the default - thus, what you are proposing should be an opt-in.
Yes, the entire discussion has been about defaults. If you go and read my responses, they are all taking about the defaults.
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.
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. |
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.
It's simple, I already explained it above.
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.
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
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.
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. |
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 Saleem seems to agree that that RPATH should be removed, but simply disagrees with my implementation of using
He proposes two alternative implementations instead:
Given these problems with other approaches, I chose this 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. |
@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. |
@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. |
@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? |
Remove the absolute path to the host toolchain's stdlib from the three Foundation shared libraries.
@swift-ci please test and merge |
Submitted for the 5.3 branch too, #2865. |
Looks like the CI command didn't go through. |
@swift-ci test |
Hm, not sure why the bot isn't working. /cc @shahmishal |
@swift-ci test |
Passes CI, ready to merge. |
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: