Skip to content

Commit

Permalink
Use a "virtual CWD" for each terminal window (#15280)
Browse files Browse the repository at this point in the history
Before process model v3, each Terminal window was running in its own process, each with its own CWD. This allowed `startingDirectory: .` to work relative to the terminal's own CWD. However, now all windows share the same process, so there can only be one CWD. That's not great - folks who right-click "open in terminal", then "Use parent process directory" are only ever going to be able to use the CWD of the _first_ terminal opened.

This PR remedies this issue, with a theory we had for another issue. Essentially, we'll give each Terminal window a "virtual" CWD. The Terminal isn't actually in that CWD, the terminal is in `system32`. This also will prevent the Terminal from locking the directory it was originally opened in.

* Closes #5506
* There wasn't a 1.18 issue for "Use parent process directory is broken" yet, presumably selfhosters aren't using that feature
* Related to #14957

Many more notes on this topic in #4637 (comment)

> **Warning**
> ## Breaking change‼️

This will break a profile like

```json
{
    "commandline": "media-test.exe",
    "name": "Use CWD for media-test",
    "startingDirectory": "."
},
```

if the user right-clicks "open in terminal", then attempts to open that profile. There's some theoretical work we could do in a follow up to fix this, but I'm inclined to say that relative paths for `commandline`s were already dangerous and should have been avoided.

(cherry picked from commit 5c08a86)
Service-Card-Id: 89180224
Service-Version: 1.18
  • Loading branch information
zadjii-msft authored and DHowett committed May 12, 2023
1 parent e80661f commit 8d2447b
Show file tree
Hide file tree
Showing 12 changed files with 138 additions and 23 deletions.
2 changes: 1 addition & 1 deletion src/cascadia/Remoting/Peasant.idl
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace Microsoft.Terminal.Remoting
CommandlineArgs(String[] args, String cwd, UInt32 showWindowCommand);

String[] Commandline { get; set; };
String CurrentDirectory();
String CurrentDirectory { get; };
UInt32 ShowWindowCommand { get; };
};

Expand Down
32 changes: 15 additions & 17 deletions src/cascadia/TerminalApp/TerminalPage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -557,27 +557,28 @@ namespace winrt::TerminalApp::implementation
// Handle it on a subsequent pass of the UI thread.
co_await wil::resume_foreground(Dispatcher(), CoreDispatcherPriority::Normal);

// If the caller provided a CWD, switch to that directory, then switch
// If the caller provided a CWD, "switch" to that directory, then switch
// back once we're done. This looks weird though, because we have to set
// up the scope_exit _first_. We'll release the scope_exit if we don't
// actually need it.
auto originalCwd{ wil::GetCurrentDirectoryW<std::wstring>() };
auto restoreCwd = wil::scope_exit([&originalCwd]() {

auto originalVirtualCwd{ _WindowProperties.VirtualWorkingDirectory() };
auto restoreCwd = wil::scope_exit([&originalVirtualCwd, this]() {
// ignore errors, we'll just power on through. We'd rather do
// something rather than fail silently if the directory doesn't
// actually exist.
LOG_IF_WIN32_BOOL_FALSE(SetCurrentDirectory(originalCwd.c_str()));
_WindowProperties.VirtualWorkingDirectory(originalVirtualCwd);
});

if (cwd.empty())
{
// We didn't actually need to change the virtual CWD, so we don't
// need to restore it
restoreCwd.release();
}
else
{
// ignore errors, we'll just power on through. We'd rather do
// something rather than fail silently if the directory doesn't
// actually exist.
LOG_IF_WIN32_BOOL_FALSE(SetCurrentDirectory(cwd.c_str()));
_WindowProperties.VirtualWorkingDirectory(cwd);
}

if (auto page{ weakThis.get() })
Expand Down Expand Up @@ -1232,15 +1233,12 @@ namespace winrt::TerminalApp::implementation
// construction, because the connection might not spawn the child
// process until later, on another thread, after we've already
// restored the CWD to its original value.
auto newWorkingDirectory{ settings.StartingDirectory() };
if (newWorkingDirectory.size() == 0 || newWorkingDirectory.size() == 1 &&
!(newWorkingDirectory[0] == L'~' || newWorkingDirectory[0] == L'/'))
{ // We only want to resolve the new WD against the CWD if it doesn't look like a Linux path (see GH#592)
auto cwdString{ wil::GetCurrentDirectoryW<std::wstring>() };
std::filesystem::path cwd{ cwdString };
cwd /= settings.StartingDirectory().c_str();
newWorkingDirectory = winrt::hstring{ cwd.wstring() };
}
const auto currentVirtualDir{ _WindowProperties.VirtualWorkingDirectory() };
const auto cwdString{ std::wstring_view{ currentVirtualDir } };
const auto requestedStartingDir{ settings.StartingDirectory() };
auto newWorkingDirectory = winrt::hstring{
Utils::EvaluateStartingDirectory(cwdString, std::wstring_view{ requestedStartingDir })
};

auto conhostConn = TerminalConnection::ConptyConnection();
auto valueSet = TerminalConnection::ConptyConnection::CreateSettings(settings.Commandline(),
Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalApp/TerminalPage.idl
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ namespace TerminalApp
String WindowNameForDisplay { get; };
String WindowIdForDisplay { get; };

String VirtualWorkingDirectory { get; set; };

Boolean IsQuakeWindow();
};

Expand Down
6 changes: 5 additions & 1 deletion src/cascadia/TerminalApp/TerminalWindow.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1019,11 +1019,14 @@ namespace winrt::TerminalApp::implementation
// returned.
// Arguments:
// - args: an array of strings to process as a commandline. These args can contain spaces
// - cwd: The CWD that this window should treat as its own "virtual" CWD
// Return Value:
// - the result of the first command who's parsing returned a non-zero code,
// or 0. (see TerminalWindow::_ParseArgs)
int32_t TerminalWindow::SetStartupCommandline(array_view<const winrt::hstring> args)
int32_t TerminalWindow::SetStartupCommandline(array_view<const winrt::hstring> args, winrt::hstring cwd)
{
_WindowProperties->SetInitialCwd(std::move(cwd));

// This is called in AppHost::ctor(), before we've created the window
// (or called TerminalWindow::Initialize)
const auto result = _appArgs.ParseArgs(args);
Expand Down Expand Up @@ -1345,6 +1348,7 @@ namespace winrt::TerminalApp::implementation
CATCH_LOG();
}
}

uint64_t WindowProperties::WindowId() const noexcept
{
return _WindowId;
Expand Down
8 changes: 7 additions & 1 deletion src/cascadia/TerminalApp/TerminalWindow.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,14 @@ namespace winrt::TerminalApp::implementation
winrt::hstring WindowNameForDisplay() const noexcept;
bool IsQuakeWindow() const noexcept;

WINRT_OBSERVABLE_PROPERTY(winrt::hstring, VirtualWorkingDirectory, _PropertyChangedHandlers, L"");

WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler);

public:
// Used for setting the initial CWD, before we have XAML set up for property change notifications.
void SetInitialCwd(winrt::hstring cwd) { _VirtualWorkingDirectory = std::move(cwd); };

private:
winrt::hstring _WindowName{};
uint64_t _WindowId{ 0 };
Expand All @@ -71,7 +77,7 @@ namespace winrt::TerminalApp::implementation

bool HasCommandlineArguments() const noexcept;

int32_t SetStartupCommandline(array_view<const winrt::hstring> actions);
int32_t SetStartupCommandline(array_view<const winrt::hstring> actions, winrt::hstring cwd);
void SetStartupContent(const winrt::hstring& content, const Windows::Foundation::IReference<Windows::Foundation::Rect>& contentBounds);
int32_t ExecuteCommandline(array_view<const winrt::hstring> actions, const winrt::hstring& cwd);
void SetSettingsStartupArgs(const std::vector<winrt::Microsoft::Terminal::Settings::Model::ActionAndArgs>& actions);
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalApp/TerminalWindow.idl
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ namespace TerminalApp

Boolean HasCommandlineArguments();

Int32 SetStartupCommandline(String[] commands);
Int32 SetStartupCommandline(String[] commands, String cwd);
void SetStartupContent(String json, Windows.Foundation.IReference<Windows.Foundation.Rect> bounds);
Int32 ExecuteCommandline(String[] commands, String cwd);
String ParseCommandlineMessage { get; };
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/WindowsTerminal/AppHost.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ void AppHost::_HandleCommandlineArgs(const Remoting::WindowRequestedArgs& window
}
else if (args)
{
const auto result = _windowLogic.SetStartupCommandline(args.Commandline());
const auto result = _windowLogic.SetStartupCommandline(args.Commandline(), args.CurrentDirectory());
const auto message = _windowLogic.ParseCommandlineMessage();
if (!message.empty())
{
Expand Down
12 changes: 11 additions & 1 deletion src/cascadia/WindowsTerminal/WindowEmperor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,17 @@ bool WindowEmperor::HandleCommandlineArgs()
{
std::vector<winrt::hstring> args;
_buildArgsFromCommandline(args);
auto cwd{ wil::GetCurrentDirectoryW<std::wstring>() };
const auto cwd{ wil::GetCurrentDirectoryW<std::wstring>() };

{
// ALWAYS change the _real_ CWD of the Terminal to system32, so that we
// don't lock the directory we were spawned in.
std::wstring system32{};
if (SUCCEEDED_LOG(wil::GetSystemDirectoryW<std::wstring>(system32)))
{
LOG_IF_WIN32_BOOL_FALSE(SetCurrentDirectoryW(system32.c_str()));
}
}

// Get the requested initial state of the window from our startup info. For
// something like `start /min`, this will set the wShowWindow member to
Expand Down
3 changes: 3 additions & 0 deletions src/types/inc/utils.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -112,4 +112,7 @@ namespace Microsoft::Console::Utils
// testing easier.
std::wstring_view TrimPaste(std::wstring_view textView) noexcept;

// Same deal, but in TerminalPage::_evaluatePathForCwd
std::wstring EvaluateStartingDirectory(std::wstring_view cwd, std::wstring_view startingDirectory);

}
64 changes: 64 additions & 0 deletions src/types/ut_types/UtilsTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ class UtilsTests
TEST_METHOD(TestTrimTrailingWhitespace);
TEST_METHOD(TestDontTrimTrailingWhitespace);

TEST_METHOD(TestEvaluateStartingDirectory);

void _VerifyXTermColorResult(const std::wstring_view wstr, DWORD colorValue);
void _VerifyXTermColorInvalid(const std::wstring_view wstr);
};
Expand Down Expand Up @@ -546,3 +548,65 @@ void UtilsTests::TestDontTrimTrailingWhitespace()
// * trim when there's a tab followed by only whitespace
// * not trim then there's a tab in the middle, and the string ends in whitespace
}

void UtilsTests::TestEvaluateStartingDirectory()
{
// Continue on failures
const WEX::TestExecution::DisableVerifyExceptions disableExceptionsScope;

auto test = [](auto& expected, auto& cwd, auto& startingDir) {
VERIFY_ARE_EQUAL(expected, EvaluateStartingDirectory(cwd, startingDir));
};

// A NOTE: EvaluateStartingDirectory makes no attempt to cannonicalize the
// path. So if you do any sort of relative paths, it'll literally just
// append.

{
std::wstring cwd = L"C:\\Windows\\System32";

// Literally blank
test(L"C:\\Windows\\System32\\", cwd, L"");

// Absolute Windows path
test(L"C:\\Windows", cwd, L"C:\\Windows");
test(L"C:/Users/migrie", cwd, L"C:/Users/migrie");

// Relative Windows path
test(L"C:\\Windows\\System32\\.", cwd, L"."); // ?
test(L"C:\\Windows\\System32\\.\\System32", cwd, L".\\System32"); // ?
test(L"C:\\Windows\\System32\\./dev", cwd, L"./dev");

// WSL '~' path
test(L"~", cwd, L"~");
test(L"~/dev", cwd, L"~/dev");

// WSL or Windows / path - this will ultimately be evaluated by the connection
test(L"/", cwd, L"/");
test(L"/dev", cwd, L"/dev");
}

{
std::wstring cwd = L"C:/Users/migrie";

// Literally blank
test(L"C:/Users/migrie\\", cwd, L"");

// Absolute Windows path
test(L"C:\\Windows", cwd, L"C:\\Windows");
test(L"C:/Users/migrie", cwd, L"C:/Users/migrie");

// Relative Windows path
test(L"C:/Users/migrie\\.", cwd, L"."); // ?
test(L"C:/Users/migrie\\.\\System32", cwd, L".\\System32"); // ?
test(L"C:/Users/migrie\\./dev", cwd, L"./dev");

// WSL '~' path
test(L"~", cwd, L"~");
test(L"~/dev", cwd, L"~/dev");

// WSL or Windows / path - this will ultimately be evaluated by the connection
test(L"/", cwd, L"/");
test(L"/dev", cwd, L"/dev");
}
}
24 changes: 24 additions & 0 deletions src/types/utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -828,3 +828,27 @@ std::wstring_view Utils::TrimPaste(std::wstring_view textView) noexcept

return textView.substr(0, lastNonSpace + 1);
}

std::wstring Utils::EvaluateStartingDirectory(
std::wstring_view currentDirectory,
std::wstring_view startingDirectory)
{
std::wstring resultPath{ startingDirectory };

// We only want to resolve the new WD against the CWD if it doesn't look
// like a Linux path (see GH#592)

// Append only if it DOESN'T look like a linux-y path. A linux-y path starts
// with `~` or `/`.
const bool looksLikeLinux =
resultPath.size() >= 1 &&
(til::at(resultPath, 0) == L'~' || til::at(resultPath, 0) == L'/');

if (!looksLikeLinux)
{
std::filesystem::path cwd{ currentDirectory };
cwd /= startingDirectory;
resultPath = cwd.wstring();
}
return resultPath;
}
4 changes: 4 additions & 0 deletions tools/bcz.cmd
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ if (%1) == (rel) (
echo Manually building release
set _LAST_BUILD_CONF=Release
)
if (%1) == (audit) (
echo Manually building audit mode
set _LAST_BUILD_CONF=AuditMode
)
if (%1) == (no_clean) (
set _MSBUILD_TARGET=Build
)
Expand Down

0 comments on commit 8d2447b

Please sign in to comment.