microsoft / CsWin32

A source generator to add a user-defined set of Win32 P/Invoke methods and supporting types to a C# project.
MIT License
2k stars 84 forks source link

Unable to get dbcc_name from DEV_BROADCAST_DEVICEINTERFACE_W #1005

Closed a4a6cb31 closed 11 months ago

a4a6cb31 commented 11 months ago

Actual behavior

The generated struct includes the dbcc_name field as winmdroot.__char_1. It should be a null terminated string, but there's only ever one character there.

Expected behavior

I expect a char* with my full string available.

Repro steps

  1. NativeMethods.txt content:

    RegisterDeviceNotification
    DEV_BROADCAST_HDR
    DEV_BROADCAST_DEVICEINTERFACE_W
    WM_DEVICECHANGE
    DBT_DEVICEARRIVAL
  2. Any of your own code that should be shared?

Here's a quick WinForms example that should repro the issue. Plug in a flash drive with this running.

    public unsafe partial class Form1 : Form
    {
        public Form1()
        {
            InitializeComponent();

            DEV_BROADCAST_HDR dbi = new()
            {
                dbch_size = (uint)Marshal.SizeOf<DEV_BROADCAST_HDR>(),
                dbch_devicetype = DEV_BROADCAST_HDR_DEVICE_TYPE.DBT_DEVTYP_DEVICEINTERFACE
            };

            PInvoke.RegisterDeviceNotification((HANDLE)Handle, &dbi, REGISTER_NOTIFICATION_FLAGS.DEVICE_NOTIFY_ALL_INTERFACE_CLASSES);
        }

        protected override void WndProc(ref Message m)
        {
            if (m.Msg == PInvoke.WM_DEVICECHANGE && m.WParam.ToInt64() == PInvoke.DBT_DEVICEARRIVAL)
            {
                DEV_BROADCAST_DEVICEINTERFACE_W deviceInterface = Marshal.PtrToStructure<DEV_BROADCAST_DEVICEINTERFACE_W>(m.LParam);
                string badData = deviceInterface.dbcc_name.ToString();
            }

            base.WndProc(ref m);
        }
    }

If I declare my own struct as follows, the above code then works as I expect.

    [StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)]
    internal unsafe struct DEV_BROADCAST_DEVICEINTERFACE
    {
        public uint dbcc_size;
        public uint dbcc_devicetype;
        public uint dbcc_reserved;
        public Guid dbcc_classguid;
        public fixed char dbcc_name[255];
    }

Context

AArnott commented 11 months ago

It won't be a char*, since it's an inline array. But you're right that this is an issue. Duplicate of #387.

Ashfaaq18 commented 5 months ago

Hi @AArnott , I'm not sure if this is fully fixed. I'm working with some USB event notification code and I use the CsWin32 library as follows,

    case PInvoke.WM_DEVICECHANGE:
        {
            switch ((uint)wParam)
            {
                case PInvoke.DBT_DEVICEARRIVAL:
                    {
                        if (lParam != nint.Zero)
                        {
                            DEV_BROADCAST_HDR hdr = Marshal.PtrToStructure<DEV_BROADCAST_HDR>(lParam);
                            if (hdr.dbch_devicetype == DEV_BROADCAST_HDR_DEVICE_TYPE.DBT_DEVTYP_DEVICEINTERFACE)
                            {
                                DEV_BROADCAST_DEVICEINTERFACE_W devIntW = Marshal.PtrToStructure<DEV_BROADCAST_DEVICEINTERFACE_W>(lParam);
                                if (devIntW.dbcc_classguid.Equals(PInvoke.GUID_DEVINTERFACE_USB_DEVICE))
                                {
                                    unsafe
                                    {
                                        // This outputs => "\\"
                                        string name = new string((char*)&devIntW.dbcc_name);

                                        var address = nint.Add((nint)(&devIntW.dbcc_name), 1184);
                                        // This outputs => "\\?\USB#VID_18A5&PID_0243#07010A63CCD74E16#{a5dcbf10-6530-11d2-901f-00c04fb951ed}"
                                        //I looked around the memory of this address and found the dbcc_name far away from where it should be.
                                        //An offset of 1184 gets me the USB id
                                        name = new string((char*)address);
                                    }
                                }
                            }
                        }
                    }
                    break;

I have some more code where I extract the SP_DEVICE_INTERFACE_DETAIL_DATA_W's __char_1 DevicePath and return the null terminated string as " return new string((char*)&pD->DevicePath); " without a problem. I am lost as to why i should offset the this dbcc_name by 1184 to obtain the device_name. Any help/tips? is it something to do with the generated struct?

BartoszCichecki commented 2 months ago

I am seeing exactly the same issue. The name is waaaay further in memory than it should be. Here is the workaround I am using:

do not use this hack

This is with cswin32 0.3.106.

AArnott commented 2 months ago

The OP code was faulty. Using Marshal.PtrToStructure is inappropriate because the pointer given in LParam is a pointer to a buffer that starts with the struct but the struct does not define all the data, so PtrToStructure has no idea to copy the actual text of the device name. It is important to access the data where it originally is.

I tested this modified code and found it works:

protected override void WndProc(ref Message m)
{
    if (m.Msg == PInvoke.WM_DEVICECHANGE && m.WParam.ToInt64() == PInvoke.DBT_DEVICEARRIVAL)
    {
        ref DEV_BROADCAST_DEVICEINTERFACE_W deviceInterface = ref Unsafe.AsRef<DEV_BROADCAST_DEVICEINTERFACE_W>((void*)m.LParam);
        // The length of the entire structure (including trailing buffer) is given by dbcc_size.
        // We subtract the known size of the structure to get the length of the trailing buffer.
        // Then because the length is given in bytes, we divide by the size of a char to get the number of characters.
        int length = ((int)deviceInterface.dbcc_size - sizeof(DEV_BROADCAST_DEVICEINTERFACE_W)) / sizeof(char);
        string? name = deviceInterface.dbcc_name.AsSpan(length).ToString();
    }

    base.WndProc(ref m);
}

@bartoszcichecki, Your workaround seems extremely dangerous and you evidently are just scanning random memory. You shouldn't make any assumptions about the length of the string or where in memory it might have been copied to. The above code I provide seems to work precisely.

BartoszCichecki commented 2 months ago

Thanks for the correct solution! You are right this was a very dangerous hack.

By the way, would it make sense to generate convenience methods for stuff like this?

AArnott commented 2 months ago

I'm glad the solution works for you.

The VariableLengthInlineArray<T> type that CsWin32 emits is the best thing we have come up with so far to make this more convenient. The challenge is that there isn't a single consistent pattern for how native inline arrays are used, defined, sized, etc. We don't have the time budget to add special case convenience APIs for every win32 API, so we mainly focus on general patterns that we can apply to make things easier. FWIW, I don't think what we require users to do with our general APIs is worse than a C programmer. And our scope with CsWin32 is not to make interop easier in C# than C. We merely aim to expose the APIs so you don't have to write them yourself. But reading the API docs and carefully managing memory as necessary is still an exercise left to the reader.