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

Investigate why the COOKED_READ_DATA ctor takes a string_view for wchar_t data? #5618

Closed
zadjii-msft opened this issue Apr 28, 2020 · 3 comments · Fixed by #15780
Closed
Labels
Area-CookedRead The cmd.exe COOKED_READ handling Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Conhost For issues in the Console codebase

Comments

@zadjii-msft
Copy link
Member

COOKED_READ_DATA takes an initialData parameter of a string_view

COOKED_READ_DATA::COOKED_READ_DATA(_In_ InputBuffer* const pInputBuffer,
_In_ INPUT_READ_HANDLE_DATA* const pInputReadHandleData,
SCREEN_INFORMATION& screenInfo,
_In_ size_t UserBufferSize,
_In_ PWCHAR UserBuffer,
_In_ ULONG CtrlWakeupMask,
_In_ CommandHistory* CommandHistory,
const std::wstring_view exeName,
const std::string_view initialData) :

However, when it memcpy's it, it copies it as a wchar_t buffer:

if (!initialData.empty())
{
memcpy_s(_bufPtr, _bufferSize, initialData.data(), initialData.size());
_bytesRead += initialData.size();
const size_t cchInitialData = initialData.size() / sizeof(wchar_t);

As you can see here, _bufPtr is a wchar_t* which is initialized to _bufPtr = reinterpret_cast<wchar_t*>(_buffer.get())

wchar_t* _bufPtr; // current position to insert chars at
// should be const. the first char of the buffer
wchar_t* _backupLimit;
size_t _userBufferSize; // doubled size in ansi case
wchar_t* _userBuffer;
size_t* _pdwNumBytes;
std::unique_ptr<byte[]> _buffer;
std::wstring _exeName;
std::unique_ptr<ConsoleHandleData> _tempHandle;

As I'm writing tests that use some cooked read data for #1856, I'm finding that the string I'm initializing the cooked read with is getting horribly mangled, because I'm passing a char buffer in.

Why is it like this? Does this magically just normally work when users call ReadConsole with wchar_t data in the buffer? I couldn't really figure it out in just a morning, so I'm filing this to make sure to follow up.

The test in question is ConptyRoundtripTests::TestResizeWithCookedRead.

@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 Apr 28, 2020
@zadjii-msft zadjii-msft added Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Conhost For issues in the Console codebase labels Apr 28, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Apr 28, 2020
@zadjii-msft zadjii-msft added this to the 21H1 milestone Apr 28, 2020
@DHowett-MSFT
Copy link
Contributor

AFAIK this is related to how we share cooked read between A and W APIs?

@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 Apr 28, 2020
@DHowett-MSFT
Copy link
Contributor

Actually, yeah, that doesn't make sense.

ghost pushed a commit that referenced this issue Apr 29, 2020
… window (#5620)

Hide any commandline (cooked read) we have before we begin a resize, and
show it again after the resize. 

## References

* I found #5618 while I was working on this.

## PR Checklist
* [x] Closes #1856
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

Basically, during a resize, we try to restore the viewport position
correctly, and part of that checks where the current commandline ends.
However, when we do that, the commandline's _current_ state still
reflects the _old_ buffer size, so resizing to be smaller can cause us
to throw an exception, when we find that the commandline doesn't fit in
the new viewport cleanly.

By hiding it, then redrawing it, we avoid this problem entirely. We
don't need to perform the check on the old commandline contents (since
they'll be empty), and we'll redraw it just fine for the new buffer size

## Validation Steps Performed
* ran tests
* checked resizing, snapping in conhost with a cooked read
* checked resizing, snapping in the Terminal with a cooked read
@zadjii-msft zadjii-msft modified the milestones: Console Backlog, 22H2 Jan 4, 2022
@zadjii-msft zadjii-msft added the Area-CookedRead The cmd.exe COOKED_READ handling label Feb 23, 2022
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Aug 1, 2023
microsoft-github-policy-service bot pushed a commit that referenced this issue Aug 1, 2023
This is a minor cleanup to deduplicate the two ReadConsole methods
and will help with making changes to how `COOKED_READ_DATA` is called.
It additionally changes the initial data payload from a `string_view`
to a `wstring_view` as it is guaranteed to be `wchar_t`.
This matches the current `COOKED_READ_DATA` implementation which
blindly assumes that the initial data consists of `wchar_t`.

Closes #5618
@DHowett
Copy link
Member

DHowett commented Aug 1, 2023

IT TURNS OUT

That this was because initialData was explicitly documented as being broken for ReadConsoleA, and only worked for ReadConsoleW. We didn't need to support the ANSI version, so we were just being weirdos using string_view to pass around a pointer+length.

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-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants