starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-09-25 00:15:10 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

Missing operator delete/delete[] for some allocated objects #1185

New Issue
Open
opened 2026-01-30 22:18:20 +00:00 by claunia · 7 comments
claunia commented 2026-01-30 22:18:20 +00:00
Owner
Copy Link

Originally created by @dreamer-dead on GitHub (May 17, 2019).

Hi.

Looks like there are few places in the Terminal code where I can suspect a memory leaks due to missing operator delete or operator delete[].
These allocation sites can be grouped into following:

Here is my gist with quite a simple grep foroperator new calls - https://gist.github.com/dreamer-dead/c43248466fc4d445378b4dfba80d8d25

Is this an intentional behaviour or a bug that should be fixed?

Originally created by @dreamer-dead on GitHub (May 17, 2019). Hi. Looks like there are few places in the Terminal code where I can suspect a memory leaks due to missing `operator delete` or `operator delete[]`. These allocation sites can be grouped into following: - raw `operator new` without `operator delete` like this https://github.com/Microsoft/Terminal/blob/master/src/cascadia/TerminalConnection/ConptyConnection.cpp#L191 - allocation by `operator new` some global object or class member (https://github.com/Microsoft/Terminal/blob/master/src/cascadia/TerminalControl/TermControl.cpp#L281) - allocation by `operator new` some memory in a function and return it by raw pointer, which can leak then. Here is my gist with quite a simple grep for`operator new` calls - https://gist.github.com/dreamer-dead/c43248466fc4d445378b4dfba80d8d25 Is this an intentional behaviour or a bug that should be fixed?
claunia added the Help WantedProduct-ConhostIssue-BugArea-CodeHealth labels 2026-01-30 22:18:20 +00:00
claunia commented 2026-01-30 22:18:21 +00:00
Author
Owner
Copy Link

@fghzxm commented on GitHub (May 18, 2019):

I don't think that could be intentional. Plus, we should probably use std::unique_ptr<T[]> or something similar regardless whether we want the allocated memory to be long-lived.

@fghzxm commented on GitHub (May 18, 2019): I don't think that could be intentional. Plus, we should probably use `std::unique_ptr<T[]>` or something similar regardless whether we want the allocated memory to be long-lived.
claunia commented 2026-01-30 22:18:21 +00:00
Author
Owner
Copy Link

@zadjii-msft commented on GitHub (May 20, 2019):

I don't believe that these are intentional at all. They might just be oversights.

  • The ConptyConnection one I'm not terribly worried about currently, since that's not actually compiled in right now.
  • The TermControl's Terminal should probably be a unique_ptr - that's DEFINITELY wrong.
  • many of the other ones in conhost are usually objects that don't change during the lifetime of the application, but that doesn't make them right. Other ones are just places of the code that haven't been modernized yet.
@zadjii-msft commented on GitHub (May 20, 2019): I don't believe that these are intentional at all. They might just be oversights. * The `ConptyConnection` one I'm not terribly worried about currently, since that's not actually compiled in right now. * The TermControl's `Terminal` should probably be a `unique_ptr` - that's DEFINITELY wrong. * many of the other ones in conhost are usually objects that don't change during the lifetime of the application, but that doesn't make them right. Other ones are just places of the code that haven't been modernized yet.
claunia commented 2026-01-30 22:18:21 +00:00
Author
Owner
Copy Link

@DHowett-MSFT commented on GitHub (May 20, 2019):

... Terminal should probably be a unique_ptr ...

Could this be germane to #745?

@DHowett-MSFT commented on GitHub (May 20, 2019): > ... `Terminal` should probably be a `unique_ptr` ... Could this be germane to #745?
claunia commented 2026-01-30 22:18:22 +00:00
Author
Owner
Copy Link

@zadjii-msft commented on GitHub (May 20, 2019):

It's real possible. That makes a ton of sense. IDK why that was never caught during review

@zadjii-msft commented on GitHub (May 20, 2019): It's real possible. That makes a ton of sense. IDK why that was never caught during review
claunia commented 2026-01-30 22:18:22 +00:00
Author
Owner
Copy Link

@dlong11 commented on GitHub (Jun 1, 2019):

Just a note (even though this isn't compiled in) - this one is a false positive.
https://github.com/Microsoft/Terminal/blob/master/src/cascadia/TerminalConnection/ConptyConnection.cpp#L191
DeleteProcThreadAttributeList is called at the end of the function. Even though it is "correct" it looks like this could still be cleaned up to be exception safe. (Also shouldn't this be a HeapAlloc instead of new?) (Looks like a low priority though, based on DHowett-MSFT comment. )

@dlong11 commented on GitHub (Jun 1, 2019): Just a note (even though this isn't compiled in) - this one is a false positive. https://github.com/Microsoft/Terminal/blob/master/src/cascadia/TerminalConnection/ConptyConnection.cpp#L191 DeleteProcThreadAttributeList is called at the end of the function. Even though it is "correct" it looks like this could still be cleaned up to be exception safe. (Also shouldn't this be a HeapAlloc instead of new?) (Looks like a low priority though, based on DHowett-MSFT comment. )
claunia commented 2026-01-30 22:18:22 +00:00
Author
Owner
Copy Link

@dreamer-dead commented on GitHub (Jun 6, 2019):

@dlong11 are you sure that calling DeleteProcThreadAttributeList is enough to free the memory block?
My opinion is that if InitializeProcThreadAttributeList does not allocate memory, then DeleteProcThreadAttributeList should not free it. And even more, DeleteProcThreadAttributeList definitely can't free memory allocated by the operator new because it's a pure C function.

Looks like the Chromium authors think the same, see https://chromium.googlesource.com/chromium/src/+/master/base/win/startup_information.cc#62

@dreamer-dead commented on GitHub (Jun 6, 2019): @dlong11 are you sure that calling `DeleteProcThreadAttributeList` is enough to free the memory block? My opinion is that if `InitializeProcThreadAttributeList` does not allocate memory, then `DeleteProcThreadAttributeList` should not free it. And even more, `DeleteProcThreadAttributeList` definitely can't free memory allocated by the `operator new` because it's a pure C function. Looks like the Chromium authors think the same, see https://chromium.googlesource.com/chromium/src/+/master/base/win/startup_information.cc#62
claunia commented 2026-01-30 22:18:22 +00:00
Author
Owner
Copy Link

@dlong11 commented on GitHub (Jun 6, 2019):

@dreamer-dead I saw that "new" to and it was a red flag.

Also shouldn't this be a HeapAlloc instead of new?

@dlong11 are you sure that calling DeleteProcThreadAttributeList is enough to free the memory block?

You are indeed correct. Sorry for the misinformation. What a terrible API. 😄

@dlong11 commented on GitHub (Jun 6, 2019): @dreamer-dead I saw that "new" to and it was a red flag. > Also shouldn't this be a HeapAlloc instead of new? > @dlong11 are you sure that calling DeleteProcThreadAttributeList is enough to free the memory block? You are indeed correct. Sorry for the misinformation. What a terrible API. 😄
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
⛺ Reserved
A11yCO
A11yMAS
A11ySev1
A11ySev2
A11ySev3
A11yTTValidated
A11yUsable
A11yVoiceAccess
A11yWCAG
Area-Accessibility
Area-AtlasEngine
Area-AzureShell
Area-Build
Area-Build
Area-Chat
Area-CmdPal
Area-CodeHealth
Area-Commandline
Area-CookedRead
Area-DefApp
Area-Extensibility
Area-Fonts
Area-GroupPolicy
Area-i18n
Area-Input
Area-Interaction
Area-Interop
Area-Localization
Area-Output
Area-Performance
Area-Portable
Area-Quality
Area-Remoting
Area-Rendering
Area-Schema
Area-Server
Area-Settings
Area-SettingsUI
Area-ShellExtension
Area-ShellExtension
Area-ShellExtension
Area-Suggestions
Area-Suggestions
Area-TerminalConnection
Area-TerminalControl
Area-Theming
Area-UserInterface
Area-VT
Area-Windowing
Area-WPFControl
AutoMerge
Blocking-Ingestion
Culprit-Centennial
Culprit-WinUI
Disability-All
Disability-Blind
Disability-LowVision
Disability-Mobility
External-Blocked-WinUI3
Fixed
Gathering-Data
good first issue
HCL-E+D
HCL-WindowsTerminal
Help Wanted
Impact-Compatibility
Impact-Compliance
Impact-Correctness
Impact-Visual
In-PR
InclusionBacklog
InclusionBacklog-Windows TerminalWin32
InclusionCommitted-202206
Issue-Bug
Issue-Docs
Issue-Feature
Issue-Feature
Issue-Question
Issue-Samples
Issue-Scenario
Issue-Task
Needs-Attention
Needs-Author-Feedback
Needs-Bisect
Needs-Discussion
Needs-Repro
Needs-Tag-Fix
Needs-Tag-Fix
Needs-Triage
No-Recent-Activity
Priority-0
Priority-1
Priority-2
Priority-3
Product-Cmd.exe
Product-Colortool
Product-Colortool
Product-Colortool
Product-Conhost
Product-Conpty
Product-Meta
Product-Powershell
Product-Terminal
Product-WSL
pull-request
Mirrored from GitHub Pull Request
 Resolution-Answered
Resolution-By-Design
Resolution-Duplicate
Resolution-External
Resolution-Fix-Available
Resolution-Fix-Committed
Resolution-No-Repro
Resolution-Won't-Fix
Severity-Blocking
Severity-Crash
Severity-DataLoss
spam
this-will-be-a-breaking-change
Tracking-External
WindowsTerminal_Win32
Work-Item
zAskModeBug
zInbox-Bug
No Label Area-CodeHealth Help Wanted Issue-Bug Product-Conhost
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
claunia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/terminal#1185
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?