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

Ensure PowerShell Core profile commandline is quoted #12086

Merged
1 commit merged into from Jan 4, 2022

Conversation

ianjoneill
Copy link
Contributor

@ianjoneill ianjoneill commented Jan 4, 2022

Ensures the PowerShell Core profile's commandline is quoted. This allows
the profile to work correctly if there are files in place on the machine
(e.g. one called C:\Program) that prevent CreateProcess() from
invoking the un-quoted commandline.

Validation Steps Performed

Created a file called C:\Program and opened the PowerShell profile in
terminal.

Closes #11717

@ghost ghost added Area-Settings Issues related to settings and customizability, for console or terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Jan 4, 2022
@zadjii-msft zadjii-msft self-assigned this Jan 4, 2022
@DHowett
Copy link
Member

DHowett commented Jan 4, 2022

uuuuuuuuuugh thank you

@DHowett DHowett added zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. labels Jan 4, 2022
Comment on lines +313 to +319
const auto& unquotedCommandline = psI.executablePath.native();
std::wstring quotedCommandline;
quotedCommandline.reserve(unquotedCommandline.size() + 2);
quotedCommandline.push_back(L'"');
quotedCommandline.append(unquotedCommandline);
quotedCommandline.push_back(L'"');
profile->Commandline(winrt::hstring{ quotedCommandline });
Copy link
Member

Choose a reason for hiding this comment

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

for cheap tricks, we could also...

auto quotedCommandline = fmt::format(FMT_COMPILE(L"\"{}\""), psI.executablePath.native());

which should (?????) compile down to effectively optimal code (!)

Copy link
Contributor Author

@ianjoneill ianjoneill Jan 4, 2022

Choose a reason for hiding this comment

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

A micro-benchmark on my machine shows my code to be faster :)

        auto start = std::chrono::steady_clock::now();
        for (int i = 0; i < 1000000; i++)
        {
            const auto& unquotedCommandline = psI.executablePath.native();
            std::wstring quotedCommandline;
            quotedCommandline.reserve(unquotedCommandline.size() + 2);
            quotedCommandline.push_back(L'"');
            quotedCommandline.append(unquotedCommandline);
            quotedCommandline.push_back(L'"');
            profile->Commandline(winrt::hstring{ quotedCommandline });
        }
        auto end = std::chrono::steady_clock::now();
        std::wstringstream stream;
        stream << "Elapsed microseconds for native code " << std::chrono::duration_cast<std::chrono::microseconds>(end - start).count() << std::endl;
        OutputDebugString(stream.str().c_str());

        start = std::chrono::steady_clock::now();
        for (int i = 0; i < 1000000; i++)
        {
            auto quotedCommandline = fmt::format(FMT_COMPILE(L"\"{}\""), psI.executablePath.native());
            profile->Commandline(winrt::hstring{ quotedCommandline });
        }
        end = std::chrono::steady_clock::now();
        std::wstringstream stream2;
        stream2 << "Elapsed microseconds for fmt code " << std::chrono::duration_cast<std::chrono::microseconds>(end - start).count() << std::endl;
        OutputDebugString(stream2.str().c_str());

Output:

Elapsed microseconds for native code 234055
Elapsed microseconds for fmt code 258514

Edit: Run the loop a factor of 10 more times, and do something with the constructed string.

Copy link
Member

Choose a reason for hiding this comment

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

A micro-benchmark on my machine shows my code to be faster :)

Well, you'll make @lhecker 's day. And I chuckled. Good enough.

Copy link
Member

Choose a reason for hiding this comment

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

LOL. Fair enough 😁

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 4, 2022
@ghost
Copy link

ghost commented Jan 4, 2022

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 4930508 into microsoft:main Jan 4, 2022
DHowett pushed a commit that referenced this pull request Jan 21, 2022
Ensures the PowerShell Core profile's commandline is quoted. This allows
the profile to work correctly if there are files in place on the machine
(e.g. one called `C:\Program`) that prevent `CreateProcess()` from
invoking the un-quoted commandline.

## Validation Steps Performed
Created a file called `C:\Program` and opened the PowerShell profile in
terminal.

Closes #11717

(cherry picked from commit 4930508)
@DHowett DHowett removed the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label Jan 26, 2022
@ghost
Copy link

ghost commented Feb 3, 2022

🎉Windows Terminal Preview v1.13.10336.0 has been released which incorporates this pull request.:tada:

Handy links:

@DHowett DHowett removed the zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. label Feb 4, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Settings Issues related to settings and customizability, for console or terminal AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

error 0x800700c1 when launching pwsh.exe
4 participants