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

[mono][wasm] DllImport with value-type args that contain unsupported marshalled fields crashes AOT compiler #104463

Closed
maxkatz6 opened this issue Jul 5, 2024 · 3 comments · Fixed by #105156
Assignees
Labels
arch-wasm WebAssembly architecture area-Codegen-AOT-mono
Milestone

Comments

@maxkatz6
Copy link
Contributor

maxkatz6 commented Jul 5, 2024

Description

Normally, Mono WASM replaces unsupported DllImports with stubs, that throw an exception in runtime.
For example, if I declare any parameter with [MarshalAs(UnmanagedType.IUnknown)] in the DllImport:

[DllImport("ole32.dll")]
internal static extern void ReleaseStgMedium([MarshalAs(UnmanagedType.IUnknown)] ref object medium);

It will work as expected - compiler won't crash, and code is successfully executed unless this DllImport method is actually called in runtime.

But if I move [MarshalAs(UnmanagedType.IUnknown)] to the argument struct, Mono AOT compiler crashes with marshalling conversion 27 not implemented error.

In the example below, I used ReleaseStgMedium and STGMEDIUM just to reproduce the issue.
It crashes because STGMEDIUM struct is defined with unsupported marshalling type:

[MarshalAs(UnmanagedType.IUnknown)]
public object? pUnkForRelease;

Reproduction Steps

  1. Create browser-wasm project from a template.
  2. Modify Program.cs:
public static void Main(string[] args)
{
    Console.WriteLine("Hello, Browser!");

    DoStuff(args.FirstOrDefault());

    Console.WriteLine("After DoStuff");
}

private static void DoStuff(string key)
{
    if (key == "test-windows")
    {
        var s = new STGMEDIUM();
        ReleaseStgMedium(ref s);
    }

    // Other branches for other platforms that we don't care in this repro.
}

[DllImport("ole32.dll")]
internal static extern void ReleaseStgMedium(ref STGMEDIUM medium);

WasmRepro.zip

Expected behavior

Assembly and the app are compiled successfully.
Ideally, compiler outputs a warning/message about unsupported DllImport.

Faulty DllImport is replaced with a stub that throws in runtime, instead of crashing the compiler.

Actual behavior

  WasmRepro failed with 2 error(s) (2.2s) → bin\Release\net9.0\browser-wasm\publish\
    C:\Program Files\dotnet\packs\Microsoft.NET.Runtime.WebAssembly.Sdk\9.0.0-preview.5.24306.7\Sdk\WasmApp.Common.targets(696,5): error :
      Precompiling failed for E:\Work\Projects\WasmRepro\WasmRepro\obj\Release\net9.0\browser-wasm\wasm\for-publish\aot-in\aot-instances.dll with exit code -1073740791.
      marshalling conversion 27 not implemented
    C:\Program Files\dotnet\packs\Microsoft.NET.Runtime.WebAssembly.Sdk\9.0.0-preview.5.24306.7\Sdk\WasmApp.Common.targets(696,5): error :
      Precompiling failed for E:\Work\Projects\WasmRepro\WasmRepro\obj\Release\net9.0\browser-wasm\wasm\for-publish\aot-in\ClassLibrary.dll with exit code -1073740791.
      Mono Ahead of Time compiler - compiling assembly E:\Work\Projects\WasmRepro\WasmRepro\obj\Release\net9.0\browser-wasm\wasm\for-publish\aot-in\ClassLibrary.dll
      marshalling conversion 27 not implemented
  WasmRepro failed (2.5s) → bin\Release\net9.0\browser-wasm\publish\

Regression?

As far as I can tell, the same behavior was in .NET 8 and .NET 7.

Known Workarounds

Use Cecil or trimming descriptor files to exclude these specific dll imports from compilation.

Configuration

.NET 9 preview5
Windows 11
browser wasm
any browser

Other information

Other platform RIDs, like linux, macOS as well as mobile mono do support such DllImports without issues as far as I can tell.
It causes inconsistency

In ideal world, where we have access to the fauly code, we would replace these DllImports with safer alternatives or added OperatingSystem.IsWindows checks. But we can't do that, as in real world this code is hidden somewhere inside closed source third party dependency, making it completely unsable.

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jul 5, 2024
@lewing lewing added area-Codegen-AOT-mono arch-wasm WebAssembly architecture and removed area-Interop-coreclr untriaged New issue has not been triaged by the area owner labels Jul 5, 2024
@lewing lewing added this to the 9.0.0 milestone Jul 5, 2024
@lewing
Copy link
Member

lewing commented Jul 5, 2024

cc @steveisok

@steveisok
Copy link
Member

I modified a test for our mobile targets and the aot compiler produces the same error.

I believe this might be a linker issue. I think the challenge is easily detecting when it's a COM interop scenario vs a legit native library import. @vitek-karas, is that correct?

@vitek-karas
Copy link
Member

There's some heuristic in the trimmer to detect incompatible COM interop, but it's just a heuristic:

static bool IsComInterop (IMarshalInfoProvider marshalInfoProvider, TypeReference parameterType, LinkContext context)

In this case it fails to detect the problem because the struct is passed by-ref, and we didn't implement the heuristic to look "inside" references. But there are many other ways this can fail. Writing a full solution would be more work and it would need to copy lot of rules otherwise only known at runtime.

Regardless, WASM has trim warnings disabled by default anyway, so I would expect that this would not be visible to the user even if it worked.

Ultimately, I don't think this is something the trimmer should try to solve - we added this to only help people detect possible issues - by producing diagnostics. The AOT compiler should not fail in this case, and that has nothing to do with the trimmer. If the compiler can't compile a PInvoke, it should instead replace it with throwing method - like it already does in several other cases. I don't know if we have some custom steps which should do this at trimmer time, but I'd say they would also be a "best-guess", as they won't know the full set of capabilities the AOT compiler has.

@steveisok steveisok self-assigned this Jul 18, 2024
steveisok added a commit to steveisok/runtime that referenced this issue Jul 19, 2024
…_CONV_OBJECT_IUNKNOWN

This change prevents the aot compiler from erroring out when dealing with COM types that were not marked with MarshalAs attributes. In most scenarios that we support, we want to allow pinvokes to aot compile as in cases like anything COM interop, will end up erroring out if you try to use it.

Fixes dotnet#104463
@github-actions github-actions bot locked and limited conversation to collaborators Aug 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Codegen-AOT-mono
Projects
None yet
4 participants