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

ComInterfaceGenerator generates cleanup stage for stateless marshalled parameters when it shouldn't #85795

Closed
jtschuster opened this issue May 4, 2023 · 2 comments · Fixed by #86415
Closed

ComInterfaceGenerator generates cleanup stage for stateless marshalled parameters when it shouldn't #85795

jtschuster opened this issue May 4, 2023 · 2 comments · Fixed by #86415
Assignees
@jkoritzinsky
Labels
area-System.Runtime.InteropServices

Comments

@jtschuster
Copy link
Member

jtschuster commented May 4, 2023 •
edited
Loading

Parameters marshalled by stateless marshallers are being freed in too many scenarios. They should only be freed in unmanaged to managed ref scenarios (on the pointer that was passed in, not the value that is returned at the end of the method. In and Out parameters should not be cleaned up. Stateful marshallers can have free called on them as they can know which MarshalMode they are in and decide to actually free the underlying memory or not.
@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 4, 2023
@ghost
Copy link

ghost commented May 4, 2023

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Issue Details

Parameters marshalled by stateless marshallers are being freed in too many scenarios. They should only be freed in unmanaged to managed ref scenarios (on the pointer that was passed in, not the value that is returned at the end of the method. In and Out parameters should not be cleaned up.

Author: jtschuster
Assignees: -
Labels:

area-System.Runtime.InteropServices
Milestone: -

@jtschuster jtschuster removed the untriaged New issue has not been triaged by the area owner label May 4, 2023
@jkoritzinsky
Copy link
Member

For stateful marshallers specifically, they may also be tracking additional state for an unmanaged->managed in/out/ref scenario that needs to be freed in addition to/instead of the unmanaged value.

@jkoritzinsky jkoritzinsky mentioned this issue May 10, 2023
Merged
@jkoritzinsky jkoritzinsky self-assigned this May 17, 2023
@jkoritzinsky jkoritzinsky mentioned this issue May 17, 2023
Merged
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 17, 2023
jkoritzinsky added a commit to jkoritzinsky/runtime that referenced this issue May 19, 2023
@jkoritzinsky
Refactor our linear collection marshalling implementation
5b8bebb 
Refactor the elements marshalling logic to split out space allocation and source/destination span construction from the elements marshalling logic. Pull out the element marshalling logic to a common base class and separate it out from the actual marshalling strategies.

This refactoring is required to provide customization points to correctly fix dotnet#85795.

I've validated that this change is a zero-diff change on all of the generated code for the LibraryImportGenerator integration tests.
@jkoritzinsky jkoritzinsky mentioned this issue May 19, 2023
Merged
jkoritzinsky added a commit to jkoritzinsky/runtime that referenced this issue May 19, 2023
@jkoritzinsky
Only call Free in unmanaged->managed stubs when ownership has been tr…
616d53f 
…ansfered to the callee

Fixes dotnet#85795
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 23, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@jkoritzinsky jkoritzinsky

Labels
area-System.Runtime.InteropServices
Projects
None yet
Milestone
No milestone
2 participants
@jkoritzinsky @jtschuster