Bug: Multiple issues with the method signatures for #[implement(IStream)]

[wravery]

Which crate is this about?

windows

Crate version

0.30.0 and 0.32.0

Summary

Using 0.32.0 as a reference, here's the trait which describes IStream::Read and IStream::Write:

pub trait ISequentialStream_Impl: Sized {
    fn Read(&mut self, pv: *mut ::core::ffi::c_void, cb: u32, pcbread: *mut u32) -> ::windows::core::Result<()>;
    fn Write(&mut self, pv: *const ::core::ffi::c_void, cb: u32) -> ::windows::core::Result<u32>;
}

Problem 1

Read is supposed return S_FALSE when it reaches the end of the stream. There does not seem to be a way to return any success codes other than S_OK.

Problem 2

The caller can set the out-param for Write (or Seek, or a few others) to null. When the Vtbl wrapper invokes the Impl function (and it returns an Ok(u32) value), it assigns the result to the out-param pointer without checking for null and the app crashes:

        unsafe extern "system" fn Write<Identity: ::windows::core::IUnknownImpl, Impl: ISequentialStream_Impl, const OFFSET: isize>(this: *mut ::core::ffi::c_void, pv: *const ::core::ffi::c_void, cb: u32, pcbwritten: *mut u32) -> ::windows::core::HRESULT {
            let this = (this as *mut ::windows::core::RawPtr).offset(OFFSET) as *mut Identity;
            let this = (*this).get_impl() as *mut Impl;
            match (*this).Write(::core::mem::transmute_copy(&pv), ::core::mem::transmute_copy(&cb)) {
                ::core::result::Result::Ok(ok__) => {
                    *pcbwritten = ::core::mem::transmute(ok__);
                    ::windows::core::HRESULT(0)
                }
                ::core::result::Result::Err(err) => err.into(),
            }
        }

Expected behavior

For the first problem, I think maybe the return type should be Result<HRESULT>. I'm not sure why Read kept a separate out-param and Write turned it into a result, but in this case it would probably be a better signature if it kept the out-param and used the Result<> to pass additional success codes.

The second one seems like a flaw in the generated Vtbl implementation. Perhaps there should always be a null check before the result is assigned to an out-param in the Vtbl functions.

Actual behavior

It's impossible to accurately implement this interface right now with the #[implement] macro, and some of the optional out-param handling leads to access violations and crashes.

Additional comments

I was experimenting with this in this branch of a crate I just created: https://github.com/wravery/tempfile-istream/tree/windows-0.32.0. The main branch is still on 0.30.0.

[riverar]

Hey Bill 👋, good catch on the ISequentialStream::Read and ISequentialStream::Write. Appears this is a metadata bug (PreserveSig bit missing) that we can fix up quickly.

Regarding the optional out param, will defer to @kennykerr. We might already be tracking the impl. of OptionalAttribute as an issue elsewhere.

Additional context:

.method /* 06004F24 */ public hidebysig newslot abstract virtual 
	instance valuetype [Windows.Win32.winmd]Windows.Win32.Foundation.HRESULT Read (
		[out] void* pv,
		[in] uint32 cb,
		[out] [opt] uint32* pcbRead
	) cil managed 
{
	.param [1]
		.custom instance void [Windows.Win32.Interop]Windows.Win32.Interop.MemorySizeAttribute::.ctor() = (
			01 00 01 00 53 06 0f 42 79 74 65 73 50 61 72 61
			6d 49 6e 64 65 78 01 00
		)
} // end of method ISequentialStream::Read

.method /* 06004F25 */ public hidebysig newslot abstract virtual 
	instance valuetype [Windows.Win32.winmd]Windows.Win32.Foundation.HRESULT Write (
		[in] void* pv,
		[in] uint32 cb,
		[out] [opt] uint32* pcbWritten
	) cil managed 
{
	.param [1]
		.custom instance void [Windows.Win32.Interop]Windows.Win32.Interop.ConstAttribute::.ctor() = (
			01 00 00 00
		)
		.custom instance void [Windows.Win32.Interop]Windows.Win32.Interop.MemorySizeAttribute::.ctor() = (
			01 00 01 00 53 06 0f 42 79 74 65 73 50 61 72 61
			6d 49 6e 64 65 78 01 00
		)
} // end of method ISequentialStream::Write

[kennykerr]

Thanks for reporting - I'm almost entirely focused on implementation support now, so this is a helpful repro.

[kennykerr]

Took a look at this specifically.

Your first problem is blocked by microsoft/win32metadata#797

Your second problem is blocked by microsoft/win32metadata#805

[kennykerr]

Note to self: when the metadata bugs are fixed, add flags.optional() to this check:

windows-rs/crates/libs/metadata/src/signature.rs

Line 82 in 445247f

if flags.input() || !flags.output() || self.def.array_info() {

[wravery]

Your second problem is blocked by microsoft/win32metadata#805

Just double checking, but I think the optional attribute is correct in this case, and the SAL annotation is wrong/incomplete. The comments on MSDN say it can be NULL: https://docs.microsoft.com/en-us/windows/win32/api/objidl/nf-objidl-isequentialstream-write#parameters. Fixing that issue in the metadata might actually enforce this bug.

[kennykerr]

Right, sadly I can't fix the windows crate (by honoring the optional) as that hits a bunch of false positives, breaking other APIs.

[wravery]

If in doubt, would it make sense to generate a runtime null check in Rust?

[kennykerr]

@wravery do you want to give it another try? I still some fishy metadata around optional parameters but it looks like your specific issues have been resolved.

[kennykerr]

Feel free to reopen if you're still having issues with this interface. Looking at the latest metadata and code gen it looks correct.