`[In, Out] char*` parameter MUST NOT generate `string` as a friendly overload about cswin32 HOT 15 CLOSED

I've seen warnings in various discussion places (maybe by Tanner and others if I remember right) that made me realize my code that mutates new string instances could break in the future.

Right. The runtime has investigated automatic string interning and reserves the right to enable it in any release.
Strings are immutable, by design, and attempting to mutate them (particularly literals) can result in weird behaviors.

My favorite example is:

using System;

PseudoNativeCall("Hello, World!");
Console.WriteLine("Hello, World!");

unsafe static void PseudoNativeCall(string s)
{
    fixed (char* c = s)
    {
        c[0] = 'J';
    }
}

which prints Jello, World!.

Likewise, there is no guarantee the mutation will succeed. The runtime is free to place literals and the IL, etc in read only memory in which case mutating it may result in an AccessViolationException.
In some cases (such as on more locked down platforms) this may even be the default scenario. Apple Silicon, the ARM64 variant, for example requires that executable code not be writable and so we had to add such support and begin enforcing it for that platform.

If you really must mutate a string, such as for efficient allocation, we provide String.Create which provides a SpanAction callback allowing you to safely mutate before the memory becomes "immutable" and is open to being placed in readonly memory or subject to string interning.

from cswin32.

For reference, we also block [Out] string (which is distinct from out string or [Out] out string) as of recent .NET Core: dotnet/coreclr#21513

https://docs.microsoft.com/en-us/dotnet/standard/native-interop/best-practices should be up to date with the modern best practices

from cswin32.

If you have links to share, I'd love to learn what is planned around new string that you mention.

https://github.com/dotnet/runtime/blob/master/docs/design/features/StringDeduplication.md

I've seen warnings in various discussion places (maybe by Tanner and others if I remember right) that made me realize my code that mutates new string instances could break in the future.

from cswin32.

@jnm2 Let's follow up on the span discussion over at #144

from cswin32.

in this case mutating what was expected to be an immutable string.

Yikes! This is definitely a must-fix.

from cswin32.

This should apply to any [Out] string (anything documented as mutating the string)

from cswin32.

@AArnott How do I consume the bleeding edge to test this fix? There was talk about [Out] strings but that isn't applicable to this API so want to take the fix for a spin.

from cswin32.

@riverar Testing this would be great. Thank you.

The readme includes the link and instructions to consume our daily builds. You'll want to look for a package version of 0.1.372 or later to test this fix. The daily build that includes this change will come out in about 5 hours.

from cswin32.

@AArnott Well I verified the string overload is gone, so bug fixed I suppose. But now there's just an incredibly difficult to use PWSTR in its place that I can't seem to initialize to a value without tainting my code with a char*. 😑

from cswin32.

I guess the scenario is now:

var notImmutableIconLocation = "icon.dll,1";
unsafe
{
    fixed (char* ptr = notImmutableIconLocation)
    {
        PInvoke.PathParseIconLocation(ptr);
    }
}

And the user just has to remember notImmutableIconLocation is not an immutable string? Is there another pattern I'm missing here?

from cswin32.

I believe it is never safe to mutate a string because the runtime interns (deduplicates) string instances and IIRC there are plans in the works to make even new string overloads return existing pooled instances.

var notImmutableIconLocation = "icon.dll,1".ToCharArray(); Using a char array is more semantically correct in my understanding.

from cswin32.

the runtime interns (deduplicates) string instances

I've never heard that it does. And some substantial libraries I know go to lengths to intern strings themselves which wouldn't be necessary if the CLR did it. The BCL itself has a public interning method.
@jnm2 If you have links to share, I'd love to learn what is planned around new string that you mention.

@riverar no, don't pass string into a mutable function. That defeats the point of the change. What @jnm2 suggests is counter to the docs, which require a string of MAX_PATH in length in this case. Here is what I would do:

private unsafe (string Path, int Index) PathParseIconLocation(string pathAndIndex)
{
    const int MAX_PATH = 260;
    Span<char> szIconFile = stackalloc char[MAX_PATH];
    pathAndIndex.AsSpan().CopyTo(szIconFile);
    fixed (char* pszIconFile = szIconFile)
    {
        int index = PInvoke.PathParseIconLocation(pszIconFile);
        return (new string(pszIconFile), index);
    }
}

from cswin32.

If the friendly overload accepted a StringBuilder, the code could look like this:

private unsafe (string Path, int Index) PathParseIconLocation(string pathAndIndex)
{
    const int MAX_PATH = 260;
    var szIconFile = new StringBuilder(pathAndIndex, MAX_PATH);
    int index = PInvoke.PathParseIconLocation(szIconFile);
    return (szIconFile.ToString(), index);
}

That would incur an extra allocation, but that's a common cost of the friendly overloads, so probably an ok cost.

from cswin32.

See #144

from cswin32.

So we've come full circle to my original report more or less 😄

Opinion: I'm also starting to wonder if all this hoop jumping truly helps anyone. Without a documented problem and supporting data, it really feels like we are micro-optimizing perf over language safety features and C# developer sanity.

from cswin32.