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

Can't bypass DOSkey macro with leading space #4189

Closed
cniggeler opened this issue Jan 12, 2020 · 39 comments · Fixed by #13476
Closed

Can't bypass DOSkey macro with leading space #4189

cniggeler opened this issue Jan 12, 2020 · 39 comments · Fixed by #13476
Labels
Area-CookedRead The cmd.exe COOKED_READ handling Area-Input Related to input processing (key presses, mouse, etc.) good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Impact-Compatibility Sure don't work like it used to. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@cniggeler
Copy link

Environment

Windows build number: [run `[Environment]::OSVersion` for powershell, or `ver` for cmd]
Windows Terminal version (if applicable):
Microsoft Windows [Version 10.0.18362.535]
Any other software? A DosKEY macro

Steps to reproduce

Create a DOSkey macro, e.g.,
doskey dir=dir /b
Run the command,
dir
then run the command (note space in front)
dir

Expected behavior

In the 2nd run, you should get the default "dir" listing. This is the case in classic console.

Actual behavior

You get the results of "dir /b" again!

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jan 12, 2020
@Luuk34
Copy link

Luuk34 commented Jan 12, 2020

The ' dir' does not work in my default console (CMD), it will still use the alias?

@cniggeler
Copy link
Author

The ' dir' does not work in my default console (CMD), it will still use the alias?

Sorry, but are you a bot? If not, are you on the Experimental Console programming team trying to replicate my reported bug? The dir command is probably the most-used command in console operation, so how could you be without one?

@eryksun
Copy link

eryksun commented Jan 12, 2020

It's not counter-intuitive that matching a console alias would trim leading spaces. However, the doskey docs do state plainly that the user should "type the macro name starting at the first position on the command line". Moreover, they include the following instructions for creating a macro with same name as a command:

If you always use a particular command with specific command-line options, you can create a macro that has the same name as the command. To specify whether you want to run the macro or the command, follow these guidelines:

  • To run the macro, begin typing the macro name immediately after the command prompt, with no space between the prompt and the command name.

  • To run the command, insert one or more white spaces between the command prompt and the command name.

@Luuk34
Copy link

Luuk34 commented Jan 12, 2020

What I mean to say is that this bug report is claiming that running ' dir', after defining an alias for dir, should output the normal behavior of what is expected in CMD.

But when I tested this outside WT, I did not see this behavior. The macro is always being run, and I do not have a way to run DIR with other options than the one given in the alias.

I am running the same version of Windows, Microsoft Windows [Version 10.0.18363.535].

@cniggeler
Copy link
Author

cniggeler commented Jan 12, 2020

@Luuk34: ah, thanks for the clarification. But eryksun has found the chapter/verse of how it should work according to the docs - thanks! - and what it's doing is definitely NOT according to the docs.

Can't speak to your situation/configuration. I've replicated it on two different machines. You do need to completely exit, then restart, your console session after unchecking/checking "use legacy console" to make the changes stick.

Also:

The macro is always being run, and I do not have a way to run DIR with other options than the one given in the alias.

Note that my original post says the way around it is to put a space before the "dir" command. That's the canonical method of bypassing a DOSkey macro with the same name as a nother command.

@j4james
Copy link
Collaborator

j4james commented Jan 12, 2020

It looks like this should be easy to fix. I can get it working in a local build just by deleting this line:

terminal/src/host/alias.cpp

Lines 1154 to 1155 in 9b92986

// Trim leading spaces off of sourceCopy if it has any.
s_TrimLeadingSpaces(sourceCopy);

But I'm curious why that trimming routine was added in the first place if it's not compatible with the legacy console. Someone from the core team will probably need to confirm whether that was a mistake and it's safe to remove.

@DHowett-MSFT
Copy link
Contributor

DHowett-MSFT commented Jan 13, 2020

Author: Michael Niksa
Date: Fri Apr 27 20:38:26 2018 +0000

Merged PR 1747128: Make interactive mode command line trim leading spaces from aliases to improve usability

Per MSFT:16672211 trim spaces from alias lookups since they never resolved to anything anyway. Makes aliases a touch
more useful at the cooked command line.

Paul noticed this while I was doing the alias improvement code and suggested the enhancement.

Related work items: MSFT:16672211

Looks like our usability improvement was a usability detriment. 😄

@DHowett-MSFT DHowett-MSFT added Area-Input Related to input processing (key presses, mouse, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase labels Jan 13, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jan 13, 2020
@DHowett-MSFT DHowett-MSFT added this to the 21H1 milestone Jan 13, 2020
@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jan 13, 2020
@DHowett-MSFT DHowett-MSFT added the Impact-Compatibility Sure don't work like it used to. label Mar 19, 2020
@zadjii-msft zadjii-msft added good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. labels Jan 4, 2022
@zadjii-msft zadjii-msft modified the milestones: Windows vNext, 22H1 Jan 4, 2022
@zadjii-msft zadjii-msft modified the milestones: 22H1, Terminal v1.14 Feb 2, 2022
@zadjii-msft zadjii-msft added the Area-CookedRead The cmd.exe COOKED_READ handling label Feb 23, 2022
@cniggeler
Copy link
Author

cniggeler commented Mar 19, 2022

Hi, it's been 2+ years and now I note this issue is still present in Windows 11! Looking at the status, I see it continues to be pushed out, and the last milestone Feb. 2 is past. Can we ensure this gets put in this time? The fix can't be that hard - don't left trim the command :-)

@zadjii-msft
Copy link
Member

@cniggeler this is currently slated for the current milestone, but is fairly low priority relative to some of the other bugs in the milestone. I do believe this is a fairly easy-starter issue if you'd like to try submitting a fix yourself 😉

@cniggeler
Copy link
Author

Uh, OK I'll think about it. Windows being a for-profit product I'm surprised at the suggestion of a user-contributed, free solution ;-)

@cniggeler
Copy link
Author

@zadjii-msft I just found that you are in charge of this project - congrats! That being the case, what is your strategy for handling bugs? This IS a functional bug - I'm not asking about font rendering or cursor blink rate. When does the fact that this is a bug, that it's been aging two years, and that it's probably a one-line change (since you or your team would know where the line is and I don't) factor into your decision to just fix this and close it?

@DHowett
Copy link
Member

DHowett commented Mar 28, 2022

Huh, this is pretty curious.

Would you be able to run it under the debugger after enabling loader snaps?

  • Windbg comes with a tool called "Global Flags" (gflags) - launch it, navigate to Image File, and enter OpenConsole.exe
  • Enable "Show loader snaps"
    • image
  • [Apply]

When you run OpenConsole under the debugger, it should emit a terrible volume of text about library loading, symbol binding, etc. that might help figure out why it won't load on this machine. 😄

@DHowett
Copy link
Member

DHowett commented Mar 28, 2022

Oh that might do it. Hard to exactly parse that, but 10.0.17134.12 is quite a bit out of date at this point. I would not be surprised at all if the current conhost builds don't run on that version of windows 10 anymore.

Hmm. Quite possible... but to my knowledge we haven't made any changes that would exclude running on that version.

In addition, I think @cniggeler was only stating that they have that SDK installed (not that they're running on that version of Windows!)

@cniggeler
Copy link
Author

I'm running Windows 10 Pro 21H1. Microsoft Windows [Version 10.0.19044.1586]. @DHowett is correct that that was the version for the SDK.

I will run openconsole through windbg with loader snaps and reply next.

@cniggeler
Copy link
Author

cniggeler commented Mar 28, 2022

@DHowett I loaded gflags and checked loader snaps as requested. I pressed Launch, which had no visual feedback, so I returned to Windbg. There is now 2155 lines of info, which I've attached as a text file.
opencon_snap.txt

I have VS2017 loaded on the test machine. Would it be possible/ advisable to build with debug info on the VS2019 build machine and then open the debug .exe in VS2017 on the test machine?

@DHowett
Copy link
Member

DHowett commented Mar 28, 2022

It looks like everything we need is loading; any libraries that can't be found (Status c0000135) later get picked up from the system directory.

Can you generate a backtrace when you get here? I wonder if we're exiting in an orderly fashion for some reason...

image

A debug build might help. You'll have to copy over the debug CRT (from the VC\ directory in your VS 2019 install!)

@cniggeler
Copy link
Author

I'm assuming you meant View - Call stack:
opencon_bt

Also, here's the output after selecting GO, which adds another 1050 lines of output:
opencon_go.txt

@DHowett
Copy link
Member

DHowett commented Mar 28, 2022

Indeed!

It looks like the console is choosing to exit at +0xdb7a4. If you copy the pdb for OpenConsole from your build machine to the run you're running on, you'll get a bit more info. It could just be returning from main after failing to do something, admittedly... but nothing in that loader snap log looks wrong.

@cniggeler
Copy link
Author

I think this took. I kept getting message WARNING: Non-directory path: 'C:\temp\OpenConsole.pdb' even after using "Browse" in windbg. Anyway, slightly larger file attached:
opencon_w_pdb.txt

@DHowett
Copy link
Member

DHowett commented Mar 28, 2022

And what's the call stack at termination for that build? :)

@cniggeler
Copy link
Author

Has a function name in it :-)

opencon_bt2

@cniggeler
Copy link
Author

Update: I also tried this on a brand new Win11 machine. Installed the latest C runtime and copied the release version of OpenConsole.exe to it. It also does not run.

@DHowett
Copy link
Member

DHowett commented Mar 30, 2022

You know, I'm still stumped by this. Sorry to leave you on read.

Here's what I'll say: You can send out a pull request if you'd like, without further validation. The CI will take care of producing a runnable copy of this kit and kaboodle and then actually run it on your behalf 😄

To be fair, trying to validate something on multiple machines is farther than I suspect most of the team usually goes 😁

@cniggeler
Copy link
Author

cniggeler commented Mar 30, 2022

Thanks. At this stage I was more interested in why the build wasn't able to run on different machines, because maybe that would point out some corrective action in the build scripts or execution prerequisites. But I would love to have it on my main machine where I work every day - been waiting 2.25 years for it!

The change was literally the one line change above, and a pull request seems heavyweight. Is there an alternative to just add it to the branch for the next scheduled release? My build/test has managed to show it works ;-)

@zadjii-msft zadjii-msft modified the milestones: Terminal v1.14, 22H2 Apr 28, 2022
@ghost ghost added the In-PR This issue has a related PR label Jul 10, 2022
@ghost ghost closed this as completed in #13476 Jul 11, 2022
ghost pushed a commit that referenced this issue Jul 11, 2022
## Summary of the Pull Request

When you create a console alias that overrides an existing command, it
should still be possible to execute the original command by prefixing it
with a space. However, at some point in the past, there was an attempt
to improve the usability by trimming leading spaces, and that ended up
breaking this functionality. This PR reverts that change, so leading
spaces can once again be used to bypass an alias.

## PR Checklist
* [x] Closes #4189
* [x] CLA signed.
* [x] Tests added/passed
* [ ] Documentation updated.
* [ ] Schema updated.
* [x] I've discussed this with core contributors already. Issue number
where discussion took place: #4189

## Validation Steps Performed

I've updated the existing alias unit test for leading spaces to match
the new behavior, i.e. it now confirms that a command with leading
spaces will not match the alias.

I've also manually confirmed that the `doskey` test case reported in
issue #4189 is now working as expected.
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Jul 11, 2022
DHowett pushed a commit that referenced this issue Jul 19, 2022
## Summary of the Pull Request

When you create a console alias that overrides an existing command, it
should still be possible to execute the original command by prefixing it
with a space. However, at some point in the past, there was an attempt
to improve the usability by trimming leading spaces, and that ended up
breaking this functionality. This PR reverts that change, so leading
spaces can once again be used to bypass an alias.

## PR Checklist
* [x] Closes #4189
* [x] CLA signed.
* [x] Tests added/passed
* [ ] Documentation updated.
* [ ] Schema updated.
* [x] I've discussed this with core contributors already. Issue number
where discussion took place: #4189

## Validation Steps Performed

I've updated the existing alias unit test for leading spaces to match
the new behavior, i.e. it now confirms that a command with leading
spaces will not match the alias.

I've also manually confirmed that the `doskey` test case reported in
issue #4189 is now working as expected.

(cherry picked from commit cd2166a)
Service-Card-Id: 84072860
Service-Version: 1.15
@ghost
Copy link

ghost commented Aug 5, 2022

🎉This issue was addressed in #13476, which has now been successfully released as Windows Terminal Preview v1.15.200.:tada:

Handy links:

@cniggeler
Copy link
Author

I'm trying v1.15.200 and am encountering problems for which I've entered a separate comment under its release. My questions here are,

  • Is Terminal intended to replace cmd.exe?
  • What happens when I UNcheck "Use legacy console" in the cmd.exe Properties - is that supposed to start this Terminal - because it's not, I continue to get the old doskey behavior.
  • How do I get in/out of the legacy/ new console behavior is via this Properties checkbox in cmd.exe... or am I just supposed to create a shortcut to wt.exe instead?

Thanks!

@Luuk34
Copy link

Luuk34 commented Aug 6, 2022

I'm trying v1.15.200 and am encountering problems for which I've entered a separate comment under its release. My questions here are,

  • Is Terminal intended to replace cmd.exe?

The Windows Terminal is a modern, fast, efficient, powerful, and productive terminal application for users of command-line tools and shells like Command Prompt, PowerShell, and WSL. Its main features include multiple tabs, panes, Unicode and UTF-8 character support, a GPU accelerated text rendering engine, and custom themes, styles, and configurations.
(see: https://apps.microsoft.com/store/detail/windows-terminal/9N0DX20HK701?l=en

In other words Terminal is an app which combines the functionality of different shells.

  • What happens when I UNcheck "Use legacy console" in the cmd.exe Properties - is that supposed to start this Terminal - because it's not, I continue to get the old doskey behavior.

see: Legacy Console Mode

  • How do I get in/out of the legacy/ new console behavior is via this Properties checkbox in cmd.exe... or am I just supposed to create a shortcut to wt.exe instead?

?

Thanks!

@cniggeler
Copy link
Author

Thanks for the reply! Some of this raises more questions than it answers.

see: Legacy Console Mode

This link references "the Console Host team". Who is that?

In Task Manager, the "Windows Command Processor" app is comprised of two components: Console Windows Host (conhost.exe) and (the recursively named) Windows Command Processor (cmd.exe). But if I try to run conhost.exe by itself, nothing happens (Run... type in conhost, no window opens).

Then, in the next paragraph, that linked page says "If you experience an issue that requires you to use the legacy console that is not documented here, please contact the team on the microsoft/terminal GitHub repository." This is how/why I opened this ticket regarding doskey.

Is the Terminal team different from the Console Host team?

This is still unclear: if I UNcheck "Use legacy console" in the conhost console that comes up when I run cmd.exe, what application is then run - is it Terminal? Because if it is, I can say for sure that the Terminal Preview v1.15.200 is NOT being accessed. Or is it instead some extended mode of conhost.exe?

All of which lead me to ask my last question, which I will try to rephrase in a couple questions:

  • Is the console (conhost.exe) that is opened when running cmd.exe deprecated?
  • If so, is Terminal intended to replace it?
  • And in that case, should I just create a new shortcut to wt.exe and use that instead of cmd.exe?

Thanks!

@Luuk34
Copy link

Luuk34 commented Aug 6, 2022

@Sniggler: too much questions for /me to answer (and not make any mistakes, or wrong assumptions), I will leave that to the Team which is responsible for this 😉

@cniggeler
Copy link
Author

Despite the merge and closed-fixed claim in #13476 afraid this is still not fixed in 1.19.10573.0! The test process is the same as it's always been.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CookedRead The cmd.exe COOKED_READ handling Area-Input Related to input processing (key presses, mouse, etc.) good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Impact-Compatibility Sure don't work like it used to. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants