Skip to content

Conversation

@sdmaclea
Copy link
Contributor

@sdmaclea sdmaclea commented Dec 28, 2020

For these 9 libraries, cmake generates a cmake_install.cmake file which includes the code below. (I don't really understand why this only affects 8 libraries).

if("x${CMAKE_INSTALL_COMPONENT}x" STREQUAL "xUnspecifiedx" OR NOT CMAKE_INSTALL_COMPONENT)
  file(INSTALL DESTINATION "${CMAKE_INSTALL_PREFIX}/corehost" TYPE SHARED_LIBRARY FILES "/Users/stmaclea/git/runtime/artifacts/obj/osx-arm64.Release/cli/hostpolicy/standalone/libhostpolicy.dylib")
  if(EXISTS "$ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/corehost/libhostpolicy.dylib" AND
     NOT IS_SYMLINK "$ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/corehost/libhostpolicy.dylib")
    execute_process(COMMAND "/usr/bin/install_name_tool"
      -id "libhostpolicy.dylib"
      "$ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/corehost/libhostpolicy.dylib")
    if(CMAKE_INSTALL_DO_STRIP)
      execute_process(COMMAND "/Applications/Xcode_12_beta_6.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/strip" -x "$ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/corehost/libhostpolicy.dylib")
    endif()
  endif()
endif()

This corrupts the adhoc signature automatically added by the XCode 12 linker.

This PR inserts code like the following after the above code to repair the signature.

if("x${CMAKE_INSTALL_COMPONENT}x" STREQUAL "xUnspecifiedx" OR NOT CMAKE_INSTALL_COMPONENT)
  execute_process(COMMAND /usr/bin/codesign -f -s - /Users/stmaclea/git/runtime/artifacts/bin/osx-arm64.Release/corehost/libhostpolicy.dylib)
endif()

This is at least a reasonable interim fix for #46369

I think install(CODE ...) or install(SCRIPT ...) is the right approach to fix this type of issue. I couldn't get generator expressions $<TARGET_FILE:target> to work, so I hardcoded the lib prefix and suffix. I think since this is OSX I suspect it should be OK.

Fixes #46369

@ghost
Copy link

ghost commented Dec 28, 2020

Tagging subscribers to this area: @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

For these 8 libraries, cmake generates a cmake_install.cmake file which includes the code below. (I don't really understand why this only affects 8 libraries).

if("x${CMAKE_INSTALL_COMPONENT}x" STREQUAL "xUnspecifiedx" OR NOT CMAKE_INSTALL_COMPONENT)
  file(INSTALL DESTINATION "${CMAKE_INSTALL_PREFIX}/corehost" TYPE SHARED_LIBRARY FILES "/Users/stmaclea/git/runtime/artifacts/obj/osx-arm64.Release/cli/hostpolicy/standalone/libhostpolicy.dylib")
  if(EXISTS "$ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/corehost/libhostpolicy.dylib" AND
     NOT IS_SYMLINK "$ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/corehost/libhostpolicy.dylib")
    execute_process(COMMAND "/usr/bin/install_name_tool"
      -id "libhostpolicy.dylib"
      "$ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/corehost/libhostpolicy.dylib")
    if(CMAKE_INSTALL_DO_STRIP)
      execute_process(COMMAND "/Applications/Xcode_12_beta_6.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/strip" -x "$ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/corehost/libhostpolicy.dylib")
    endif()
  endif()
endif()

This corrupts the adhoc signature automatically added by the XCode 12 linker.

This PR inserts code like the following after the above code to repair the signature.

if("x${CMAKE_INSTALL_COMPONENT}x" STREQUAL "xUnspecifiedx" OR NOT CMAKE_INSTALL_COMPONENT)
  execute_process(COMMAND /usr/bin/codesign -f -s - /Users/stmaclea/git/runtime/artifacts/bin/osx-arm64.Release/corehost/libhostpolicy.dylib)
endif()

This is at least a reasonable interim fix for #46369

I think install(CODE ...) or install(SCRIPT ...) is the right approach to fix this type of issue. I couldn't get generator expressions $<TARGET_FILE:target> to work, so I hardcoded the lib prefix and suffix. I think since this is OSX I suspect it should be OK.

Fixes #46369

Author: sdmaclea
Assignees: sdmaclea
Labels:

arch-arm64, area-Infrastructure, os-mac-os-x-big-sur

Milestone: 6.0.0

@janvorli
Copy link
Member

Let me take a quick look at why the CMAKE_INSTALL_DO_STRIP is set. I would prefer fixing the cause instead of the symptoms.
You mention 8 libs in this PR, but the issue lists just three libs built by the installer part of this repo. So I wonder if there are others or if the 8 was a typo.

@sdmaclea
Copy link
Contributor Author

8 was a miscount. There are actually 9 libraries with corrupt signatures. Many of them are for testing only. There are three libmock*.dylibs and three libsuperpmi-shim-*.dylibs which are also corrupted.

Most libraries/programs have the strip code in their cmake_install.cmake. I was guessing the issue was the /usr/bin/install_name_tool -id <target_id> command.

@janvorli
Copy link
Member

You may be right and the issue is caused by the install_name_tool. There is a comment in cmake sources that explains that this tool is invoked in case the build tree and install tree use different path components of the install_name field:
https://github.com/Kitware/CMake/blob/63aa279343df34d34e0a0fd0c92a444e2528bb8a/Source/cmInstallTargetGenerator.cxx#L556-L558.

Looking at one of the libraries, it gets built in
artifacts/obj/osx-x64.Release/cli/nethost/libnethost.dylib
but installed in
artifacts/bin/osx-x64.Release/corehost/libnethost.dylib
So it seems it could really be the culprit.

However, the strange thing is that on x64 macos, the binaries signatures are not corrupted. So maybe the issue is a bug in the version of the install_name_tool present on the Apple Silicon device. I have verified that crossbuilding for arm64 on the same x64 mac gets the corrupted signatures while the x64 build is correct.

Investigating that further on, it seems to me that the signatures are broken for libraries that are installed using install_with_stripped_symbols(...TARGETS... while the ones installed install_with_stripped_symbols(...PROGRAMS... are not affected.

Invocations of install_clr in the coreclr work fine, because the install_clr internally uses install(PROGRAMS.... And libraries use install(PROGRAMS... too.

Reading the cmake install command doc seems to explain why there is a difference. Install PROGRAMS is meant to install executable things that are not targets, so these would be scripts or externally produced executables. I think that's why cmake doesn't try to fix the install name for these.

So it looks like binaries that are not affected are in fact installed in an "incorrect" way. However, we use the install in a way that's different from the intended usage. The intended usage is to install the build result to the final location (e.g. /usr/local/bin etc). But we (mis)use it to install it to our "bin" subdirectories where the path doesn't correspond to the final path on the device. I guess that may be the reason we use install(PROGRAMS... in most of the cases, as we don't want the extra massaging that cmake might add.

It seems to me that we should fix the issue by using install_with_stripped_symbols(...PROGRAMS...) at all places where we now use install_with_stripped_symbols(...TARGETS... to behave the same way for all parts of the runtime. I don't see why installer should use a different way.
I've tried to apply this to the libhostfxr.dylib and it worked as expected.

@janvorli
Copy link
Member

@sdmaclea Since the alternative way didn't work for the libraries from the installer repo, let's use your way. But I would like to make the change more localized. I'd change the superpmi stuff handling the way of my now closed PR and call the install_adhoc_codesign from install_with_stripped_symbols when it is called with TARGETS (or inline it into the install_with_stripped_symbols, whatever you prefer).

@sdmaclea
Copy link
Contributor Author

sdmaclea commented Dec 29, 2020

I couldn't get generator expressions to work so it was difficult to generalize my approach. Therefore this took a form closer to @janvorli 's proposal this morning.

@sdmaclea
Copy link
Contributor Author

@janvorli PTAL

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@janvorli janvorli merged commit 51f59f5 into dotnet:master Dec 30, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Jan 29, 2021
@sdmaclea sdmaclea deleted the codesign branch June 10, 2021 00:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Apple Silicon official runtime build unsigned binaries

3 participants