I/O Safety

[kennykerr]

Just a reminder to consider how we might integrate or leverage the concepts behind the I/O Safety RFC.

rust-lang/rust#87074

[kennykerr]

I had a brief look, but I don't think there's any relevant work for the windows-rs project at this time.

[sunfishcode]

The I/O safety feature has features intended to be useful in this use case, so I'm curious about whether you're thinking this isn't relevant at this time, or whether you see a reason they won't be a good fit in the future either.

It is at least the case that the I/O safety APIs aren't stable yet. They are currently expected to be stable in Rust 1.63 which won't be released until next month. And of course, even when it's stable, there will be MSRV concerns. But other than those, I'm curious if there are other concerns here.

[kennykerr]

No concerns just don't really see an obvious application at this level, but time will tell. The windows and windows-sys crates also have pretty low-level abstractions for handles and a lot more handle types than what this RFC covered last I checked. Of course, I haven't looked too deeply so if you have suggestions, I'm happy to hear them.

[sunfishcode]

As a few examples, Rust's internal FFI bindings are using HandleOrInvalid to describe the return value of CreateFileW, which is either a handle or INVALID_HANDLE_VALUE, and HandleOrNull to describe the return value of CreateThread, which is either a handle or NULL. Also it uses BorrowedHandle to describe ReadFile's handle argument, as well as a few others.

[ChrisDenton]

windows-rs recently gained a generic Borrowed<'a, T> which I think aims to fulfill a similar role to BorrowedHandle<'a> but is intended to be more general.

You wouldn't normally want to return HandleOrInvalid in a public API (converting to a Result is better). Maybe in windows-sys? But that tends to use more primitive types and has to be no_std compatible. I'm also unsure if the metadata says precisely which value is documented invalid for each value.

[sunfishcode]

ReadFile in the windows crate doesn't use Borrowed<'a, T>; it currently looks like this:

pub unsafe fn ReadFile<'a, Param0: IntoParam<'a, HANDLE>>(
    hfile: Param0, 
    lpbuffer: *mut c_void, 
    nnumberofbytestoread: u32,
    lpnumberofbytesread: *mut u32, 
    lpoverlapped: *mut OVERLAPPED
) -> BOOL

It looks like functions like this could use AsHandle in place of IntoParam, which would have the advantage of being directly usable with std::fs::File and more as the ecosystem starts adopting AsHandle.

In window-sys, ReadFile looks like this:

pub unsafe extern "system" fn ReadFile(
    hfile: HANDLE,
    lpbuffer: *mut c_void, 
    nnumberofbytestoread: u32,
    lpnumberofbytesread: *mut u32, 
    lpoverlapped: *mut OVERLAPPED
) -> BOOL

Except for the no_std requirement, it looks like functions like this could use BorrowedHandle in place of HANDLE.

One option would be to add a "std" feature where the std types are used, and then have windows-sys define its own version of the types in no_std mode. The io-lifetimes crate has implementations that would be pretty easy to copy, if there's interest. Granted, this has tradeoffs, but I'm interested in learning how the tradeoffs look here.

I'm also unsure if the metadata says precisely which value is documented invalid for each value.

I have this concern too. It may not have this information now, but this might be something worth thinking about. INVALID_HANDLE_VALUE vs. NULL is notoriously error-prone. It looks like most things in the windows crate that return a handle check for both NULL and INVALID_HANDLE_VALUE; that could be simplified if the metadata had this information.

[kennykerr]

The metadata does indeed include this information, which is where the windows crate gets it from.

[ChrisDenton]

I thought the generic is_invalid was used for functions like CreateFileW that return win32 handles? Which is likely fine because it tests for both -1 and 0.

[kennykerr]

There's no generic is_invalid method. They're all generated from metadata.

windows-rs/crates/libs/bindgen/src/handles.rs

Lines 25 to 56 in 56f7799

	let check = if underlying_type.is_pointer() {
	quote! {
	impl #ident {
	pub fn is_invalid(&self) -> bool {
	self.0.is_null()
	}
	}
	}
	} else {
	let invalid = gen.reader.type_def_invalid_values(def);
	if !invalid.is_empty() {
	let invalid = invalid.iter().map(|value| {
	let literal = Literal::i64_unsuffixed(*value);
	if *value < 0 && underlying_type.is_unsigned() {
	quote! { self.0 == #literal as _ }
	} else {
	quote! { self.0 == #literal }
	}
	});
	quote! {
	impl #ident {
	pub fn is_invalid(&self) -> bool {
	#(#invalid)||*
	}
	}
	}
	} else {
	quote! {}
	}
	};

For example, here's the metadata for Windows.Win32.Foundation.HANDLE:

[RAIIFree("CloseHandle")]
[InvalidHandleValue(-1L)]
[InvalidHandleValue(0L)]
[NativeTypedef]
public struct HANDLE
{
	public IntPtr Value;
}

[ChrisDenton]

Sorry, I worded that badly. The Handle::is_invalid method is used rather than anything specific to CreateFileW, which only documents -1 as being the error condition.

HandleOrInvalid could not possibly be used in this situation even if it was wanted because the metadata doesn't say that.

[ChrisDenton]

To put it another way, for CreateFileW the return type is documented as:

If the function succeeds, the return value is an open handle to the specified file, device, named pipe, or mail slot.

If the function fails, the return value is INVALID_HANDLE_VALUE. To get extended error information, call GetLastError.

But this information is not in the metadata:

[DllImport("KERNEL32", ExactSpelling = true, PreserveSig = false, SetLastError = true)]
[SupportedOSPlatform("windows5.1.2600")]
public unsafe static extern HANDLE CreateFileW([In][Const] PWSTR lpFileName, [In] FILE_ACCESS_FLAGS dwDesiredAccess, [In] FILE_SHARE_MODE dwShareMode, [Optional][In] SECURITY_ATTRIBUTES* lpSecurityAttributes, [In] FILE_CREATION_DISPOSITION dwCreationDisposition, [In] FILE_FLAGS_AND_ATTRIBUTES dwFlagsAndAttributes, [Optional][In] HANDLE hTemplateFile);

So windows-rs essentially tests both 0 and -1 (via is_invalid) as this will work for all fallible functions that return a HANDLE.

The HandleOrInvalid type is intended for cases where you know the function uses -1 to indicate an error. But the metadata does not tell you which functions those are.

[riverar]

But this information is not in the metadata

That function returns a HANDLE, which is annotated with that information. The InvalidHandleValue attribute indicates which values are considered invalid. You should not be comparing that to docs.

If CreateFile had incompatible sentinel values, it would have had its return type changed in metadata so that we could annotate it correctly.

[ChrisDenton]

Right, I'm explaining why windows-rs could not use the HandleOrInvalid type from the standard library.

To put it simply, HandleOrInvalid is equivalent to testing handle != -1.

Whereas is_invalid is equivalent to testing handle != -1 && handle != 0.

[riverar]

What I'm saying is your belief that -1 is somehow more precise/right is wrong. The metadata indicates 0 and -1 are invalid, so HandleOrInvalid cannot work with CreateFileW.

[ChrisDenton]

I did not express such a belief. Again, I'm trying to explain why windows-rs could not use HandleOrInvalid.

I'm sorry, but I'm unsure how else to state this more clearly.

[riverar]

I must have taken a wrong turn somewhere then, my mistake/ignore me!