[In, Out] char* parameter MUST NOT generate string as a friendly overload

[riverar]

This API takes in a LPWStr, scribbles a NUL on it, and returns the numeric component.

Shlwapi!PathParseIconLocationW per headers is defined as such:

int PathParseIconLocationW(
  LPWSTR pszIconFile
);

Win32 metadata describes it as such:

[DllImport("SHLWAPI", ExactSpelling = true)]
public unsafe static extern int PathParseIconLocationW(
  [In] [Out] [NativeTypeInfo(UnmanagedType.LPWStr)] ushort* pszIconFile);

CsWin32 generated:

[DllImport("ShlwApi", ExactSpelling = true, EntryPoint = "PathParseIconLocationW")]
internal static extern unsafe int PathParseIconLocation([In, Out] char *pszIconFile);
// ...
internal static unsafe int PathParseIconLocation(string pszIconFile)
{
    fixed (char *pszIconFileLocal = pszIconFile)
    {
        return PInvoke.PathParseIconLocation(pszIconFileLocal);
    }
}

A simpler, safer approach:

[DllImport("shlwapi.dll", ExactSpelling = true, PreserveSig = true)]
internal static extern int PathParseIconLocationW(
    [MarshalAs(UnmanagedType.LPWStr)]
    StringBuilder pszIconFile);

We might be able to get away with using StringBuilder in all cases where metadata describes a parameter as [In, Out] char *. But I haven't had my coffee yet to say so definitively.

This is also a great example of how these wrappers dangerously hide the side effects of unsafe, in this case mutating what was expected to be an immutable string.

[AArnott]

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

Yikes! This is definitely a must-fix.

[tannergooding]

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

[riverar]

@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.

[AArnott]

@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.

[riverar]

@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*. 😑

[riverar]

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?

[jnm2]

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.

[AArnott]

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);
    }
}

[AArnott]

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.

[AArnott]

See #144

[jnm2]

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.

[riverar]

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.

[AArnott]

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

[tannergooding]

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.

[tannergooding]

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