Simplified muxer fix #99576
Simplified muxer fix #99576
Conversation
realpath is guaranteed to resolve symlinks. fullpath may resolve symlinks. Moves all existing code to call fullpath, which is the existing contract. On Unix, fullpath calls realpath, meaning it resolves symlinks. On Windows it doesn't. Code which requires symlinks to be resolved should be moved to call realpath. realpath is temporarily renamed realpath2 in this commit to find dangling references.
src/installer/tests/HostActivation.Tests/PortableAppActivation.cs
Outdated
Show resolved
Hide resolved
|
|
||
| if (file.get() == INVALID_HANDLE_VALUE) | ||
| { | ||
| // If we get "access denied" that may mean the path represents a directory. |
There was a problem hiding this comment.
Is this an intentional limitation? CreateFile should be able to get a directory handle with FILE_FLAG_BACKUP_SEMANTICS.
There was a problem hiding this comment.
To be honest I can't remember. There seem to be a lot of caveats in FILE_FLAG_BACKUP_SEMANTICS docs. Since we're going to fullpath in this case anyway, is there any point in proceeding?
Also, does Windows have directory symlinks?
There was a problem hiding this comment.
Windows does have directory symlinks - for example, via mklink /d.
The current uses of pal::realpath only operate on a file, so it is probably fine that this wouldn't resolve directory symlinks if we want to avoid any complexity that FILE_FLAG_BACKUP_SEMANTICS might bring. But in that case, maybe update the // Like fullpath, but resolves symlinks. comment to indicate that only resolving for files is known/intentional limitation of this function (on Windows). I could see hitting cases in the future where it'd be confusing that this doesn't handle directories (maybe dotnet root being a symlink or something like that).
There was a problem hiding this comment.
Added a comment. The file security implications of FILE_FLAG_BACKUP_SEMANTICS worry me a bit just because I don't understand all the consequences.
Co-authored-by: Elinor Fung <[email protected]>
|
@elinor-fung I think this is ready for re-review whenever you're free. No rush. |
| { | ||
| trace::error(_X("Error resolving full path [%s]"), path->c_str()); | ||
| trace::error(_X("GetLastError(): [%s]"), ::GetLastError()); | ||
| return false; |
There was a problem hiding this comment.
Should this be outside of the if block so that we still return false when not printing out errors? Otherwise, skip_error_logging changes the actual error behaviour, not just logging.
|
|
||
| if (file.get() == INVALID_HANDLE_VALUE) | ||
| { | ||
| // If we get "access denied" that may mean the path represents a directory. |
There was a problem hiding this comment.
Windows does have directory symlinks - for example, via mklink /d.
The current uses of pal::realpath only operate on a file, so it is probably fine that this wouldn't resolve directory symlinks if we want to avoid any complexity that FILE_FLAG_BACKUP_SEMANTICS might bring. But in that case, maybe update the // Like fullpath, but resolves symlinks. comment to indicate that only resolving for files is known/intentional limitation of this function (on Windows). I could see hitting cases in the future where it'd be confusing that this doesn't handle directories (maybe dotnet root being a symlink or something like that).
This is a simplified version of #87717 that tries to narrowly enable running a host behind a symlink.
Fixes #83314