Friendly overload should expose [In, Out] PWSTR in more friendly way

[AArnott]

Today, accessing such a method requires:

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

With StringBuilder in its friendly overload it reduces to just 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);
}

Based on feedback from @riverar.

[jnm2]

Would you agree that Span<char> would be friendly and wouldn't demand an allocation?

Used without unsafe something like:

private (string Path, int Index) PathParseIconLocation(string pathAndIndex)
{
    const int MAX_PATH = 260;
    var szIconFile = new char[MAX_PATH];
    pathAndIndex.CopyTo(szIconFile, 0);
    int index = PInvoke.PathParseIconLocation(szIconFile);
    return (new string(szIconFile), index);
}

Span<char> doesn't demand heap allocations if you pool the array or the memory, or if you use unsafe + stackalloc.

My second choice if Span<char> is not available would be char[], before StringBuilder.

[riverar]

Put all that gunk in the wrapper. I should be able to just pass in a StringBuilder and call it a day. Why does everyone have to do this extra work? And why is everyone so focused on removing an alloc and not the ancillary effects like increasing expended cpu cycles or image size?

[AArnott]

stackalloc assigned to span does not require unsafe. And it is very tempting to add Span<char> for mutable strings. The problem is that span implies a length, but where native code does not take a length, that implication is misleading. Suppose for example you provide a Span<char> with a length of 5, whose content is "hello". Sound good so far? Well, it's bad, because the native code that didn't take a length assumes a null-terminated string, and there is no justification in as assumption that two null bytes (a UTF-16 null character) follows that span in memory, which would lead to malfunction in the native code.

So where the native code takes a pointer and a length we can and do take Span<T> or ReadOnlySpan<T>. Otherwise we take pointers (or a StringBuilder, after this issue) so that null termination requirements are more obvious.

[AArnott]

And why is everyone so focused on removing an alloc and not the ancillary effects like increasing expended cpu cycles or image size?

None of these alternatives to StringBuilder involve additional CPU cycles. On the contrary: StringBuilder undoubtedly consumes the most CPU. Do not confuse less user code with less CPU time. The alternatives discussed here do only less than StringBuilder would do internally.

Image size would not increase appreciably either.

I suspect the reason @jnm2 is still suggesting alternatives to StringBuilder is that StringBuilder incurs multiple allocations whereas char[] would incur just one. However whether it's 1 or 3 allocations, it's all relatively minimal, so IMO we should produce the easiest API to get right, given we already offer the low-level method that takes pointers so if you really are counting allocations you can always do the stackalloc thing and pass in a pointer and have 0 allocations.

[jnm2]

Does StringBuilder guarantee that the built string is null-terminated without exceeding the initialized StringBuilder capacity? If not, then I don't understand how StringBuilder is less of a pitfall than Span<char> or char[].

I see StringBuilder as extra work. Usually it feels like I'm doing more work and writing more code than I really want. Unless I have at least two CopyTo calls, which is rare, StringBuilder always feels like it's less user friendly to me.

It would be a plus if the friendly overload was also capable of avoiding allocations if desired, not just the pointer overload, but that's not the driving factor for me. The driving factor for me is that StringBuilder is not as nice to use as stackalloc/new char[...] with the majority of the APIs that I recall using. Clearly, opinions diverge on this. :D

[AArnott]

@jnm2 FYI: this is legal C# and would work if we offered a Span<char> overload. Note the lack of unsafe anywhere:

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

[jnm2]

@AArnott Yes, I picked up that you said that. I like that.

[AArnott]

It would be a plus if the friendly overload was also capable of avoiding allocations if desired, not just the pointer overload

I agree. In the dotnet/pinvoke project we auto-generated several friendly overloads in order to appease several use cases. However this became a real problem in that the overloads were sometimes so similar that arguments sometimes were not sufficient to guide the C# compiler to disambiguate which overload to call, which ironically made the method harder overall. So at the moment I am searching for a nice middle-ground between usability and efficiency that requires only 1 friendly overload. This is an area we're still learning from though, so we may add more friendly overloads or modify signatures on the existing ones in the future.

Does StringBuilder guarantee that the built string is null-terminated

You can read about StringBuilder interop here, where it documents it always allocates capacity + 1 in order to allow for native interop that will write null terminators.

I need to research however whether the string it represents is always contiguous when I get a pointer to it (some evidence suggests that is not the case) or whether the content when initialized by managed code is always null terminated.

[AArnott]

To be clear, .NET supports marshaling StringBuilder natively, but as we have a goal to have just one extern method, and that takes pointers, our friendly overload is on the hook to reproduce any marshaling steps that the runtime normally would have done.

[AArnott]

Just found this doc which says:

AVOID StringBuilder parameters. StringBuilder marshaling always creates a native buffer copy. As such, it can be extremely inefficient.

It elaborates further if you care to read about it. It pitches use of char[]. So I'll noodle on this a bit more. I wouldn't do char[] at least wherever Span<char> is an option (see #117) as the latter works everywhere char[] does as an input parameter and enables you to skip allocating an array on the heap for short arrays. And both arrays and spans share the same risk of implying a length that may not be communicated to the native side.

[jnm2]

You can read about StringBuilder interop here, where it documents it always allocates capacity + 1 in order to allow for native interop that will write null terminators.

That page doesn't answer my question. What happens if the text you append exceeds the initial capacity? Then there might be no zero terminator within the buffer length that the API expects. Is that not a pitfall that puts StringBuilder on more even footing with Span<char> in terms of zero termination pitfalls?

[AArnott]

I know it doesn't answer your question. I already stated that I need to research...

whether the content when initialized by managed code is always null terminated

[AArnott]

And anyway, as the StringBuilder does not guarantee its content is even contiguous, a copy will always be required, so it is the responsibility of the marshaler (or in this case, my friendly overload) to ensure that the copy target is null-terminated.

[jnm2]

Ah, I didn't recognize your comment about contiguity as being in reference to my question. Sorry!

[AArnott]

If we accept Span<char>, the marshaler (friendly overload) will have to ensure that '\0' shows up as the last character or somewhere else in the span. If it does not, a copy will have to be created to ensure the native code does not misbehave.

[jnm2]

so it is the responsibility of the marshaler (or in this case, my friendly overload) to ensure that the copy target is null-terminated.

I wonder what the marshaler does. My preference would be to throw rather than truncate the string, since the consequences of proceeding with an unintentionally truncated string (especally with I/O) could be important and maybe even exploitable.

[AArnott]

My preference would be to throw rather than truncate the string

When/why would we ever truncate the string? I wasn't suggesting that. I was saying that if we accept a StringBuilder, and its length is 5, we would allocate a 6 character memory block, copy the 5 chars in, then ensure the 6th is the null char. No truncating is happening there.

[riverar]

To be clear, .NET supports marshaling StringBuilder natively, but as we have a goal to have just one extern method, and that takes pointers, our friendly overload is on the hook to reproduce any marshaling steps that the runtime normally would have done.

Can you change your goal instead? Why is that your goal? If you consider that off-topic then I can open a separate bug for general project documentation.

[jnm2]

@AArnott In an API like this, where there's a documented maximum length:

    const int MAX_PATH = 260;
    var szIconFile = new StringBuilder(pathAndIndex, MAX_PATH);
    int index = PInvoke.PathParseIconLocation(szIconFile);
    return (szIconFile.ToString(), index);

What happens if pathAndIndex is bigger than PathParseIconLocation expects it to be, and the null terminator is not within the first MAX_PATH + 1 chars?

Maybe the API detects this and errors. But then there is no concern that the use of Span<char> would cause the null terminator to be forgotten about, is there? If you forget to null-terminate your span, the API would detect that and error.

I'm still trying to understand how StringBuilder succeeds where Span<char> fails, in terms of zero termination safety, from an earlier comment (#144 (comment)). I hope I'm not completely misunderstanding.

[AArnott]

@riverar One extern method is our goal because every extern method comes with hidden runtime costs, including a slow first invocation. To support AOT environments, some of this runtime cost may eventually change to extra compile-time generated code, which translates to larger image sizes. The CLR team reviewed our design and were delighted to hear this was our goal for this reason.
It also allows us to fix marshaling that .NET would get wrong. For example the MSIHANDLE type in 32-bits instead of IntPtr-sized, so the way .NET marshals SafeHandle would be broken if we relied on it.

[riverar]

@AArnott I'm not convinced at all, but rather than be an impediment I'll kindly step back and watch from afar. Thanks for your patience.

It also allows us to fix marshaling that .NET would get wrong. For example the MSIHANDLE type in 32-bits instead of IntPtr-sized, so the way .NET marshals SafeHandle would be broken if we relied on it.

Not sure what this means. It's a typedef unsigned long MSIHANDLE; in the SDK. Why do you think this needs to be IntPtr sized? Traditionally, only the lower 32 bits of a HANDLE are significant (see HWND, GDI objects, etc.) regardless of platform architecture.

[jnm2]

I think I see what I'm missing. It's not that the API wouldn't check, it's that there might be additional characters and then a zero terminator immediately following the span, so the API can't know that this is not what you meant to send.

The marshaling impl could solve this for Span<char> passed to PWSTR by throwing if span.EndsWith('\0') is false.

[jnm2]

@riverar Andrew doesn't think MSIHANDLE needs to be IntPtr sized. .NET thinks it does, if you use SafeHandle to wrap the handle. Rather than missing out on the benefits of SafeHandle encapsulation, Andrew is passing the wrapped handle value as 32 bits since .NET won't.

[AArnott]

What happens if pathAndIndex is bigger than PathParseIconLocation expects it to be, and the null terminator is not within the first MAX_PATH + 1 chars?

I don't know. That would be a per-API behavior and outside the scope of a general policy for generating overloads. Folks need to read the docs for these APIs and do the right thing.

Maybe the API detects this and errors. But then there is no concern that the use of Span would cause the null terminator to be forgotten about, is there? If you forget to null-terminate your span, the API would detect that and error.

We must not rely an that. We know nothing about the bytes after the last char in the span. It might include 50 garbage bytes and then two null bytes in a row, which may be enough for the API to report success since it found a null character although it isn't at all what was intended.
Or even more nefarious, we don't know when the memory beyond the span will change. So it's conceivable that a null may exist at one point in time during the call and then disappear later, just in time to make something very bad happen.

While we can certainly write it off as user error if they do not null-terminate, I think looking at the last char to see if it is null is cheap enough to be a reasonable mitigation to the risk we are introducing by use of span that the user assumes the span's length is passed to native code when it is not.

I'm still trying to understand how StringBuilder succeeds where Span<char> fails, in terms of zero termination safety

It doesn't. StringBuilder does not even guarantee contiguous memory, so even if it always terminates with null, we still have to copy to guarantee contiguous memory. IMO the only win StringBuilder offers is a slightly easier syntax than char[] or Span<char>.

[AArnott]

Traditionally, only the lower 32 bits of a HANDLE are significant (see HWND, GDI objects, etc.) regardless of platform architecture.

Certain handles are documented to never use the high 32-bits, but this cannot be assumed to be true for all handles. Just to warn you.

[AArnott]

it's that there might be additional characters and then a zero terminator immediately following the span, so the API can't know that this is not what you meant to send.

That's one possible end. Another is that it encounters an AV before it encounters a null character and crashes your app.

[AArnott]

The marshaling impl could solve this for Span<char> passed to PWSTR by throwing if span.EndsWith('\0') is false.

Yes, that's what I'm proposing.

[AArnott]

but that isn't important for CsWin32 since its Windows only

@tannergooding That is almost true. What about the wine case? I wonder what we should do to run properly there. Also the calling convention used in C# 9 function pointers left me wondering how we would work there. It certainly isn't a top concern or supported. But it makes me wonder.

[tannergooding]

What about the wine case?

I'd imagine that's a case where WINE itself needs to be concerned with converting between Windows type and Unix type rather than the user code. As far as the Windows API and its implementation is concerned, long is 32-bits. If you happen to run on a platform where it isn't, that platform needs to adjust itself to account for that at the relevant boundaries (just like SysWoW64, SysArm32, and the equivalent emulation layers on macOS, etc)

[michael-hawker]

Definitely a gotcha when trying to use these APIs, if you're a C# developer, you just start hitting a wall. Most of the old examples findable use StringBuilder as an easy way to get access to the string. I understand about it duplicating a buffer, but I sometimes care more about easily getting the data than performance at least when trying to find an initial solution to a problem. Can always use a better pattern after.

Would be nice to see some documentation and help to guide folks down the right path and be able to have different overloads which can use a simple StringBuilder or something more direct like span.

It is weird when the official docs have a nice sample here, but then say not to do that here.

Just want to point out when it's faster/easier to copy two lines of code to do the old PInvoke with a StringBuilder for GetWindowText than to use the source generator, there's a bit of a problem for discovery/usage.

[palenshus]

Coming here a year later, has there been any progress on this in the interim? Like @michael-hawker alluded to, I'm using cswin32 for all pinvokes in my current project except for GetWindowText, as the manual pinvoke is just so much more readable

[AArnott]

Not on this issue. I've been working on some other projects, but recently spent a bit more time on CsWin32. Not sure when I'll get to this, TBH. I hope soon.

[AArnott]

The friendly overloads in the issue description are obviously hand-written and contain return types that would not be generally creatable by CsWin32 from their native prototypes.
Here is my thinking for the final solution:

// demonstrates calling pattern for the new friendly overload:
[Fact]
public void PathParseIconLocation_Friendly()
{
    string sourceString = "hi there,3";
    Span<char> buffer = new char[PInvoke.MAX_PATH + 1];
    sourceString.AsSpan().CopyTo(buffer);
    int result = PathParseIconLocation(ref buffer);
    Assert.Equal(3, result);
    Assert.Equal("hi there", buffer.ToString());
}

// proposed helper method:
private static unsafe int PathParseIconLocation(ref Span<char> buffer)
{
    fixed (char* pBuffer = buffer)
    {
        PWSTR wstr = pBuffer;
        int result = PInvoke.PathParseIconLocation(wstr);
        buffer = buffer.Slice(0, wstr.Length);
        return result;
    }
}

[riverar]

This issue was to cover the lack of a friendly way to call this API. I maintain that using a bunch of unsafe unidiomatic code with memory pinning is not friendly in any way, shape, or form.

[AArnott]

There is no unsafe code or memory pinning in the [Fact] method, which represents user code in this example.

[riverar]

@AArnott Oh I misunderstood that. This is great then.

[tannergooding]

Here is my thinking for the final solution:

@AArnott this code looks incorrect. The native signatures is int PathParseIconLocationW([in, out] LPWSTR pszIconFile); and LPWSTR is meant to be null-terminated. ToCharArray does not include a terminating null and so the native method will end up reading past the end of the array.

Further, this function in particular requires that a buffer of MAX_PATH be passed in and so native code will likely end up writing outside the bounds of what's been allocated.

[tannergooding]

In general, there are many native APIs in this format and even the C headers do not provide enough context (even in the SAL annotations) to be able to automatically determine the "correct" handling in every scenario.

There are going to be many potential CVEs and other issues for APIs like this where the "safe" signature is detected via some heuristic.

[riverar]

Good catch, missed that. Am a little surprised CI didn't catch this, are those manual tests? (Not familiar with this project.)

[riverar]

Further, this function in particular requires that a buffer of MAX_PATH be passed in and so native code will likely end up writing outside the bounds of what's been allocated.

I see that in the docs, but it doesn't quite make sense. And looking at the impl., it doesn't appear this is true (anymore?). Suspect this is a doc bug.

[tannergooding]

The MAX_PATH in this case may be a doc bug, its possible it was changed alongside many other MAX_PATH APIs when long path support was added.

The null-terminator being required is definitely still the case (there is no length passed in) as is the more general point of potential CVEs if these "helper signatures" aren't carefully reviewed and considered.

[AArnott]

Thanks for reviewing, @tannergooding.

this code looks incorrect. ... ToCharArray does not include a terminating null and so the native method will end up reading past the end of the array.

Well, that's a bug in my example rather than a bug in the generated code. But I should still fix that. It'll be considerably more complicated to do it 'right' which undermines the simplicity of the use case, sadly.

In general, there are many native APIs in this format and even the C headers do not provide enough context (even in the SAL annotations) to be able to automatically determine the "correct" handling in every scenario.

That's why the helper method just takes a Span<char>, leaving it up to the caller to size and initialize the buffer as necessary.

I wonder though, to be more defensive around easy bugs like the one I made in the caller, should the generated code ensure that the span contains a null character somewhere? We could first check the last character to see if it's null, and only search the whole span if null isn't at the last position. The goal being to ensure that the PWSTR contract of a null terminator is honored somewhere in the buffer. That penalizes runtime for those who get it right, but it will probably not be significant. And if it is for some, well those folks can always go back to calling the extern method.

There are going to be many potential CVEs and other issues for APIs like this where the "safe" signature is detected via some heuristic.

I wonder if CsWin32 should inject an assembly-level attribute to effectively document the version of CsWin32 that was used in a particular compilation. That way if memory bugs are found in CsWin32-generated code, we can more easily identify possibly impacted assemblies.

[AArnott]

@tannergooding So, possible doc bug aside, when an API says "a null-terminated string of length MAX_PATH", does that mean the buffer must be MAX_PATH + 1 so it can have MAX_PATH characters while still being null-terminated, or must the null terminator appear within MAX_PATH, effectively reducing the number of characters that can be there to MAX_PATH - 1?

[tannergooding]

I think there are lots of options and there has been discussion (on the Runtime team) on whether there could be some analyzer to help flag problematic cases around Span<char>, ReadOnlySpan<char>, and the byte equivalents for ASCII/UTF-8 text. CC. @stephentoub, @GrabYourPitchforks

In general, however, this is a large potential pit of failure, especially for users who aren't intimately familiar with interop code or the requirements.

In the case that a string literal is passed in, the Span/ROSpan won't include the terminating null-character and so having the marshaller check for this will have a lot of false positives for common scenarios. Likewise, you can't safely read one past the end of the Span/ROSpan since that may itself be an AV (access violation).

---

does that mean the buffer must be MAX_PATH + 1 so it can have MAX_PATH characters while still being null-terminated, or must the null terminator appear within MAX_PATH, effectively reducing the number of characters that can be there to MAX_PATH - 1?

The latter, as covered by https://docs.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation?tabs=cmd.

The premise is that MAX_PATH is 256 because you can have a drive + separator C:\, one or more path parts (a single path part can be up to 256 characters, even with "long path" support), and the null terminator.

[tannergooding]

In general I'd recommend either taking string in which case you can know that its null-terminated or always allocating and ensuring you include a null-terminator yourself (just as the DllImport P/Invoke Generator will do for the non-string scenario).

I'd also lean towards matching what the docs say and not looking at the implementation. Not only can the implementation change over time (and potentially differ on older or future versions of Windows) but not everyone can look at the implementation and so no one else can replicate or rationalize the "why".

If there is a case where the docs are out of sync, we should likely push to ensure those get fixed. Likewise we should push to ensure the SAL annotations are up to date and cover the relevant information where possible.

[AArnott]

In the case that a string literal is passed in, the Span/ROSpan won't include the terminating null-character

A string literal couldn't be used where a Span<char> is required because string literals are immutable. This friendly overload we're talking about is specifically for [in, out] PWSTR, so string literals will never be accepted here.

In general I'd recommend either taking string in which case you can know that its null-terminated

We do take string for [in] PCWSTR parameters. And yes, we take the optimal path by knowing that .NET strings are always null-terminated.

[AArnott]

I corrected my earlier example method to allocate a proper, null-terminated buffer. I'm thinking I'll go ahead and add a null-termination check to the friendly overload as well.

[AArnott]

I wonder if CsWin32 should inject an assembly-level attribute to effectively document the version of CsWin32 that was used in a particular compilation. That way if memory bugs are found in CsWin32-generated code, we can more easily identify possibly impacted assemblies.

FYI I went ahead and did this in #555

[palenshus]

Hi! Pardon me but I think I'm missing something, I'd been under the impression the fix for this issue would include a friendly overload for GetWindowText, but all I'm seeing is this:

internal static extern int GetWindowText(winmdroot.Foundation.HWND hWnd, winmdroot.Foundation.PWSTR lpString, int nMaxCount);

Is that still coming in another issue or was it deemed impossible? It's hard for me to parse where y'all landed on that from the above discussion. Thanks!!

[AArnott]

@palenshus, this issue was about [In, Out] PWSTR parameters specifically. This was resolved by offering a friendly overload where that parameter becomes ref Span<char>.

In your GetWindowText case, that's an [Out] PWSTR parameter, so it's unrelated to this. I see we don't produce a friendly overload for it. If you'd like to see that, please open a new issue. Maybe we could offer an overload with a Span<char> parameter.

[palenshus]

Ah I see, makes sense, thanks! I filed #614, though I'm not positive that either the "current way to do it" code or any of the proposals are correct, just trying to get the ball rolling. Thanks!