Skip to content

Conversation

@jkoritzinsky
Copy link
Member

Fix a case where we accidentally leaked a COM object reference in System.DirectoryServices.AccountManagement.

I validated that there's no other cases where we missed a leak in the rest of the codebase. The rest of the cases where we use COM interop either didn't have this issue or get their COM object references through COM Activation, so they don't go through LibraryImport at all.

Fixes #83269

@ghost
Copy link

ghost commented Jul 31, 2023

Tagging subscribers to this area: @dotnet/area-system-directoryservices, @jay98014
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix a case where we accidentally leaked a COM object reference in System.DirectoryServices.AccountManagement.

I validated that there's no other cases where we missed a leak in the rest of the codebase. The rest of the cases where we use COM interop either didn't have this issue or get their COM object references through COM Activation, so they don't go through LibraryImport at all.

Fixes #83269

Author: jkoritzinsky
Assignees: -
Labels:

area-System.DirectoryServices

Milestone: -

@jkoritzinsky jkoritzinsky requested a review from ericstj July 31, 2023 22:12
@jkoritzinsky jkoritzinsky added this to the 8.0.0 milestone Jul 31, 2023
{
if (ppObjPtr != IntPtr.Zero)
{
Marshal.Release(ppObjPtr);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to add a unit test for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know enough about this space to add a unit test and testing specifically if we leak a COM object here is difficult.

Copy link
Contributor

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

LGTM, it would be great if we could add a unit test. Also, I believe we want to port it into 7.0

@jkoritzinsky
Copy link
Member Author

/backport to release/7.0-staging

@jkoritzinsky jkoritzinsky merged commit b039eab into dotnet:main Aug 1, 2023
@jkoritzinsky jkoritzinsky deleted the leaked-release branch August 1, 2023 17:00
@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2023

Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/5729686954

@jkoritzinsky jkoritzinsky restored the leaked-release branch August 1, 2023 17:00
@jkoritzinsky jkoritzinsky deleted the leaked-release branch August 1, 2023 17:45
@ghost ghost locked as resolved and limited conversation to collaborators Aug 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

2 participants