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

Jump to bottom

Remove CONSOLE_API_MSG::UpdateUserBufferPointers hack #10326

Merged
2 commits merged into from
Jun 14, 2021
Merged

Remove CONSOLE_API_MSG::UpdateUserBufferPointers hack #10326

2 commits merged into from
Jun 14, 2021

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Jun 4, 2021 •
edited by DHowett

Summary of the Pull Request

This commit introduces a copy constructor/operator for
_CONSOLE_API_MSG. The change is not trivial as the struct contains a
union of unnamed structs that cannot be copied using regular language
features. As such a copy operator using memcpy was implemented.
Additionally all access specifiers were removed, as those allow a C++
compiler to reorder struct members. This would break message passing.
This commit is a good opportunity to prevent such miscompilations
proactively.

Validation Steps Performed

  • Command prompts of WSL2 fish-shell and pwsh still work ✔️

Closes #10076

@ghost ghost added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase labels Jun 4, 2021
@lhecker lhecker force-pushed the dev/lhecker/api-msg-copy branch 2 times, most recently from 19006a8 to 0a5be0d Compare June 4, 2021 19:51
miniksa
miniksa reviewed Jun 9, 2021
View reviewed changes
src/server/ApiMessage.h
void SetReplyStatus(const NTSTATUS Status);
void SetReplyInformation(const ULONG_PTR pInformation);

// DO NOT PUT ACCESS SPECIFIERS HERE.
Copy link
Member

Choose a reason for hiding this comment

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

👍

miniksa
miniksa reviewed Jun 9, 2021
View reviewed changes
src/server/WaitBlock.cpp
{
delete _pWaiter;
}
delete _pWaiter;
Copy link
Member

Choose a reason for hiding this comment

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

Because the constructor threw if null? Yeah?

Copy link
Member

Choose a reason for hiding this comment

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

(Might want to write FAIL_FAST_IF_NULL on it in case some future goof violates your assumption.)

Copy link
Member Author

Choose a reason for hiding this comment

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

delete is a no-op for nullptr! That's why you don't have to check for nullptr first. 🙂
That's basically a drive-by fix. I only noticed it while working on the other changes in this file.

Copy link
Member

Choose a reason for hiding this comment

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

Oh ho. Sneaky sneaky.

@lhecker
Copy link
Member Author

lhecker commented Jun 14, 2021

Force push to resolve merge conflicts in .github/actions/spelling/allow/apis.txt.

@lhecker lhecker added the Needs-Second It's a PR that needs another sign-off label Jun 14, 2021
DHowett
DHowett approved these changes Jun 14, 2021
View reviewed changes
src/server/ApiMessage.cpp Outdated
CreateObject{ 0 },
CreateScreenBuffer{ 0 },
msgHeader{ 0 }
inline constexpr size_t structPacketDataSize = sizeof(_CONSOLE_API_MSG) - offsetof(_CONSOLE_API_MSG, Descriptor);
Copy link
Member

Choose a reason for hiding this comment

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

I forgot which way you explained this -- does inline constexpr force a symbol to be created for this? If so: do we need one here?

Copy link
Member Author

@lhecker lhecker Jun 14, 2021 •
edited

Choose a reason for hiding this comment

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

Thanks for catching this mistake! inline forces external linkage.
Yeah... it's... it's something special indeed.

@DHowett
Copy link
Member

DHowett commented Jun 14, 2021

Would you mind running the host feature tests with the app verifier enabled for Heap checks? This will exercise the "multiple inflight messages" test 😄

@DHowett
Copy link
Member

DHowett commented Jun 14, 2021

Oh, you'll need to set appverifier for Te.ProcessHost.exe (forgot to mention)

@lhecker
Copy link
Member Author

lhecker commented Jun 14, 2021

All host feature tests completed successfully. 🙂

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jun 14, 2021
@ghost
Copy link

ghost commented Jun 14, 2021

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 296037a into main Jun 14, 2021
@ghost ghost deleted the dev/lhecker/api-msg-copy branch June 14, 2021 19:52
@ghost
Copy link

ghost commented Jul 14, 2021

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

Handy links:

@ghost ghost mentioned this pull request Jul 14, 2021
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miniksa miniksa miniksa approved these changes

@DHowett DHowett DHowett approved these changes

@zadjii-msft zadjii-msft Awaiting requested review from zadjii-msft

@carlos-zamora carlos-zamora Awaiting requested review from carlos-zamora

@leonMSFT leonMSFT Awaiting requested review from leonMSFT

@PankajBhojwani PankajBhojwani Awaiting requested review from PankajBhojwani

Assignees
No one assigned
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Second It's a PR that needs another sign-off Product-Conhost For issues in the Console codebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove CONSOLE_API_MSG::UpdateUserBufferPointers hack
3 participants
@lhecker @DHowett @miniksa