Bug if native strings are moved in memory

[Louis-Simon22]

Hello,

Using the following code in the Runtime.CString, if the native strings are moved in memory, but their values are not changed, then it either complains about duplicate values in PointersToStrings or it points to garbage data.

   public static string String(CString value)
    {
        if (value.IsNull)
        {
            return string.Empty;
        }

        if (PointersToStrings.TryGetValue(value._pointer, out var result))
        {
            return result;
        }

        var hash = Djb2((byte*)value._pointer);
        if (StringHashesToPointers.TryGetValue(hash, out var pointer2))
        {
            result = PointersToStrings[pointer2._pointer];
            return result;
        }

        // calls ASM/C/C++ functions to calculate length and then "FastAllocate" the string with the GC
        // https://mattwarren.org/2016/05/31/Strings-and-the-CLR-a-Special-Relationship/
        result = Marshal.PtrToStringAnsi(value._pointer);

        if (string.IsNullOrEmpty(result))
        {
            return string.Empty;
        }

        StringHashesToPointers.Add(hash, value);
        PointersToStrings.Add(value._pointer, result);

        return result;
    }

If I manually disable the cache, the problems go away. I think there should be a way to disable this caching and a warning about its requirements in the documentation.

[lithiumtoast]

Thanks for the bug report! I encountered this myself before when working with libclang. If you wish to have caching still the workaround is to call FreeString or one the similar functions.

In the next version of C2CS I will add an option to disable string caching.

Would Marshal.PtrToStringAnsi be called every time then?

[Louis-Simon22]

I think calling every time is kind of unavoidable if the native strings can be moved

[lithiumtoast]

Okay. I'm concerned about the implicit/explicit operator causes allocation of memory; perhaps I'll remove them if caching can be disabled. What do you think?

[Louis-Simon22]

Explicit operators doing allocation does not bother me, but implicit can be more tricky because people might not be aware or know it happens. I didn't even think that there was a cache until I encountered this bug.

We might be able to keep the cache if the key is the pointer and the hash, but it might not be worth it performance-wise.

[lithiumtoast]

Discussion on flecs discord, in the next version of C2CS:

• Caching will be disabled by default. It's upon the developer using the bindings to deal with the lifetime of C strings and other unmanaged memory correctly. I understand that this is an uphill battle because the average C# developer doesn't know how to do unmanaged or "unsafe" C# code, but multiple people have requested for this.
• Documentation will be added on unmanaged memory of strings from C to C# and vice versa. This will include best practices including ways to reduce memory pressure of the garbage collector in C# for performance reasons.

[Louis-Simon22]

Sounds good, thank you.

If anyone encounters this in the current version, you can manually edit the Runtime.CStrings.String function for something like this:

public static string String(CString value)
{
    if (value.IsNull)
    {
        return string.Empty;
    }

    // calls ASM/C/C++ functions to calculate length and then "FastAllocate" the string with the GC
    // https://mattwarren.org/2016/05/31/Strings-and-the-CLR-a-Special-Relationship/
    var result = Marshal.PtrToStringAnsi(value._pointer);

    if (string.IsNullOrEmpty(result))
    {
        return string.Empty;
    }

    return result;
}

Remember to do the same for the StringWide if needed

[lithiumtoast]

fixed and released under v4.0.2