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.

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

char* members in value types marshalled as string result in a memory leak #1785

Closed
Saalvage opened this issue Oct 20, 2023 · 4 comments · Fixed by #1787
Closed

char* members in value types marshalled as string result in a memory leak #1785

Saalvage opened this issue Oct 20, 2023 · 4 comments · Fixed by #1787

Comments

@Saalvage
Copy link
Contributor

The following type

CS_VALUE_TYPE class DLL_API ValueType {
public:
    const char* char_ptr_member;
};

results in a memory leak when the following code is executed:

while (true) {
    var test = new ValueType();
    test.CharPtrMember = "Looooooooooooong string";
}

The set accessor allocates memory which is never freed outside of the set accessor.

At this time I see three options to solve this, listed in presumed increasing level of complexity:

  1. Disabling char* to string marshalling for value type members
  2. Using IDisposable
  3. Wrapping the unmanaged memory in a managed class that cleans up the memory when it gets finalized and having that class be an additional member of the value type.
@tritao
Copy link
Collaborator

tritao commented Oct 20, 2023

Can you post the generated code for the string setter? I wonder if we have a 4th option, which is to optimize the string marshaling so it does not allocate at all.

Edit: Or is the issue that native destructor for the value type object is never called? If that's the case, then I think it should be IDisposable like classes are I think?

@Saalvage
Copy link
Contributor Author

Here's the setter.

set
{
    if (__char_ptr_member_OwnsNativeMemory)
        Marshal.FreeHGlobal(__instance.char_ptr_member);
    __char_ptr_member_OwnsNativeMemory = true;
    if (value == null)
    {
        __instance.char_ptr_member = global::System.IntPtr.Zero;
        return;
    }
    var __bytes0 = global::System.Text.Encoding.UTF8.GetBytes(value);
    var __bytePtr0 = Marshal.AllocHGlobal(__bytes0.Length + 1);
    Marshal.Copy(__bytes0, 0, __bytePtr0, __bytes0.Length);
    Marshal.WriteByte(__bytePtr0 + __bytes0.Length, 0);
    __instance.char_ptr_member = (__IntPtr) __bytePtr0;
}

Seems like IDisposable seems most sensible here!

@Saalvage
Copy link
Contributor Author

After some more testing, if the string is long enough the same issues presents itself with std::string members, when the assigned string is long enough (longer than the short-string buffer). So the dtor needs to be called as well as the unmanaged memory freed up.

@tritao
Copy link
Collaborator

tritao commented Oct 20, 2023

Ok, this helps. I was mistakenly thinking you were assigning to a native std::string for some reason, which I thought would deal itself with keeping ownership of the string data. In this case, I think we need to have IDisposable indeed.

After some more testing, if the string is long enough the same issues presents itself with std::string members, when the assigned string is long enough (longer than the short-string buffer). So the dtor needs to be called as well as the unmanaged memory freed up.

Damn, more bugs! 😱

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants