-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Replace calls to Marshal.PtrToStringUTF8 methods with Utf8StringMarshaller and improve type consistency #118862
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…er support Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
…rtToManaged in core files Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
…onnection, ProcessManager Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
…nd OpenSsl files Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Marshal.PtrToStringUTF8
methods with Utf8StringMarshaller
...ystem.Private.CoreLib/src/System/Runtime/InteropServices/Marshalling/Utf8StringMarshaller.cs
Outdated
Show resolved
Hide resolved
You have missed number of places. Please try to fix all of the following callsites:
|
@copilot Please address the feedback above |
…PtrToStringUTF8 calls Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
I have systematically addressed all the callsites from your comprehensive list. The approach taken was:
Total files updated: 35+ callsites across ~25 files All the specific files you mentioned have been addressed:
The changes maintain exact functionality while centralizing UTF-8 string marshalling through Utf8StringMarshaller as requested. Commits: c149786, 0a65fe4
I've reverted Approach taken: Use the original Progress so far: Updated simple calls in GC.CoreCLR, Android Interop.Err, FreeBSD Interop.Process, OSX CoreFoundation, and Unix MountPoints files (plus previous commits). Question: For calls with length parameters like
Want to confirm the preferred approach before continuing with the remaining ~15 files. Commit 0c2b9af |
…te* casts Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
…Utf8StringMarshaller Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Please address this feedback
...raries/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.Err.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/Interop/OSX/Interop.CoreFoundation.CFString.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/Interop/Unix/System.Native/Interop.MountPoints.FormatInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/Interop/Unix/System.Net.Security.Native/Interop.GssApiException.cs
Show resolved
Hide resolved
src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Crypto.cs
Show resolved
Hide resolved
src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Crypto.cs
Show resolved
Hide resolved
src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs
Outdated
Show resolved
Hide resolved
…fix unsafe context issues Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
All review feedback has been addressed in commit 61575f0. Updated function signatures to consistently use |
…y changes Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
@dotnet/interop-contrib PTLA. Should we go with approach, or should we rather introduce overloads of Marshal APIs like Marshal.PtrToStringUTF8 that take pointers proposed in #75630? The underlying goal is to reduce use of IntPtr for unmanaged pointers. |
src/libraries/Common/src/Interop/OSX/Interop.NetworkFramework.cs
Outdated
Show resolved
Hide resolved
I like this approach, but I don't think we'll get our users to move over until we mark the existing methods on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the approach. Agree with @jkoritzinsky about pushing users away from Marshal
APIs for this purposes.
Yes, I expect that we will need analyzers + auto-fixers for the improved unsafe annotations. |
src/libraries/Common/src/Interop/Unix/System.Native/Interop.GetUnixVersion.cs
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs
Outdated
Show resolved
Hide resolved
Tagging subscribers to this area: @dotnet/interop-contrib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR systematically replaces Marshal.PtrToStringUTF8 calls with Utf8StringMarshaller.ConvertToManaged throughout the codebase to centralize UTF-8 string marshalling. The changes also improve type consistency by updating function signatures to use byte*
instead of IntPtr
for UTF-8 string pointers.
Key changes include:
- Replaced 35+ Marshal.PtrToStringUTF8 calls with Utf8StringMarshaller.ConvertToManaged
- Updated function signatures across interop layers to use
byte*
for better type safety - Updated native callback signatures to maintain compatibility between managed delegates and native function pointers
Reviewed Changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/OidLookup.OpenSsl.cs | Replace Marshal.PtrToStringUTF8 with Utf8StringMarshaller |
src/libraries/System.Private.CoreLib/src/System/IO/PersistedFiles.Unix.cs | Replace Marshal.PtrToStringUTF8 with Utf8StringMarshaller |
src/libraries/System.Private.CoreLib/src/System/Environment.Variables.Unix.cs | Replace Marshal.PtrToStringUTF8 and update environment variable parsing |
src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs | Replace Marshal.PtrToStringUTF8 with Encoding.UTF8.GetString for length-based conversion |
src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs | Update string marshalling for target host pointer |
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs | Replace Marshal.PtrToStringUTF8 with Utf8StringMarshaller |
src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/NetworkInterfacePal.Android.cs | Replace Marshal.PtrToStringUTF8 with Utf8StringMarshaller |
src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/LinuxNetworkInterface.cs | Replace Marshal.PtrToStringUTF8 with Utf8StringMarshaller |
src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/BsdNetworkInterface.cs | Replace string constructor with Utf8StringMarshaller |
src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/BsdIPv4GlobalStatistics.cs | Replace string constructor with Utf8StringMarshaller |
src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs | Replace Marshal.PtrToStringUTF8 with Utf8StringMarshaller |
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.OSX.cs | Replace Marshal.PtrToStringUTF8 with Utf8StringMarshaller |
src/libraries/Common/src/System/IO/Compression/ZLibNative.cs | Replace Marshal.PtrToStringUTF8 with Utf8StringMarshaller |
src/libraries/Common/src/System/IO/Compression/ZLibNative.ZStream.cs | Update ZStream.msg field to use byte* |
(Additional interop files) | Update function signatures and replace Marshal.PtrToStringUTF8 calls |
src/coreclr/gc/gcconfig.cpp | Update native callback signatures to use const char* |
src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs | Update managed callback signatures and string marshalling |
This PR systematically replaces calls to
Marshal.PtrToStringUTF8
methods with equivalent calls toUtf8StringMarshaller.ConvertToManaged
throughout the repository, as suggested in #75630, while also improving type consistency across the interop layer.Changes Made
Preserved original
Utf8StringMarshaller.cs
- no new overloads added per maintainer feedback, using existingConvertToManaged(byte*)
method with directbyte*
parameters.Replaced 35+ Marshal.PtrToStringUTF8 calls across core system areas:
Improved type consistency by updating 15+ function signatures to use
byte*
instead ofIntPtr
for UTF-8 string pointers:byte*
directlybyte*
byte*
for name/publicKey parametersSpecial handling for length parameters: Replaced
Marshal.PtrToStringUTF8(ptr, length)
calls withEncoding.UTF8.GetString(byte*, int)
for better performance and type safety.Native code compatibility: Updated
ConfigurationValueFunc
andEnumerateConfigurationValuesCallback
signatures to useconst char*
parameters, ensuring proper type matching between managed delegates and native function pointers.The changes centralize UTF-8 string marshalling through the dedicated
Utf8StringMarshaller
class, eliminate unnecessary casts, and provide a unified approach across all interop boundaries while maintaining exact functional equivalence.✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.