rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
96.57k stars 12.48k forks source link

[strict provenance] Review Windows HANDLE Types #95490

Open Gankra opened 2 years ago

Gankra commented 2 years ago

This issue is part of the Strict Provenance Experiment - https://github.com/rust-lang/rust/issues/95228

Windows defines HANDLE to be void* and rust faithfully reproduces that. But now windows-rs is actually trying to insist it's an integer. Also there are other types like SOCKET that "are" HANDLEs for many purposes but are defined to be integers.

I filed an issue against windows-rs for this, detailing the many places that define typedefs for these types, which has some interesting discussion from Microsoft devs. I think the ultimate conclusion is that truly truly truly these are absolutely just integers and to the extent that the address of something might be embedded within, acting on that fact is being a smartass and basically just "abi crimes".

It's possible there is "nothing to do here" but at very least it's suboptimal for Rust and windows-sys to disagree on typedefs and if that's gonna happen we should at least Actively Decide To Do That instead of Accidentally Doing It.

I left a mildly-confused-at-the-time FIXME about this here:

https://github.com/rust-lang/rust/blob/3e7514670db841a7f0d7656f3b13b1c8b2c11599/library/std/src/os/windows/io/socket.rs#L132-L136

(Arguably the "real" issue is that we define RawHandle to be void*)

ChrisDenton commented 2 years ago

To sum up, yes standard* HANDLEs are just offsets into a table (i.e. integers) although that is an implementation detail. Either way they are opaque and not meant to be used as pointers or integers (except for testing against known special values, and maybe some bit tagging if you're into that sort of thing).

Though we'd have to be careful changing the global type. Some functions allow smuggling random pointers (and anything else) through arguments labelled as HANDLE types. I'm thinking this would mostly be a problem for callback functions.

* Note: I'm using "standard HANDLEs" to basically mean the types of handle being used in the standard library. There are all sorts of other HANDLEs (IIRC not currently used by the standard library) that may or may not alias the HANDLE type but are their own thing.

Gankra commented 2 years ago

(Also whoops I did the Worst Typo and the original version of the initial comment said "absolutely just pointers" when I meant to write "absolutely just integers". 🤦‍♀️)

ChrisDenton commented 2 years ago

Also it might be worth mentioning that there is a public HANDLE type (and associated "raw" traits) that probably can't change definition because stability. Although the new (currently unstable) Owned/Borrowed handle types might make that obsolete for kernel handles.

Lokathor commented 2 years ago

That HANDLE is correctly a pointer alias at least.

I think the main reason for a Handle type to be isize over a pointer is literally just Send/Sync.

ChrisDenton commented 2 years ago

Hm. I thought the intent here was to avoid unnecessary ptr to int (and vice versa) conversions?

A HANDLE (as used by standard library types) must never be actually used as a pointer but may sometimes need to treated as an integer for zero tests, equality testing and testing against known pseudo HANDLE values.

(Although I would reiterate that some APIs lie about their arguments and actually take a union of types)

Lokathor commented 2 years ago

So if things are ever a pointer, use a pointer. Even if they're also sometimes an int, still use a pointer.

(This was already said in another issue but by now there's a whole lot of issues so it's easy to miss).

ChrisDenton commented 2 years ago

In that case it doesn't really matter what windows-rs or the standard library use for actual kernel HANDLEs. Either or both will be fine from that point of view.

The only thing to be wary of is proper function definitions. And for that the C type may be unreliable.

Gankra commented 2 years ago

Yeah it's just annoying that the two will need a cast to interop.

riverar commented 2 years ago

[original post...] which has some interesting discussion from Microsoft devs

Full disclosure: I'm not a Microsoft employee. I've just been working with Windows for a billion years and am a current Windows Development MVP. So I wouldn't necessarily take anything I wrote as gospel... but I did draw from experience and did review Windows OS manuals going back to 1.0.

That HANDLE is correctly a pointer alias at least.

I'm not aware of any cases where a returned handle is designed/intended to be used as a pointer by the developer. (Do fact check me.) It would seem isize would be more appropriate right?

@Gankra Love what you're trying to accomplish here! Good hunting!

ChrisDenton commented 2 years ago

The issue, imho, is cases where a HANDLE type can, in some contexts, also mean any random pointer/data. Consider WriteFileEx which suggests using the hEvent member of OVERLAPPED for your "own purposes", which is then recovered in a FileIOCompletionRoutine.

thomcc commented 1 year ago

FWIW, it seems like on WINE these may be real pointers, even if they aren't under proper Windows. We've seen segfaults and the like from misuse of them under wine, for example in https://github.com/rust-lang/rust/issues/101474.

That probably means we should keep them as pointers everywhere. I don't know if windows-rs cares to support that, but in the stdlib we certainly do.

thomcc commented 1 year ago

I think the ultimate conclusion is that truly truly truly these are absolutely just integers and to the extent that the address of something might be embedded within, acting on that fact is being a smartass and basically just "abi crimes".

More concretely, my experience with these APIs is that there are plenty of places where they are just pointers even on windows. For example LocalAlloc will return a genuine pointer (not an integer) if the flags include LMEM_FIXED. GlobalAlloc works similarly too. My personal feeling is that the use of isize for HANDLE is incorrect under strict provenance, as these are really just integer|pointer types, e.g. should be represented as pointers under SP.

ChrisDenton commented 1 year ago

The windows-rs definition of LocalAlloc is a separate issue with the win32metadata (see https://github.com/microsoft/win32metadata/issues/1296). In any case it's not at all the same kind of "handle" as, say, CreateFileW returns. It even has a different type name in the actual header files.

riverar commented 8 months ago

I think the ultimate conclusion is that truly truly truly these are absolutely just integers and to the extent that the address of something might be embedded within, acting on that fact is being a smartass and basically just "abi crimes".

More concretely, my experience with these APIs is that there are plenty of places where they are just pointers even on windows. For example LocalAlloc will return a genuine pointer (not an integer) if the flags include LMEM_FIXED. GlobalAlloc works similarly too. My personal feeling is that the use of isize for HANDLE is incorrect under strict provenance, as these are really just integer|pointer types, e.g. should be represented as pointers under SP.

Apologies if this is repetitive, I've made similar comments elsewhere.

LocalAlloc has never, to my knowledge, been documented as returning a usable pointer regardless of fixed/movable flags. (It's documented as returning a handle in the Windows 1.03 SDK Programmer's Reference manual.) It appears the developers intended to always run these handles through LocalLock to do some housekeeping and produce a usable pointer.

image

That said, programming examples in the same SDK's Programmer's Guide, separate from the Reference, make use of this insider knowledge!

image

Despite this, I still don't believe we should start down the slippery slope of labeling handles based on their backing implementations or usage trends, both of which could change over time.