search

microsoft / windows-rs

Rust for Windows
https://kennykerr.ca/rust-getting-started/
Apache License 2.0
10.39k stars 488 forks source link

IMMDevice.Activate fails with IAudioSessionManager2 #1354

Closed spineki closed 2 years ago

spineki commented 2 years ago

Hello, thank you for this amazing library,

I get an error while retrieving back a pointer that should be filled by the IMMDevice::Activate function.

Exception 0xc0000005 encountered at address 0x7ff64e1d65e7: Access violation reading location 0xffffffffffffffff
(exit code: 0xc0000005, STATUS_ACCESS_VIOLATION)

I have got this error on Windows 10 with the following configuration:

Here is a minimal (non!)working example. Code is commented to highlight the problem.

Cargo.toml

[package]
name = "my-package"
version = "0.1.0"
authors = ["spineki"]
edition = "2018"

[dependencies.windows]
version = "0.28.0"
features = [
  "Win32_Foundation",
  "Win32_Media_Audio",
  "Win32_Media_Multimedia",
  "Win32_System_Com",
  "Win32_System_Com_StructuredStorage",
]

Code:

use windows::Win32::{
    Media::Audio::{
        eCapture, eMultimedia, IAudioSessionManager2, IMMDeviceEnumerator, MMDeviceEnumerator,
    },
    System::Com::{
        CoCreateInstance, CoInitializeEx, CoUninitialize, CLSCTX_ALL, CLSCTX_INPROC_SERVER,
        COINIT_APARTMENTTHREADED,
    },
};

use windows::core::{Interface, GUID};

fn main() {
    unsafe {
        CoInitializeEx(std::ptr::null(), COINIT_APARTMENTTHREADED).expect("CoInitializeEx Failed");

        // Getting the device enumerator: works
        let imm_device_enumerator: IMMDeviceEnumerator =
            CoCreateInstance(&MMDeviceEnumerator, None, CLSCTX_INPROC_SERVER)
                .expect("CoCreateInstance Failed");

        // Getting the IMMDevice of the defaultAudioEndpoint: works
        let endpoint = imm_device_enumerator
            .GetDefaultAudioEndpoint(eCapture, eMultimedia)
            .expect("GetDefaultAudioEnpoint Failed");

        // preparing a pointer that will store the address of the created IAudioSessionManager2
        let mut pinterface: *mut std::ffi::c_void = std::ptr::null_mut();

        // Activating: the target Interface is IAudioSessionManager2: No error!
        endpoint
            .Activate(
                &IAudioSessionManager2::IID,
                CLSCTX_ALL.0,
                std::ptr::null_mut(),
                &mut pinterface as *mut _,
            )
            .expect("Activate Failed");

        // -> Reaching this point, so Activate did not failed

        // Casting back to IAudioSessionManager2: works
        let audio_session_enumerator_ref = (pinterface as *mut IAudioSessionManager2)
            .as_ref()
            .expect("Pointer to ref failed");

        // -------------------- HERE IS THE PROBLEM-------------------- 
        // The call to GetSessionEnumerator fails.
        let audio_session_enumerator = audio_session_enumerator_ref
            .GetSessionEnumerator()
            .expect("GetSessionEnumerator Failed");
        CoUninitialize();
    }
}
MarijnS95 commented 2 years ago

Afaik IAudioSessionManager2 already holds the pointer, so this should be a cast/transmute to IAudioSessionManager2 instead of *mut IAudioSessionManager2.

spineki commented 2 years ago

Ideed. Thank you so much @MarijnS95 !

For future readers that didn't find an example online like me before,

here is what it looks like

Before

let audio_session_enumerator_ref = (pinterface as *mut IAudioSessionManager2)
    .as_ref()
    .expect("Pointer to ref failed");

// -------------------- HERE IS THE PROBLEM-------------------- 
// The call to GetSessionEnumerator fails.
let audio_session_enumerator = audio_session_enumerator_ref
    .GetSessionEnumerator()
    .expect("GetSessionEnumerator Failed");

After

let audio_session_enumerator_ref =
    std::mem::transmute::<*mut std::ffi::c_void, IAudioSessionManager2>(pinterface);

// The call to GetSessionEnumerator now works!!
let audio_session_enumerator = audio_session_enumerator_ref
    .GetSessionEnumerator()
    .expect("GetSessionEnumerator Failed");

Works like a charm, I can call for example

let count = audio_session_enumerator.GetCount();

println!("count {:?}", count);
MarijnS95 commented 2 years ago

I presume this might be a case of fn Activate lacking a _COM_Outptr annotation (and/or the proper uuid, ppv signature) otherwise this should have been turned into a borrow of a generic T: Interface (or a return of that generic T).

Glad you got there in the end with the raw pointer though!

kennykerr commented 2 years ago

Generally, COM out params will return ownership of a ref-counted object. You should prefer to do something like this:

let mut pinterface: Option<IAudioSessionManager2> = None;

endpoint.Activate(
    &IAudioSessionManager2::IID,
    CLSCTX_ALL.0,
    std::ptr::null_mut(),
    &mut pinterface as *mut _ as _)
    .expect("Activate Failed");

Now the pinterface will own the resulting interface pointer (if any). It is regrettable that the GUID*, void** pair aren't at the back of the parameter list. In such cases, the method is much easier to call because the Windows crate will take care of the binding automatically. I may ask the win32 metadata folks about making this easier.

kennykerr commented 2 years ago

@MarijnS95 yes, the metadata is missing the attribute as well, but it is also affected by the fact that the GUID parameter isn't the second-to-last, as is conventional.

MarijnS95 commented 2 years ago

@kennykerr if the iid parameter were to be annotated, or the _com_outptr contains an index argument of said iid parameter, this is could perhaps work for any signature. Would the win32metadata folks be open to add this (probably through an override / heuristics at first) followed by the SDK headers directly?

kennykerr commented 2 years ago

I'd prefer to avoid adding new attributes if possible. I think perhaps if the void** parameter had the ComOutPtr attribute (which this method currently lacks) I could then just walk backward and look for a GUID* parameter instead of expecting it to be the second-to-last. I'd have to experiment to see how reliable that is.

kennykerr commented 2 years ago

Opened https://github.com/microsoft/win32metadata/issues/752 to discuss metadata improvements.

riverar commented 2 years ago

I think perhaps if the void** parameter had the ComOutPtr attribute (which this method currently lacks) I could then just walk backward and look for a GUID* parameter instead of expecting it to be the second-to-last. I'd have to experiment to see how reliable that is.

Agree, thought of that too when I was building the DIA sample that has a NoRegCoCreate workflow. Am also concerned about false positives, we'd have to take a closer look.

e.g. 

HRESULT STDMETHODCALLTYPE NoRegCoCreate(
  const __wchar_t *dllName,
  REFCLSID   rclsid,
  REFIID     riid,
  void     **ppv);
kennykerr commented 2 years ago

Reopening as https://github.com/microsoft/windows-rs/issues/1354#issuecomment-975895875 can now be implemented (hopefully) to enable this API to be transformed.

kennykerr commented 2 years ago

Another example is D2D1CreateFactory but it lacks the ComOutPtr attribute. https://github.com/microsoft/win32metadata/issues/956

MarijnS95 commented 2 years ago

@kennykerr has DXC meanwhile been annotated? I still have a patch sitting that annotates some of the open source repo but that got stuck around nullability vs error cases, iirc.

kennykerr commented 2 years ago

I'm not sure what you're referring to...

MarijnS95 commented 2 years ago

I think it was https://github.com/microsoft/windows-rs/pull/1088#issuecomment-907808506.

kennykerr commented 2 years ago

Currently signatures with a trailing pair of GUID*, void** params that include the appropriate attributes like ComOutPtr will get the query transformation. You can use a tool like ILSpy to quickly confirm the metadata. For example, here's how ILSpy displays the signature for CoCreateInstance:

[DllImport("OLE32", ExactSpelling = true, PreserveSig = false)]
[SupportedOSPlatform("windows5.0")]
public unsafe static extern HRESULT CoCreateInstance([In][Const] Guid* rclsid, [Optional][In] IUnknown pUnkOuter, [In] CLSCTX dwClsContext, [In][Const] Guid* riid, [Out][ComOutPtr] void** ppv);

I'm using this issue to track adding support for more interesting cases where the GUID* and void** parameters are not in this conventional position, functions like D2D1CreateFactory:

[DllImport("d2d1", ExactSpelling = true, PreserveSig = false)]
[SupportedOSPlatform("windows6.1")]
public unsafe static extern HRESULT D2D1CreateFactory([In] D2D1_FACTORY_TYPE factoryType, [In][Const] Guid* riid, [Optional][In][Const] D2D1_FACTORY_OPTIONS* pFactoryOptions, [Out] void** ppIFactory);

Does that cover your case? If not, please open a new issue so I can look into that.

kennykerr commented 2 years ago

@MarijnS95 #1805 affects one or two DXC APIs - let me know if that resolves your concern.

MarijnS95 commented 2 years ago

@kennykerr I don't think the problem is related then. The signatures of those functions were initially detected correctly, but disappeared again once windows-rs started looking for ComOutPtr which the DXC APIs lack... I have a pending patch to add them but that stagnated.

I don't think my questions really fit windows-rs and should otherwise be small enough to sneak in here, no need to create a separate issue (if anything, they're perhaps better suited elsewhere):

kennykerr commented 2 years ago

Sorry, I'm not very proficient with SAL annotations and I don't know where DXC comes from. You could try asking on the win32metadata repo as those are the same folks who own the Windows SDK.

damyanp commented 2 years ago

_COM_Outptr_ parameters must set the output value to null if they return failure. This is alluded to in the documentation where it says:

The returned pointer has COM semantics, which is why it carries an _On_failure_ post-condition that the returned pointer is null.

In sal.h it says:

// Annotations for _Outptr_ parameters which return a pointer to a ref-counted COM object,
// following the COM convention of setting the output to NULL on failure.
// The current implementation is identical to _Outptr_result_nullonfailure_.
// For pointers to types that are not COM objects, _Outptr_result_nullonfailure_ is preferred.

These DXC functions are returning pointers to interfaces that follow COM semantics, and so _COM_Outptr_ can be used here. The definitions in sal.h are also useful to try and figure out what these annotations mean:

#define _COM_Outptr_                        _SAL2_Source_(_COM_Outptr_, (),                      _Outptr_                      _On_failure_(_Deref_post_null_))
#define _COM_Outptr_result_maybenull_       _SAL2_Source_(_COM_Outptr_result_maybenull_, (),     _Outptr_result_maybenull_     _On_failure_(_Deref_post_null_))
#define _COM_Outptr_opt_                    _SAL2_Source_(_COM_Outptr_opt_, (),                  _Outptr_opt_                  _On_failure_(_Deref_post_null_))
#define _COM_Outptr_opt_result_maybenull_   _SAL2_Source_(_COM_Outptr_opt_result_maybenull_, (), _Outptr_opt_result_maybenull_ _On_failure_(_Deref_post_null_))

_COM_Outptr_ must be passed in as a non-null value, and if the function succeeds it will be set to a value and if it fails it will be set to null.

_COM_Outptr_opt_ may be null. If it is non-null then it behaves as for _COM_Outptr_.

The maybenull versions are as above, but the function is allowed to succeed and set the parameter to null.

I suggest you ask on the dxc repo about when they're next planning to update the SDK with the latest DXC changes.

MarijnS95 commented 2 years ago

@damyanp Thanks for the links and detailed info! I've now worked this into https://github.com/microsoft/DirectXShaderCompiler/pull/4524 - it's a bit hacky and quick but I found that I couldn't spend much more time on it than that. It seems the particular function I was having issues with has been overridden in win32metadata anyways (I asked this before: will superfluous annotations be removed there if they have found their way into the headers?), yet I actually need the typedef now which doesn't have it yet :grimacing:

damyanp commented 2 years ago

I'm happy it was useful.

will superfluous annotations be removed there if they have found their way into the headers?

Is the question here about whether or not we'll clean up the overrides in win32metadata once the headers are updated? If so, then this is something that the win32metadata people can help with.

If I've misunderstood the question, then sorry - could you help me understand it?

MarijnS95 commented 2 years ago

Is the question here about whether or not we'll clean up the overrides in win32metadata once the headers are updated?

Yes.

If so, then this is something that the win32metadata people can help with.

I don't need an answer per-se, just curious. Since this can be implemented in the tooling itself I figure it's either already available or will be implemented once (the size of) the overrides gets out of hand :)

riverar commented 2 years ago

If the headers get updated and ship in the Windows SDK, the changes should just propagate through win32metadata when the targeted SDK is bumped.

MarijnS95 commented 2 years ago

@riverar I'm not worried about that, it'll happen at some point presuming my patches ever get merged (just that it is obviously cleaner to not have lingering overrides when that happens). There's a common theme though with my submissions to Microsoft repositories getting ignored, though.

riverar commented 2 years ago

Is any patch work still needed? (Superfluous annotations?) If we're still missing something in either repo, feel free to holler.

MarijnS95 commented 2 years ago

Is any patch work still needed?

https://github.com/microsoft/DirectXShaderCompiler/pull/4524 Would need to get merged, land in win32metadata and finally end up here. And it's useless to me unless windows-rs wants to pick up the arguably-niche #1835.

(Superfluous annotations?)

Nothing that needs to happen, just curious. It seems that https://github.com/microsoft/win32metadata/pull/890/files#diff-12f77aafef2c6ee2fa3a7c437254fc543c6e4246ffee487af8ae24db6d8d5bf0R877-R878 is superfluous if/when https://github.com/microsoft/DirectXShaderCompiler/pull/4524 trickles down into win32metadata, correct?

Regardless, we're straying very far off course for this issue :)