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.

Sign up for GitHub

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 console aliases not working #14991

Merged
merged 3 commits into from
Mar 17, 2023
Merged

Fix console aliases not working #14991

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
612baad
Fix console aliases not working
lhecker Mar 14, 2023
470b02c
Fix formatting
lhecker Mar 14, 2023
1924a7e
Defensive coding practices sponsored by MtnDustin
lhecker Mar 14, 2023
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
19 changes: 1 addition & 18 deletions .github/actions/spelling/expect/expect.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,6 @@ chh
chk
CHT
Cic
CLA
Clcompile
CLE
cleartype
Expand Down Expand Up @@ -479,7 +478,6 @@ defterm
DELAYLOAD
DELETEONRELEASE
Delt
demoable
depersist
deprioritized
deserializers
Expand Down Expand Up @@ -536,7 +534,6 @@ DSSCL
DSwap
DTest
DTTERM
DUMMYUNIONNAME
dup'ed
dvi
dwl
Expand Down Expand Up @@ -589,7 +586,6 @@ ETW
EUDC
EVENTID
eventing
everytime
evflags
evt
execd
Expand Down Expand Up @@ -797,7 +793,6 @@ HIBYTE
hicon
HIDEWINDOW
hinst
Hirots
HISTORYBUFS
HISTORYNODUP
HISTORYSIZE
Expand Down Expand Up @@ -898,7 +893,6 @@ INSERTMODE
INTERACTIVITYBASE
INTERCEPTCOPYPASTE
INTERNALNAME
inthread
intsafe
INVALIDARG
INVALIDATERECT
Expand Down Expand Up @@ -1255,14 +1249,12 @@ ntm
nto
ntrtl
ntstatus
ntsubauth
NTSYSCALLAPI
nttree
nturtl
ntuser
NTVDM
ntverp
NTWIN
nugetversions
nullability
nullness
Expand Down Expand Up @@ -1301,8 +1293,6 @@ opencode
opencon
openconsole
openconsoleproxy
OPENIF
OPENLINK
openps
openvt
ORIGINALFILENAME
Expand Down Expand Up @@ -1355,9 +1345,7 @@ pcg
pch
PCIDLIST
PCIS
PCLIENT
PCLONG
PCOBJECT
pcon
PCONSOLE
PCONSOLEENDTASK
Expand All @@ -1369,7 +1357,6 @@ pcshell
PCSHORT
PCSR
PCSTR
PCUNICODE
PCWCH
PCWCHAR
PCWSTR
Expand Down Expand Up @@ -1418,7 +1405,6 @@ PLOGICAL
pnm
PNMLINK
pntm
PNTSTATUS
POBJECT
Podcast
POINTSLIST
Expand All @@ -1437,7 +1423,6 @@ ppf
ppguid
ppidl
PPROC
PPROCESS
ppropvar
ppsi
ppsl
Expand Down Expand Up @@ -1501,7 +1486,6 @@ ptrs
ptsz
PTYIn
PUCHAR
PUNICODE
pwch
PWDDMCONSOLECONTEXT
pws
Expand Down Expand Up @@ -1563,7 +1547,6 @@ REGISTEROS
REGISTERVDM
regkey
REGSTR
reingest
RELBINPATH
remoting
renamer
Expand Down Expand Up @@ -1858,6 +1841,7 @@ TDP
TEAMPROJECT
tearoff
Teb
Techo
tellp
teraflop
terminalcore
Expand Down Expand Up @@ -2000,7 +1984,6 @@ unittesting
unittests
unk
unknwn
unmark
UNORM
unparseable
unregistering
Expand Down
2 changes: 1 addition & 1 deletion src/host/inputReadHandleData.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ bool INPUT_READ_HANDLE_DATA::IsInputPending() const

bool INPUT_READ_HANDLE_DATA::IsMultilineInput() const
{
FAIL_FAST_IF(!_isInputPending); // we shouldn't have multiline input without a pending input.
assert(_isInputPending); // we shouldn't have multiline input without a pending input.
Copy link
Member Author

Choose a reason for hiding this comment

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

Another one down the list #14298.

return _isMultilineInput;
}

Expand Down
38 changes: 27 additions & 11 deletions src/host/readDataCooked.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1019,27 +1019,43 @@ void COOKED_READ_DATA::SavePendingInput(const size_t index, const bool multiline
}

Tracing::s_TraceCookedRead(_clientProcess, _backupLimit, base::saturated_cast<ULONG>(idx));
ProcessAliases(LineCount);

// Don't be fooled by ProcessAliases only taking one argument. It rewrites multiple
// class members on return, including `_bytesRead`, requiring us to reconstruct `input`.
Comment on lines +1023 to +1024
Copy link
Member

Choose a reason for hiding this comment

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

Good callout!

ProcessAliases(LineCount);
input = { _backupLimit, _bytesRead / sizeof(wchar_t) };

// The exact reasons for this are unclear to me (the one writing this comment), but this code used to
// split the contents of a multiline alias (for instance `doskey test=echo foo$Techo bar$Techo baz`)
// into multiple separate read outputs, ensuring that the client receives them line by line.
//
// This code first truncates the `input` to only contain the first line, so that Consume() below only
// writes that line into the user buffer. We'll later store the remainder in SaveMultilinePendingInput().
if (LineCount > 1)
{
input = input.substr(0, idx + 1);
// ProcessAliases() is supposed to end each line with \r\n. If it doesn't we might as well fail-fast.
const auto firstLineEnd = input.find(UNICODE_LINEFEED) + 1;
input = input.substr(0, firstLineEnd);
}
}
}

const auto inputSizeBefore = input.size();
GetInputBuffer()->Consume(isUnicode, input, writer);

if (!input.empty())
if (LineCount > 1)
{
if (LineCount > 1)
{
GetInputReadHandleData()->SaveMultilinePendingInput(input);
}
else
{
GetInputReadHandleData()->SavePendingInput(input);
}
// This is a continuation of the above identical if condition.
// We've truncated the `input` slice and now we need to restore it.
const auto inputSizeAfter = input.size();
const auto amountConsumed = inputSizeBefore - inputSizeAfter;
input = { _backupLimit, _bytesRead / sizeof(wchar_t) };
input = input.substr(amountConsumed);
GetInputReadHandleData()->SaveMultilinePendingInput(input);
Copy link
Member Author

@lhecker lhecker Mar 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This isn't necessarily the most readable code I've written so far... But it does do the correct thing: After Consume() returns, input might not be empty! So if we restore the original, full input slice we need to manually calculate the amount of data that was read (amountConsumed).

An alternative I've considered is to change Consume() to return the amount of characters that were read, but given how Consume() works internally, this would be identical to this inputSizeBefore / inputSizeAfter code. Alternatively we could pass a "character limit" argument, but that would be, once again, similar to the existing code, since string-views are already strings with a limit, so I felt like passing 2 limits wouldn't be any less confusing.

}
else if (!input.empty())
{
GetInputReadHandleData()->SavePendingInput(input);
}

numBytes = _userBufferSize - writer.size();
Expand Down
28 changes: 18 additions & 10 deletions src/host/stream.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,28 +288,36 @@ try
{
bytesRead = 0;

auto pending = readHandleState.GetPendingInput();
const auto pending = readHandleState.GetPendingInput();
auto input = pending;

// This is basically the continuation of COOKED_READ_DATA::_handlePostCharInputLoop.
if (readHandleState.IsMultilineInput())
{
const auto idx = pending.find(UNICODE_LINEFEED);
if (idx != decltype(pending)::npos)
{
// +1 to include the newline.
pending = pending.substr(0, idx + 1);
}
const auto firstLineEnd = input.find(UNICODE_LINEFEED) + 1;
lhecker marked this conversation as resolved.
Show resolved Hide resolved
input = input.substr(0, firstLineEnd);
}

const auto inputSizeBefore = input.size();
std::span writer{ buffer };
inputBuffer.Consume(unicode, pending, writer);
inputBuffer.Consume(unicode, input, writer);

// Since we truncated `input` to only include the first line,
// we need to restore `input` here to the entirety of the remaining input.
if (readHandleState.IsMultilineInput())
{
const auto inputSizeAfter = input.size();
const auto amountConsumed = inputSizeBefore - inputSizeAfter;
input = pending.substr(amountConsumed);
Copy link
Member Author

Choose a reason for hiding this comment

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

This code block is identical to the other one, but it's shorter because we can easily access the previously saved pending slice.

}

if (pending.empty())
if (input.empty())
{
readHandleState.CompletePending();
}
else
{
readHandleState.UpdatePending(pending);
readHandleState.UpdatePending(input);
}

bytesRead = buffer.size() - writer.size();
Expand Down