Powershell: Review using StringBuilder in pinvokes

[iSazonov]

From https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/interop-pinvokes.md#strings

StringBuilder marshalling always re-allocate/re-copy buffers. Make sense review our code to reduce such allocations.

From @daxian-dbw https://github.com/PowerShell/PowerShell/pull/9066#issuecomment-472975185

it seems StringBuilder should never be used for P/Invoke as it incurs 4 allocations. I think it make sense to review our uses of it and replace with ArrayPool when appropriate, because we will copy the existing pattern when writing new code.

From above docs:

[USE] Char arrays from ArrayPool or StringBuffer.

[daxian-dbw]

We are using StringBuilder for the [out] string parameter in P/Invoke almost everywhere.

[iSazonov]

Is everything so bad? :-)

[IISResetMe]

I could only find 3:

[Microsoft.PowerShell.Commands.Native]::LCIDToLocaleName()
[Microsoft.PowerShell.Commands.ProcessNativeMethods]::CreateProcess()
[Microsoft.Windows.Appx.PackageManager.Commands.NativeMethods]::PackageFamilyNameFromFullName()

[iSazonov]

In CoreFX repo I found 25 files with StringBuilder and name *interop*.cs.
Interesting most of them has [out] attribute!
@jazzdelightsme Have you any thoughts about this?

It seems we need to invertigate it more deeply. Where (repo) is the StringBuilder marshalling implemented?

[daxian-dbw]

Well, some may not be a out parameter in the underlying Win32 API. I found the following by a quick search in the files @iSazonov touched in #8847

[Process.cs] CreateProcessWithLogonW, CreateProcess
[ConsolControl.cs] GetConsoleTitle, ReadConsole
[Native.cs] LookupAccountSid, LookupAccountNam, FormatMessage
[FileSystemProvider.cs] QueryDosDevice ...

[iSazonov]

Sample from CoreFX for GetWindowTextW - they use pointer instead of StringBuilder.

https://github.com/dotnet/corefx/blob/81bd671efda5db20fae1eb381aeff2cf8ea727ac/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Win32.cs#L227-L234

https://github.com/dotnet/corefx/blob/8c5260061b11323dfd97fbab614d54402405513f/src/Common/src/Interop/Windows/User32/Interop.GetWindowTextW.cs#L14

Signature doesn't have [out] https://docs.microsoft.com/en-us/windows/desktop/api/winuser/nf-winuser-getwindowtextw

[iSazonov]

One reference to DllImport marhalling https://github.com/dotnet/coreclr/blob/78cbd968cabc2bd6c84bfe6a0a240658e564a6d5/src/vm/ilmarshalers.cpp#L810
Not clear how it works.

[jazzdelightsme]

Important: [Out] (OutAttribute) is very different from out (keyword). The attribute [Out] is trying to express that information needs to flow back to the caller (so if a copy had to be made during marshaling, when the call completes, the CLR needs to copy data back into the managed object). For blittable things that just get pinned and modified directly in place, it does not matter if you forget [Out], because the memory was modified in-place.

The out keyword, though, also indicates the level of indirection. I.e. if you are passing a Foo to native code via PInvoke, and Foo is a class, and the PInvoke signature says "Foo f", that basically turns into "Foo* f". But if the signature says "out Foo f", then the equivalent native signature is "Foo** f". Big difference!

(The ref keyword is similar in terms of indicating another level of indirection; but also implies [In] and [Out], which sometimes the marshaler may need to know if it needs to do something to marshal a pointer in as well as out, such as a COM interface.)

(Maybe you folks already know this stuff, but if so, there may have been some typos in this thread that made me think it might need clearing up (it's very easy to mix [Out] and out; I did it while typing this), so I wanted to spell it out to make sure we're on the same page about stuff. Sorry to re-explain if you were already aware.)

It may be the case for StringBuilder that it is special-cased and magically works with or without out, but I would not have guessed that (which is why I got involved in the first place when I saw the out added to that PR).

I have always believed that the marshaler had special knowledge of StringBuilder and would just pin the buffer. And those comments I found on the internet (from that long-gone MSDN article) seems to indicate that perhaps it did indeed work that way once. But the doc linked by iSazonov at the top seems clear, and I would trust current documentation from CoreFX people way more than my old memory.

And in fact, I bet these are the issues that inspired that (current) documentation:

• https://github.com/dotnet/coreclr/issues/1842
• https://github.com/dotnet/coreclr/pull/21785
• https://github.com/dotnet/coreclr/pull/21800

Sounds like there was a fly in the ointment regarding null terminators; perhaps in ancient history it did work as I thought, but then had to be pessimized to be correct for certain required cases.

On the bright side, at least there is a good recommendation for alternatives that are better.

[iSazonov]

@jazzdelightsme Thanks for clarify and confirmation!

I added "[USE] Char arrays from ArrayPool or StringBuffer." in the issue description.
We need to look our P/Invokes on hot paths.

[iSazonov]

I hope it is full list:

• [Won't fix] - src\Microsoft.PowerShell.Commands.Management\commands\management\GetComputerInfoCommand.cs
  LCIDToLocaleName - it is an edge case. Won't fix
• [x] - src\Microsoft.PowerShell.Commands.Management\commands\management\Process.cs
  #9195 (Merged) - style fixes
  #9197 (Merged) - LookupAccountSid
  CreateProcessWithLogonW, CreateProcess - it is an edge case. Won't fix
• [x] - #9165 (merged) src\Microsoft.PowerShell.ConsoleHost\host\msh\ConsoleControl.cs
• [Won't fix] - src\Microsoft.PowerShell.CoreCLR.Eventing\DotNetCode\Eventing\UnsafeNativeMethods.cs
  StringBuilder is used to create a result string, using Span<> cause the same one coping a buffer to new result string - make no sense to fix.
• [Postpone] - src\Microsoft.PowerShell.LocalAccounts\LocalAccounts\Native.cs
  It is not compiled - see #4305
• [x] - src\System.Management.Automation\engine\hostifaces\HostUtilities.cs
  #9220 (merged) - remove the unused code
• [x] - src\System.Management.Automation\engine\hostifaces\NativeCultureResolver.cs
• [Won't fix] - src\System.Management.Automation\engine\NativeCommandProcessor.cs
  FindExecutableW - it is an edge case.
• [Won't fix] - src\System.Management.Automation\namespaces\FileSystemProvider.cs
  WNetGetConnection (NewDrive, RemoveDrive) - it is an edge case.
  QueryDosDevice (fallback to search file assoc) - it is an edge case.
• [x] - #9197 (dup - see above) src\System.Management.Automation\namespaces\Win32Native.cs

[iSazonov]

@daxian-dbw @SteveL-MSFT I finished (I hope) reviewing the case. Two PR is waiting a code review. Please look the list above and confirm that I do not skipped something.

[msftbot[bot]]

This issue has been marked as fixed and has not had any activity for 1 day. It has been closed for housekeeping purposes.