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

Fix muxer handling of symlinks on Windows #87717

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
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
25 changes: 25 additions & 0 deletions src/installer/tests/HostActivation.Tests/PortableAppActivation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,31 @@ public PortableAppActivation(SharedTestState fixture)
sharedTestState = fixture;
}

[Fact]
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we would add tests for all use cases of realpath in the code.

public void Muxer_behind_symlink()
{
var fixture = sharedTestState.PortableAppFixture_Built
.Copy();

var dotnetLoc = fixture.BuiltDotnet.DotnetExecutablePath;
var appDll = fixture.TestProject.AppDll;
var testDir = Directory.GetParent(fixture.TestProject.Location).ToString();

var exeSuffix = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? ".exe" : "";

using var symlink = new SymLink(Path.Combine(testDir, "dn" + exeSuffix), dotnetLoc);

var cmd = Command.Create(symlink.SrcPath, new[] { appDll })
.CaptureStdOut()
.CaptureStdErr()
.EnvironmentVariable("DOTNET_SKIP_FIRST_TIME_EXPERIENCE", "1")
.MultilevelLookup(false); // Avoid looking at machine state by default

cmd.Execute()
.Should().Pass()
.And.HaveStdOutContaining("Hello World");
}

[Fact]
public void Muxer_activation_of_Build_Output_Portable_DLL_with_DepsJson_and_RuntimeConfig_Local_Succeeds()
{
Expand Down
3 changes: 2 additions & 1 deletion src/native/corehost/corehost.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ int exe_start(const int argc, const pal::char_t* argv[])
pal::initialize_createdump();

pal::string_t host_path;
// Use realpath to find the host path through symlinks
if (!pal::get_own_executable_path(&host_path) || !pal::realpath(&host_path))
{
trace::error(_X("Failed to resolve full path of the current executable [%s]"), host_path.c_str());
Expand Down Expand Up @@ -139,7 +140,7 @@ int exe_start(const int argc, const pal::char_t* argv[])
{
trace::info(_X("Detected Single-File app bundle"));
}
else if (!pal::realpath(&app_path))
else if (!pal::realpath(&app_path)) // Use realpath to find the app path through symlinks
Copy link
Member

Choose a reason for hiding this comment

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

To properly test this we would need to setup two symlinks. The executable can be a symlink, we should look for the app.dll next to the resolved path (after resolving the symlink in host_path) and then the app.dll could also be a symlink, which we will resolve here.

It's an interesting question if we should actually behave like this. We will use the app_path for everything else (.deps.json, .runtimeconfig.json, app base path, ...).
I guess we want to keep working this way since we've done it forever on Linux - and so making Windows match Linux in this case makes a lot of sense (since previous to this change Windows symlink support was messy).

{
trace::error(_X("The application to execute does not exist: '%s'."), app_path.c_str());
return StatusCode::LibHostAppRootFindFailure;
Expand Down
2 changes: 2 additions & 0 deletions src/native/corehost/fxr/command_line.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ namespace
if (mode == host_mode_t::apphost)
{
app_candidate = host_info.app_path;
// Use realpath since we want the path to the app through symlinks
doesAppExist = bundle::info_t::is_single_file_bundle() || pal::realpath(&app_candidate);
Comment on lines 147 to 149
Copy link
Member

Choose a reason for hiding this comment

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

Maybe not in this PR, but it would be worth looking into this a bit higher level. For example in this case, the host_info.app_path might already be after symlink resolution (in fact it typically will be, since if you run apphost.exe we will populate this as resolved via the code in corehost which also uses realpath).

But there are other places which initialize the host_info.app_path to paths which are unresolved yet.

Would be nice to have a principle that it either is guaranteed resolved, or not (in which case the callers don't try to resolve at all).

}
else
Expand All @@ -169,6 +170,7 @@ namespace
}
}

// Use realpath since we want the path to the app through symlinks
doesAppExist = pal::realpath(&app_candidate);
if (!doesAppExist)
{
Expand Down
11 changes: 7 additions & 4 deletions src/native/corehost/fxr/fx_muxer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,8 @@ void append_probe_realpath(const pal::string_t& path, std::vector<pal::string_t>
{
pal::string_t probe_path = path;

if (pal::realpath(&probe_path, true))
// Use fullpath since we don't care about symlinks
if (pal::fullpath(&probe_path, true))
{
realpaths->push_back(probe_path);
}
Expand All @@ -249,7 +250,8 @@ void append_probe_realpath(const pal::string_t& path, std::vector<pal::string_t>
segment.append(tfm);
probe_path.replace(pos_placeholder, placeholder.length(), segment);

if (pal::realpath(&probe_path, true))
// Use fullpath since we don't care about symlinks
if (pal::fullpath(&probe_path, true))
{
realpaths->push_back(probe_path);
}
Expand All @@ -274,7 +276,8 @@ namespace
const runtime_config_t::settings_t& override_settings)
{
// Check for the runtimeconfig.json file specified at the command line
if (!runtime_config.empty() && !pal::realpath(&runtime_config))
// Use fullpath since we don't care about symlinks
if (!runtime_config.empty() && !pal::fullpath(&runtime_config))
{
trace::error(_X("The specified runtimeconfig.json [%s] does not exist"), runtime_config.c_str());
return StatusCode::InvalidConfigFile;
Expand Down Expand Up @@ -377,7 +380,7 @@ namespace

// This check is for --depsfile option, which must be an actual file.
pal::string_t deps_file = command_line::get_option_value(opts, known_options::deps_file, _X(""));
if (!deps_file.empty() && !pal::realpath(&deps_file))
if (!deps_file.empty() && !pal::fullpath(&deps_file))
{
trace::error(_X("The specified deps.json [%s] does not exist"), deps_file.c_str());
return StatusCode::InvalidArgFailure;
Expand Down
4 changes: 3 additions & 1 deletion src/native/corehost/fxr/hostfxr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,7 @@ namespace

if (startup_info.host_path.empty())
{
// Use realpath to find the host_path behind symlinks
Copy link
Member

Choose a reason for hiding this comment

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

Should we also "realpath" the host_path if it is specified by the caller? This comes from custom host code (so external input).
I think for consistency it would make sense to follow symlinks everywhere, but it is technically a breaking change - and in hostfxr :-(

if (!pal::get_own_executable_path(&startup_info.host_path) || !pal::realpath(&startup_info.host_path))
{
trace::error(_X("Failed to resolve full path of the current host [%s]"), startup_info.host_path.c_str());
Expand All @@ -559,7 +560,8 @@ namespace
return StatusCode::CoreHostCurHostFindFailure;

startup_info.dotnet_root = get_dotnet_root_from_fxr_path(mod_path);
if (!pal::realpath(&startup_info.dotnet_root))
// We don't support directory symlinks, so use fullpath
if (!pal::fullpath(&startup_info.dotnet_root))
{
trace::error(_X("Failed to resolve full path of dotnet root [%s]"), startup_info.dotnet_root.c_str());
return StatusCode::CoreHostCurHostFindFailure;
Expand Down
2 changes: 1 addition & 1 deletion src/native/corehost/fxr_resolver.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ int load_fxr_and_get_delegate(hostfxr_delegate_type type, THostPathToConfigCallb
pal::dll_t fxr;

pal::string_t host_path;
if (!pal::get_own_module_path(&host_path) || !pal::realpath(&host_path))
if (!pal::get_own_module_path(&host_path) || !pal::fullpath(&host_path))
{
trace::error(_X("Failed to resolve full path of the current host module [%s]"), host_path.c_str());
return StatusCode::CoreHostCurHostFindFailure;
Expand Down
4 changes: 3 additions & 1 deletion src/native/corehost/host_startup_info.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ bool get_path_from_argv(pal::string_t *path)
// the wrong location when filename ends up being found in %PATH% and not the current directory.
if (path->find(DIR_SEPARATOR) != pal::string_t::npos)
{
return pal::realpath(path);
// Use fullpath since we don't care if the final path is a symlink
return pal::fullpath(path);
}

return false;
Expand Down Expand Up @@ -86,6 +87,7 @@ const pal::string_t host_startup_info_t::get_app_name() const
}

// If argv[0] did not work, get the executable name
// Use realpath to get the host path through symlinks
if (host_path->empty() && (!pal::get_own_executable_path(host_path) || !pal::realpath(host_path)))
{
trace::error(_X("Failed to resolve full path of the current executable [%s]"), host_path->c_str());
Expand Down
1 change: 1 addition & 0 deletions src/native/corehost/hostmisc/pal.h
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,7 @@ namespace pal

bool touch_file(const string_t& path);
bool realpath(string_t* path, bool skip_error_logging = false);
bool fullpath(string_t* path, bool skip_error_logging = false);
Comment on lines 271 to +272
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment what is the difference between the two - the naming is Unix based, but Unix doesn't have fullpath, so it's unclear what that does.

bool file_exists(const string_t& path);
inline bool directory_exists(const string_t& path) { return file_exists(path); }
void readdir(const string_t& path, const string_t& pattern, std::vector<string_t>* list);
Expand Down
11 changes: 8 additions & 3 deletions src/native/corehost/hostmisc/pal.unix.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ bool pal::get_default_breadcrumb_store(string_t* recv)
{
recv->clear();
pal::string_t ext;
if (pal::getenv(_X("CORE_BREADCRUMBS"), &ext) && pal::realpath(&ext))
if (pal::getenv(_X("CORE_BREADCRUMBS"), &ext) && pal::fullpath(&ext))
{
// We should have the path in ext.
trace::info(_X("Realpath CORE_BREADCRUMBS [%s]"), ext.c_str());
Expand Down Expand Up @@ -302,7 +302,7 @@ bool pal::get_default_servicing_directory(string_t* recv)
{
recv->clear();
pal::string_t ext;
if (pal::getenv(_X("CORE_SERVICING"), &ext) && pal::realpath(&ext))
if (pal::getenv(_X("CORE_SERVICING"), &ext) && pal::fullpath(&ext))
{
// We should have the path in ext.
trace::info(_X("Realpath CORE_SERVICING [%s]"), ext.c_str());
Expand Down Expand Up @@ -333,7 +333,7 @@ bool pal::get_default_servicing_directory(string_t* recv)

bool is_read_write_able_directory(pal::string_t& dir)
{
return pal::realpath(&dir) &&
return pal::fullpath(&dir) &&
(access(dir.c_str(), R_OK | W_OK | X_OK) == 0);
}

Expand Down Expand Up @@ -945,6 +945,11 @@ bool pal::getenv(const pal::char_t* name, pal::string_t* recv)
return (recv->length() > 0);
}

bool pal::fullpath(pal::string_t* path, bool skip_error_logging)
{
return realpath(path, skip_error_logging);
}

bool pal::realpath(pal::string_t* path, bool skip_error_logging)
Comment on lines +948 to 953
Copy link
Member

Choose a reason for hiding this comment

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

I think this deserves a comment - especially since the implementation is "the other way round" from Windows.

{
auto resolved = ::realpath(path->c_str(), nullptr);
Expand Down
99 changes: 91 additions & 8 deletions src/native/corehost/hostmisc/pal.windows.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ bool pal::load_library(const string_t* in_path, dll_t* dll)

if (LongFile::IsPathNotFullyQualified(path))
{
if (!pal::realpath(&path))
if (!pal::fullpath(&path))
{
trace::error(_X("Failed to load the dll from [%s], HRESULT: 0x%X"), path.c_str(), HRESULT_FROM_WIN32(GetLastError()));
return false;
Expand Down Expand Up @@ -649,7 +649,7 @@ bool get_extraction_base_parent_directory(pal::string_t& directory)
assert(len < max_len);
directory.assign(temp_path);

return pal::realpath(&directory);
return pal::fullpath(&directory);
}

bool pal::get_default_bundle_extraction_base_dir(pal::string_t& extraction_dir)
Expand All @@ -663,7 +663,7 @@ bool pal::get_default_bundle_extraction_base_dir(pal::string_t& extraction_dir)
append_path(&extraction_dir, _X(".net"));
// Windows Temp-Path is already user-private.

if (realpath(&extraction_dir))
if (fullpath(&extraction_dir))
{
return true;
}
Expand All @@ -676,7 +676,7 @@ bool pal::get_default_bundle_extraction_base_dir(pal::string_t& extraction_dir)
return false;
}

return realpath(&extraction_dir);
return fullpath(&extraction_dir);
}

static bool wchar_convert_helper(DWORD code_page, const char* cstr, size_t len, pal::string_t* out)
Expand Down Expand Up @@ -728,8 +728,91 @@ bool pal::clr_palstring(const char* cstr, pal::string_t* out)
return wchar_convert_helper(CP_UTF8, cstr, ::strlen(cstr), out);
}

// Return if path is valid and file exists, return true and adjust path as appropriate.
bool pal::realpath(string_t* path, bool skip_error_logging)
typedef std::unique_ptr<std::remove_pointer<HANDLE>::type, decltype(&::CloseHandle)> SmartHandle;

// Like realpath, but resolves symlinks.
Copy link
Member

Choose a reason for hiding this comment

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

Should it be?

Suggested change
// Like realpath, but resolves symlinks.
// Like fullpath, but resolves symlinks.

bool pal::realpath(pal::string_t* path, bool skip_error_logging)
{
if (path->empty())
{
return false;
}

// Use CreateFileW + GetFinalPathNameByHandleW to resolve symlinks

SmartHandle file(
::CreateFileW(
path->c_str(),
0, // Querying only
FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
nullptr, // default security
OPEN_EXISTING, // existing file
FILE_ATTRIBUTE_NORMAL, // normal file
nullptr), // No attribute template
&::CloseHandle);

pal::char_t buf[MAX_PATH];
size_t size;

if (file.get() == INVALID_HANDLE_VALUE)
{
// If we get "access denied" that may mean the path represents a directory.
// Even if not, we can fall back to GetFullPathNameW, which doesn't require a HANDLE

auto error = ::GetLastError();
file.release();
if (ERROR_ACCESS_DENIED != error)
{
goto invalidPath;
}
}
else
{
size = ::GetFinalPathNameByHandleW(file.get(), buf, MAX_PATH, FILE_NAME_NORMALIZED);
// If size is 0, this call failed. Fall back to GetFullPathNameW, below
if (size != 0)
{
pal::string_t str;
if (size < MAX_PATH)
{
str.assign(buf);
}
else
{
str.resize(size, 0);
size = ::GetFinalPathNameByHandleW(file.get(), (LPWSTR)str.data(), static_cast<uint32_t>(size), FILE_NAME_NORMALIZED);
assert(size <= str.size());

if (size == 0)
{
goto invalidPath;
}
}

// Remove the \\?\ prefix, unless it is necessary or was already there
if (LongFile::IsExtended(str) && !LongFile::IsExtended(*path) &&
!LongFile::ShouldNormalize(str.substr(LongFile::ExtendedPrefix.size())))
{
str.erase(0, LongFile::ExtendedPrefix.size());
}
Comment on lines +792 to +797
Copy link
Member

Choose a reason for hiding this comment

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

Just an observation:
According to the docs the first check (LongFile::IsExtended(str)) will pretty much always return true, since the GetFinalPathNameByHandleW is supposed to return the \\?\ prefixed path. The second check that the original path is not extended is also going to be true almost always. So we will end up calling ShouldNormalize and very likely erase below as well.

Can we try to make this somehow cheaper? (or maybe it's not worth it) - this will likely allocate twice (substr and erase) which makes this method allocate at least 3 times (the last is the str.assign above).

Maybe I'm just too careful and it's not worth the complexity to optimize this.

Copy link
Member

Choose a reason for hiding this comment

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

Might be worth adding a comment that the Win32 API call will typically return \\?\ prefixed path.


*path = str;
return true;
}
}

// If the above fails, fall back to fullpath
return pal::fullpath(path, skip_error_logging);

invalidPath:
if (!skip_error_logging)
{
trace::error(_X("Error resolving full path [%s]"), path->c_str());
}
return false;
}

bool pal::fullpath(string_t* path, bool skip_error_logging)
{
if (path->empty())
{
Expand Down Expand Up @@ -804,7 +887,7 @@ bool pal::realpath(string_t* path, bool skip_error_logging)
bool pal::file_exists(const string_t& path)
{
string_t tmp(path);
return pal::realpath(&tmp, true);
return pal::fullpath(&tmp, true);
}

static void readdir(const pal::string_t& path, const pal::string_t& pattern, bool onlydirectories, std::vector<pal::string_t>* list)
Expand All @@ -816,7 +899,7 @@ static void readdir(const pal::string_t& path, const pal::string_t& pattern, boo

if (LongFile::ShouldNormalize(normalized_path))
{
if (!pal::realpath(&normalized_path))
if (!pal::fullpath(&normalized_path))
{
return;
}
Expand Down
2 changes: 1 addition & 1 deletion src/native/corehost/hostmisc/utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ bool get_file_path_from_env(const pal::char_t* env_key, pal::string_t* recv)
pal::string_t file_path;
if (pal::getenv(env_key, &file_path))
{
if (pal::realpath(&file_path))
if (pal::fullpath(&file_path))
{
recv->assign(file_path);
return true;
Expand Down
6 changes: 4 additions & 2 deletions src/native/corehost/hostpolicy/args.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -98,10 +98,12 @@ bool set_root_from_app(const pal::string_t& managed_application_path,
// for very unlikely case where the main app.dll was itself excluded from the app bundle.
// Note that unlike non-single-file we don't want to set the app_root to the location of the app.dll
// it needs to stay the location of the single-file bundle.
return pal::realpath(&args.managed_application);
// We use fullpath here instead of realpath since we don't care about the path post-symlink resolution.
return pal::fullpath(&args.managed_application);
}

if (pal::realpath(&args.managed_application))
// We use realpath here because we want the final path through symlinks for the app_root.
if (pal::fullpath(&args.managed_application))
Comment on lines +105 to +106
Copy link
Member

Choose a reason for hiding this comment

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

The comment and code disagree here.
I think we want realpath in code as well.

Copy link
Member

Choose a reason for hiding this comment

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

Apparently we're missing a test for this case.

{
args.app_root = get_directory(args.managed_application);
return true;
Expand Down
4 changes: 3 additions & 1 deletion src/native/corehost/hostpolicy/deps_format.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,9 @@ namespace

bool deps_file_exists(pal::string_t& deps_path)
{
if (bundle::info_t::config_t::probe(deps_path) || pal::realpath(&deps_path, /*skip_error_logging*/ true))
// We use fullpath because we don't care about the final path of the deps file, as it won't be used
// as a relative path to any other files.
if (bundle::info_t::config_t::probe(deps_path) || pal::fullpath(&deps_path, /*skip_error_logging*/ true))
return true;

trace::verbose(_X("Dependencies manifest does not exist at [%s]"), deps_path.c_str());
Expand Down
1 change: 1 addition & 0 deletions src/native/corehost/hostpolicy/deps_resolver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -766,6 +766,7 @@ bool deps_resolver_t::resolve_probe_dirs(
std::unordered_set<pal::string_t> items;

pal::string_t core_servicing = m_core_servicing;
// Use realpath to find the final core servicing path through symlinks
pal::realpath(&core_servicing, true);

// Filter out non-serviced assets so the paths can be added after servicing paths.
Expand Down
1 change: 1 addition & 0 deletions src/native/corehost/hostpolicy/hostpolicy_context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@ int hostpolicy_context_t::initialize(const hostpolicy_init_t &hostpolicy_init, c
}

clr_path = probe_paths.coreclr;
// Use realpath to find the clr_path through symlinks
if (clr_path.empty() || !pal::realpath(&clr_path))
{
// in a single-file case we may not need coreclr,
Expand Down
Loading