Fix a memory leak in onecore interactivity #12340
Merged
+1
−9
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As noted in #6759: > `RtlCreateUnicodeString` creates a copy of the string on the process heap and the `PortName` variable has local-scope. The string doesn't get freed with `RtlFreeUnicodeString` before the function returns creating a memory leak. > `CIS_ALPC_PORT_NAME` is a constant string and the `PortName` variable should instead be initialized using the `RTL_CONSTANT_STRING` macro: > > ```c++ > static UNICODE_STRING PortName = RTL_CONSTANT_STRING(CIS_ALPC_PORT_NAME); > ``` I actually built this in the OS repo to make sure it'll still build, because this code doesn't even build outside Windows. * [x] Closes #6759 * I work here.
msftbot
bot
added
Area-CodeHealth
lhecker
approved these changes
Feb 7, 2022
| NTSTATUS Status = STATUS_SUCCESS; | ||
|
|
||
| // Port handle and name. | ||
| HANDLE PortHandle; | ||
| UNICODE_STRING PortName; | ||
| static UNICODE_STRING PortName = RTL_CONSTANT_STRING(CIS_ALPC_PORT_NAME); |
There was a problem hiding this comment.
static constexpr? If not then static const.
There was a problem hiding this comment.
if constexpr is possible, let's do it. I can validate this in a quick razzle build (if @zadjii-msft wants that!)
There was a problem hiding this comment.
This requires a deadly const_cast -- if that is okay, let's do constexpr!
diff --git a/onecore/windows/core/console/open/src/interactivity/onecore/ConIoSrvComm.cpp b/onecore/windows/core/console/open/src/interactivity/onecore/ConIoSrvComm.cpp
index cdeebed38c365..6c1b4ffc465b0 100644
--- a/onecore/windows/core/console/open/src/interactivity/onecore/ConIoSrvComm.cpp
+++ b/onecore/windows/core/console/open/src/interactivity/onecore/ConIoSrvComm.cpp
@@ -153,7 +145,7 @@
// Request to connect to the server.
ConnectionMessageLength = sizeof(CIS_MSG);
Status = NtAlpcConnectPort(&PortHandle,
- &PortName,
+ const_cast<PUNICODE_STRING>(&PortName),
NULL,
&PortAttributes,
ALPC_MSGFLG_SYNC_REQUEST,There was a problem hiding this comment.
In that case I'd personally use a regular static... Better safe than sorry, since it's possible it might do reference counting on it, right?
lhecker
added
the
AutoMerge
|
Hello @lhecker! 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 (
|
DHowett
pushed a commit
that referenced
this issue
Mar 24, 2022
As noted in #6759: > `RtlCreateUnicodeString` creates a copy of the string on the process heap and the `PortName` variable has local-scope. The string doesn't get freed with `RtlFreeUnicodeString` before the function returns creating a memory leak. > `CIS_ALPC_PORT_NAME` is a constant string and the `PortName` variable should instead be initialized using the `RTL_CONSTANT_STRING` macro: > > ```c++ > static UNICODE_STRING PortName = RTL_CONSTANT_STRING(CIS_ALPC_PORT_NAME); > ``` I actually built this in the OS repo to make sure it'll still build, because this code doesn't even build outside Windows. * [x] Closes #6759 * I work here. (cherry picked from commit 349b767)
|
Handy links: |
|
Handy links: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
As noted in #6759:
I actually built this in the OS repo to make sure it'll still build, because this code doesn't even build outside Windows.