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

ConIoSrvComm::Connect memory leak #6759

Closed
dmex opened this issue Jul 2, 2020 · 3 comments · Fixed by #12340
Closed

ConIoSrvComm::Connect memory leak #6759

dmex opened this issue Jul 2, 2020 · 3 comments · Fixed by #12340
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal v1.14

Comments

@dmex
Copy link

dmex commented Jul 2, 2020

The ConIoSrvComm::Connect function has a memory leak from using the RtlCreateUnicodeString function incorrectly:

terminal/src/interactivity/onecore/ConIoSrvComm.cpp

Lines 101 to 106 in 396cbbb

// Initialize the server port name.
Ret = RtlCreateUnicodeString(&PortName, CIS_ALPC_PORT_NAME);
if (!Ret)
{
return STATUS_NO_MEMORY;
}

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:

static UNICODE_STRING PortName = RTL_CONSTANT_STRING(CIS_ALPC_PORT_NAME);

It could alternatively use RtlInitUnicodeString but since the string is hard-coded it should be using the macro.
@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 Jul 2, 2020
@DHowett
Copy link
Member

DHowett commented Jul 2, 2020

Hey, good find! It's nice to have someone looking at this code, especially given that it doesn't even build in the solution today 😄

@DHowett DHowett added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase labels Jul 2, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jul 2, 2020
@DHowett DHowett added this to the 21H1 milestone Jul 2, 2020
@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jul 2, 2020
@zadjii-msft zadjii-msft modified the milestones: Windows vNext, 22H1 Jan 4, 2022
@zadjii-msft zadjii-msft modified the milestones: 22H1, Terminal v1.14 Feb 2, 2022
zadjii-msft added a commit that referenced this issue Feb 3, 2022
@zadjii-msft
Fix a memory leak in onecore interactivity
aa93222 
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.
@zadjii-msft zadjii-msft mentioned this issue Feb 3, 2022
Merged
1 task
@ghost ghost added the In-PR This issue has a related PR label Feb 3, 2022
@ghost ghost closed this as completed in #12340 Feb 15, 2022
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Feb 15, 2022
ghost pushed a commit that referenced this issue Feb 15, 2022
@zadjii-msft
Fix a memory leak in onecore interactivity (#12340)
349b767 
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.
DHowett pushed a commit that referenced this issue Mar 24, 2022
@zadjii-msft @DHowett
Fix a memory leak in onecore interactivity (#12340)
6cd3e6a 
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)
@ghost
Copy link

ghost commented May 24, 2022

🎉This issue was addressed in #12340, which has now been successfully released as Windows Terminal v1.13.1143.:tada:

Handy links:

@ghost
Copy link

ghost commented May 24, 2022

🎉This issue was addressed in #12340, which has now been successfully released as Windows Terminal Preview v1.14.143.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal v1.14
Development

Successfully merging a pull request may close this issue.

Fix a memory leak in onecore interactivity microsoft/terminal
3 participants
@DHowett @dmex @zadjii-msft