-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Remove CONSOLE_API_MSG::UpdateUserBufferPointers hack #10326
Conversation
19006a8
to
0a5be0d
Compare
void SetReplyStatus(const NTSTATUS Status); | ||
void SetReplyInformation(const ULONG_PTR pInformation); | ||
|
||
// DO NOT PUT ACCESS SPECIFIERS HERE. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
{ | ||
delete _pWaiter; | ||
} | ||
delete _pWaiter; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ho. Sneaky sneaky.
0a5be0d
to
29d9365
Compare
Force push to resolve merge conflicts in |
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Would you mind running the |
Oh, you'll need to set appverifier for |
All host feature tests completed successfully. 🙂 |
Hello @DHowett! Because this pull request has the 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 (
|
🎉 Handy links: |
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 aunion 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
Closes #10076