Skip to content
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

Redirect DllImport of hostpolicy to the main executable when it's embedded #40014

Merged
merged 3 commits into from
Jul 29, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions src/coreclr/src/dlls/mscoree/unixinterface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ typedef NewArrayHolder<const WCHAR> ConstWStringHolder;
// Specifies whether coreclr is embedded or standalone
extern bool g_coreclr_embedded;

// Specifies whether hostpolicy is embedded in executable or standalone
extern bool g_hostpolicy_embedded;

// Holder for array of wide strings
class ConstWStringArrayHolder : public NewArrayHolder<LPCWSTR>
{
Expand Down Expand Up @@ -116,7 +119,8 @@ static void ConvertConfigPropertiesToUnicode(
int propertyCount,
LPCWSTR** propertyKeysWRef,
LPCWSTR** propertyValuesWRef,
BundleProbe** bundleProbe)
BundleProbe** bundleProbe,
bool* hostPolicyEmbedded)
{
LPCWSTR* propertyKeysW = new (nothrow) LPCWSTR[propertyCount];
ASSERTE_ALL_BUILDS(propertyKeysW != nullptr);
Expand All @@ -135,6 +139,11 @@ static void ConvertConfigPropertiesToUnicode(
// is passed in as the value of "BUNDLE_PROBE" property (encoded as a string).
*bundleProbe = (BundleProbe*)_wcstoui64(propertyValuesW[propertyIndex], nullptr, 0);
}
else if (strcmp(propertyKeys[propertyIndex], "HOSTPOLICY_EMBEDDED") == 0)
{
// The HOSTPOLICY_EMBEDDED property indicates if the executable has hostpolicy statically linked in
*hostPolicyEmbedded = (wcscmp(propertyValuesW[propertyIndex], W("true")) == 0);
}
}

*propertyKeysWRef = propertyKeysW;
Expand Down Expand Up @@ -177,14 +186,16 @@ int coreclr_initialize(
LPCWSTR* propertyKeysW;
LPCWSTR* propertyValuesW;
BundleProbe* bundleProbe = nullptr;
bool hostPolicyEmbedded;

ConvertConfigPropertiesToUnicode(
propertyKeys,
propertyValues,
propertyCount,
&propertyKeysW,
&propertyValuesW,
&bundleProbe);
&bundleProbe,
&hostPolicyEmbedded);

#ifdef TARGET_UNIX
DWORD error = PAL_InitializeCoreCLR(exePath, g_coreclr_embedded);
Expand All @@ -198,6 +209,8 @@ int coreclr_initialize(
}
#endif

g_hostpolicy_embedded = hostPolicyEmbedded;

ReleaseHolder<ICLRRuntimeHost4> host;

hr = CorHost2::CreateObject(IID_ICLRRuntimeHost4, (void**)&host);
Expand Down
4 changes: 4 additions & 0 deletions src/coreclr/src/vm/ceemain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -240,10 +240,14 @@ extern "C" HRESULT __cdecl CorDBGetInterface(DebugInterface** rcInterface);
#endif // !CROSSGEN_COMPILE

// g_coreclr_embedded indicates that coreclr is linked directly into the program
// g_hostpolicy_embedded indicates that the hostpolicy library is linked directly into the executable
// Note: that it can happen that the hostpolicy is embedded but coreclr isn't (on Windows singlefilehost is built that way)
#ifdef CORECLR_EMBEDDED
bool g_coreclr_embedded = true;
bool g_hostpolicy_embedded = true; // We always embed hostpolicy if coreclr is also embedded
#else
bool g_coreclr_embedded = false;
bool g_hostpolicy_embedded = false; // In this case the value may come from a runtime property and may change
#endif

// Remember how the last startup of EE went.
Expand Down
16 changes: 16 additions & 0 deletions src/coreclr/src/vm/dllimport.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ using namespace clr::fs;
// Specifies whether coreclr is embedded or standalone
extern bool g_coreclr_embedded;

// Specifies whether hostpolicy is embedded in executable or standalone
extern bool g_hostpolicy_embedded;

// remove when we get an updated SDK
#define LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR 0x00000100
#define LOAD_LIBRARY_SEARCH_DEFAULT_DIRS 0x00001000
Expand Down Expand Up @@ -6328,6 +6331,19 @@ namespace
}
#endif

if (g_hostpolicy_embedded)
{
#if defined(TARGET_LINUX)
Copy link
Member

Choose a reason for hiding this comment

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

Would it be a good idea to invert this condition to make this logic future proof; such that all OS except Windows get libhostpolicy?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think everything but Windows should use libhostpolicy

Copy link
Member

Choose a reason for hiding this comment

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

Is this actually the name used in this context? - not libhostpolicy.so or something with a path?

In the case above we could, in theory, see different file names, but we intentionally only recognize the form used in Interop.Libraries.cs.
Jut wonder whether there is anything to worry about hostpolicy naming.

Copy link
Member Author

Choose a reason for hiding this comment

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

The story is the same - we don't expect any other code but corelib to call into hostpolicy directly, so these strings need to match what's in Interop.Libraries, but nothing else.

I fixed the condition to be for Windows - should also fix the build failures in CI

if (wcscmp(wszLibName, W("libhostpolicy")) == 0) {
return PAL_LoadLibraryDirect(NULL);
}
#else
if (wcscmp(wszLibName, W("hostpolicy.dll")) == 0) {
return WszGetModuleHandle(NULL);
}
#endif
}

AppDomain* pDomain = GetAppDomain();
DWORD loadWithAlteredPathFlags = GetLoadWithAlteredSearchPathFlag();
bool libNameIsRelativePath = Path::IsRelative(wszLibName);
Expand Down
3 changes: 2 additions & 1 deletion src/installer/corehost/cli/hostpolicy/coreclr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,8 @@ namespace
_X("APP_PATHS"),
_X("APP_NI_PATHS"),
_X("RUNTIME_IDENTIFIER"),
_X("BUNDLE_PROBE")
_X("BUNDLE_PROBE"),
_X("HOSTPOLICY_EMBEDDED")
};

static_assert((sizeof(PropertyNameMapping) / sizeof(*PropertyNameMapping)) == static_cast<size_t>(common_property::Last), "Invalid property count");
Expand Down
1 change: 1 addition & 0 deletions src/installer/corehost/cli/hostpolicy/coreclr.h
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ enum class common_property
AppNIPaths,
RuntimeIdentifier,
BundleProbe,
HostPolicyEmbedded,
// Sentinel value - new values should be defined above
Last
};
Expand Down
14 changes: 13 additions & 1 deletion src/installer/corehost/cli/hostpolicy/hostpolicy_context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -231,8 +231,20 @@ int hostpolicy_context_t::initialize(hostpolicy_init_t &hostpolicy_init, const a
pal::stringstream_t ptr_stream;
ptr_stream << "0x" << std::hex << (size_t)(&bundle_probe);

coreclr_properties.add(common_property::BundleProbe, ptr_stream.str().c_str());
if (!coreclr_properties.add(common_property::BundleProbe, ptr_stream.str().c_str()))
{
log_duplicate_property_error(coreclr_property_bag_t::common_property_to_string(common_property::StartUpHooks));
return StatusCode::LibHostDuplicateProperty;
}
}

#if defined(HOSTPOLICY_EMBEDDED)
if (!coreclr_properties.add(common_property::HostPolicyEmbedded, _X("true")))
{
log_duplicate_property_error(coreclr_property_bag_t::common_property_to_string(common_property::StartUpHooks));
return StatusCode::LibHostDuplicateProperty;
}
#endif

return StatusCode::Success;
}
2 changes: 2 additions & 0 deletions src/installer/corehost/cli/hostpolicy/static/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,5 @@ set(HEADERS
set(SKIP_VERSIONING 1)
set(BUILD_OBJECT_LIBRARY 1)
include(../../lib_static.cmake)

add_definitions(-DHOSTPOLICY_EMBEDDED)