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

[.NET 9] DllImport with Custom Marshaler Passes non-null address when null value for field is specified. #109033

Open
Sewer56 opened this issue Oct 18, 2024 · 4 comments
Assignees
Labels
area-Interop-coreclr untriaged New issue has not been triaged by the area owner

Comments

@Sewer56
Copy link
Contributor

Sewer56 commented Oct 18, 2024

Description

In .NET 9 (Preview 2 and newer), passing null to a parameter which uses a CustomMarshaler results in a non-zero value passed to native code. This can lead to memory corruption or invalid memory reads, depending on what the native code does with the value.

Reproduction Steps

Attached reproduction: cimgui-stub.zip

To Reproduce

Run the csharp project (csharp.csproj) provided.

When ran with .NET 9 Preview 1 or older, the result is:

igDragInt called with:
  label address: 2941681900960
  v address: 750695999576
  format address: 0
  label value: label
  v value: 0
  v_speed: 0.1
  v_min: 0
  v_max: 100
  format: null
  flags: 0

When ran with .NET 9 Preview 2 or newer the result is:

igDragInt called with:
  label address: 1824962101440
  v address: 114923399832
  format address: 14829735431805717965
  label value: label
  v value: 0
  v_speed: 0.1
  v_min: 0
  v_max: 100
Fatal error. 0xC0000005
   at ImGui.DragInt(System.String, Int32 ByRef, Single, Int32, Int32, System.String, Int32)
   at Program.<Main>$(System.String[])

When null is passed to a string parameter with any CustomMarshaler specified, the generated DllImport stub passes a non-zero address. (Note: Marshaler is skipped, so implementation of marshaler used with CustomMarshaler is not relevant)


To Compile the Rust Library/ Native Code

[Note: I've included an x64 precompiled DLL in the csharp folder out of the box, this is only needed if you want to change the native DllImport target, e.g. reduce param count for debugging]

  1. Install rustup: https://static.rust-lang.org/rustup/dist/x86_64-pc-windows-msvc/rustup-init.exe
  2. cd into cimgui_export
  3. cargo build --release
  4. Result is in target/release/cimgui_export.dll
  5. Paste into csharp folder, replacing the existing file.

Expected behavior

format address should be 0 (null). Instead an unexpected address is used.

Actual behavior

format address is not 0 (null)

Regression?

.NET 9 (Preview 1) and older work fine.
Regression is introduced in Preview 2.

Known Workarounds

No response

Configuration

N/A

This reproduces in both x86 and x64 on Windows.

Other information

The reproduction is small enough that it can be included here for convenience:

var v = 0;
while (true)
{
    ImGui.DragInt("label", ref v, 0.1f, 0, 100, null!, 0);
    Thread.Sleep(1000);
}

static class ImGui
{
    [DllImport("cimgui_export", CallingConvention = CallingConvention.Cdecl, EntryPoint = "igDragInt")]
    [SuppressUnmanagedCodeSecurity]
    [return: MarshalAs(UnmanagedType.I1)]
    public static extern bool DragInt(
        [MarshalAs(UnmanagedType.CustomMarshaler, MarshalType = "csharp.UTF8Marshaller, csharp, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null")]
        string label,
        ref int v,
        float v_speed,
        int v_min,
        int v_max,
        [MarshalAs(UnmanagedType.CustomMarshaler, MarshalType = "csharp.UTF8Marshaller, csharp, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null")]
        string format,
        int flags
    );
}

Rust part:

use std::ffi::CStr;
use std::os::raw::{c_char, c_int, c_float, c_uchar};

#[no_mangle]
pub extern "C" fn igDragInt(
    label: *const c_char,
    v: *mut c_int,
    v_speed: c_float,
    v_min: c_int,
    v_max: c_int,
    format: *const c_char,
    flags: c_int,
) -> c_uchar {
    // Print addresses of all pointers
    println!("igDragInt called with:");
    println!("  label address: {:?}", label as usize);
    println!("  v address: {:?}", v as usize);
    println!("  format address: {:?}", format as usize);

    // Handle `label`
    if label.is_null() {
        println!("  label: null");
    } else {
        // Convert C string to Rust string
        let c_label = unsafe { CStr::from_ptr(label) };
        match c_label.to_str() {
            Ok(s) => println!("  label value: {}", s),
            Err(_) => println!("  label: Invalid UTF-8 string"),
        }
    }

    // Handle `v`
    if v.is_null() {
        println!("  v: null");
    } else {
        // Safely read the integer value
        let value = unsafe { *v };
        println!("  v value: {}", value);
    }

    // Print other parameters
    println!("  v_speed: {}", v_speed);
    println!("  v_min: {}", v_min);
    println!("  v_max: {}", v_max);

    // Handle `format`
    if format.is_null() {
        println!("  format: null");
    } else {
        // Convert C string to Rust string
        let c_format = unsafe { CStr::from_ptr(format) };
        match c_format.to_str() {
            Ok(s) => println!("  format value: {}", s),
            Err(_) => println!("  format: Invalid UTF-8 string"),
        }
    }

    println!("  flags: {}", flags);

    // For demonstration, return true (1) to indicate success
    1
}

I can across this issue when trying older code with .NET 9 that used dear imgui bindings made with CppSharp. So in the repro I tried to model that.

@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 Oct 18, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Oct 18, 2024
@Sewer56 Sewer56 changed the title [.NET 9] DllImport with Custom Marshaller Passes non-null address when [.NET 9] DllImport with Custom Marshaller Passes non-null address when null is specified. Oct 18, 2024
@Sewer56 Sewer56 changed the title [.NET 9] DllImport with Custom Marshaller Passes non-null address when null is specified. [.NET 9] DllImport with Custom Marshaller Passes non-null address when null value for field is specified. Oct 18, 2024
@Sewer56 Sewer56 changed the title [.NET 9] DllImport with Custom Marshaller Passes non-null address when null value for field is specified. [.NET 9] DllImport with Custom Marshaler Passes non-null address when null value for field is specified. Oct 18, 2024
@jkotas jkotas added area-Interop-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Oct 19, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

@huoyaoyuan
Copy link
Member

You are using the wrong marshalling mechanism. System.Runtime.InteropServices.ICustomMarshaler is for COM interop. See the documentation at https://learn.microsoft.com/dotnet/fundamentals/runtime-libraries/system-runtime-interopservices-icustommarshaler and https://learn.microsoft.com/dotnet/standard/native-interop/customize-parameter-marshalling#custom-marshallers .

To marshal a string as UTF8 in .NET Standard, you can either specify [MarshalAs((UnmanagedType)48)] to force cast the value of LPUTF8Str, or use the new LibraryImport attribute with StringMarshalling = StringMarshalling.Utf8. See the documentation at https://learn.microsoft.com/dotnet/standard/native-interop/pinvoke-source-generation .

@Sewer56
Copy link
Contributor Author

Sewer56 commented Oct 19, 2024

That is right. I am also aware that there are better ways to express such a requirement in modern .NET.

I used this code specifically as an example because this is the wrapping code that mono/CppSharp produces by default when a C/C++ header lists a char* as parameter. I.e. all projects (libraries) deriving from CppSharp in last 10 years or so are affected.

I simply ran across the issue while working (using) one of these projects.

Given there didn't seem to be a relevant entry in list of breaking changes for .NET 9, I assumed this was unintentional, or not noted.

@huoyaoyuan
Copy link
Member

The format address value is 0xCDCDCDCDCDCDCDCD, which is used for uninitialized heap memory. It looks like some memory is not correctly filled.

@jkoritzinsky jkoritzinsky self-assigned this Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Interop-coreclr untriaged New issue has not been triaged by the area owner
Projects
Status: No status
Development

No branches or pull requests

4 participants