Skip to content
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

SafeHandle marshalling broken with Mono #82308

Closed
uweigand opened this issue Feb 17, 2023 · 4 comments · Fixed by #82376
Closed

SafeHandle marshalling broken with Mono #82308

uweigand opened this issue Feb 17, 2023 · 4 comments · Fixed by #82376

Comments

@uweigand
Copy link
Contributor

As of #80715, many System.Net.Sockets.Tests test fail with the error:

System.Exception : Failed to create the 'shadow' (port blocker) socket in 16 attempts.

Looking at the behavior with strace shows the error is caused by failed attempts to bind to file descriptor 0 (which is of course not a socket):

[pid 774258] bind(0, {sa_family=AF_INET6, sin6_port=htons(44087), sin6_flowinfo=htonl(0), inet_pton(AF_INET6, "::1", &sin6_addr), sin6_scope_id=0}, 28) = -1 ENOTSOCK (Socket operation on non-socket)

Investigating this closer shows that the bind call happens via a P/Invoke in the new test infrastructure:

[Runtime.InteropServices.DllImport("libc", SetLastError = true)]
static extern int bind(SafeSocketHandle socket, IntPtr socketAddress, uint addrLen);

This seems to trigger marshalling code in Mono that accesses the IntPtr wrapped by the SafeHandle:

/* Pull the handle field from SafeHandle */
cb_to_mono->methodBuilder.emit_ldarg (mb, argnum);
cb_to_mono->methodBuilder.emit_ldflda (mb, MONO_STRUCT_OFFSET (MonoSafeHandle, handle));
cb_to_mono->methodBuilder.emit_byte (mb, CEE_LDIND_I);
cb_to_mono->methodBuilder.emit_stloc (mb, conv_arg);

using the data structure here:
/*
* The definition of the first field in SafeHandle,
* Keep in sync with SafeHandle.cs, this is only used
* to access the `handle' parameter.
*/
typedef struct {
MonoObject base;
void *handle;
} MonoSafeHandle;

However, as of #71991 the definition of the SafeHandle type on the managed side is:

// IMPORTANT:
// - Do not add or rearrange fields as the EE depends on this layout,
// as well as on the values of the StateBits flags.
// - The EE may also perform the same operations using equivalent native
// code, so this managed code must not assume it is the only code
// manipulating _state.
#if DEBUG
private readonly string? _ctorStackTrace;
#endif
/// <summary>Specifies the handle to be wrapped.</summary>
protected IntPtr handle;

Note when the library is built for the Debug configuration, the layout no longer matches - in the place Mono expects the handle field, we now have the _ctorStackTrace field. As this is usually null, we get 0 as file descriptor ...

@stephentoub I see you updated the matching CoreCLR definition with #71991, but not the Mono definition. I guess this should be done there as well - however I'm not clear how to do this only for the Debug version of the library, this is something the Mono runtime doesn't currently appear to be doing ...

Also, I was quite confused why this problem only shows up in this one test, and not for all the "normal" places in the library where SafeHandles are marshalled to native code. I now see this is using a different mechanism:

[LibraryImport(Libraries.SystemNative, EntryPoint = "SystemNative_Bind")]
internal static unsafe partial Error Bind(SafeHandle socket, ProtocolType socketProtocolType, byte* socketAddress, int socketAddressLen);

which doesn't trigger the Mono marshalling code, but generates IL that uses calls to System.Runtime.InteropServices.SafeHandle:DangerousGetHandle instead of accessing the handle at a hard-coded offset.

CC @antonfirsov @vargaz @omajid @tmds

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Feb 17, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Feb 17, 2023
@vcsjones vcsjones added area-Interop-mono and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Feb 17, 2023
@stephentoub
Copy link
Member

Note when the library is built for the Debug configuration, the layout no longer matches - in the place Mono expects the handle field, we now have the _ctorStackTrace field. As this is usually null, we get 0 as file descriptor ...
@stephentoub I see you updated the matching CoreCLR definition with #71991, but not the Mono definition. I guess this should be done there as well

That's definitely a miss on my part. Though I'm also surprised nothing has failed until now, so maybe there's more going on.

Simplest fix is to just change the ifdef in SafeHandle.cs to make that additional debugging logic exclusive to coreclr.

@tmds
Copy link
Member

tmds commented Feb 17, 2023

We have a CI job that runs tests with --runtimeconfiguration Release --librariesConfiguration Debug /p:PrimaryRuntimeFlavor=Mono on x64 where all these tests pass.

The problem doesn't sound architecture specific. I don't know why it doesn't show this issue. 🤔

@stephentoub
Copy link
Member

We have a CI job that runs tests with --runtimeconfiguration Release

The additional code I added to SafeHandle is under DEBUG and is in corelib; it wouldn't affect a Release runtime.

@tmds
Copy link
Member

tmds commented Feb 17, 2023

Ah, I wrongly assumed corelib code was under --librariesConfiguration Debug.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 20, 2023
@ghost ghost removed in-pr There is an active PR which will close this issue when it is merged untriaged New issue has not been triaged by the area owner labels Feb 28, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants