Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[strict provenance] Review Windows HANDLE Types #95490

Open
Gankra opened this issue Mar 30, 2022 · 14 comments
Open

[strict provenance] Review Windows HANDLE Types #95490

Gankra opened this issue Mar 30, 2022 · 14 comments
Labels
A-strict-provenance Area: Strict provenance for raw pointers O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@Gankra
Copy link
Contributor

Gankra commented Mar 30, 2022

This issue is part of the Strict Provenance Experiment - #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:

// FIXME(strict_provenance_magic): we defined RawSocket to be a u64 ;-;
#[cfg(not(target_vendor = "uwp"))]
pub(crate) fn set_no_inherit(&self) -> io::Result<()> {
cvt(unsafe {
c::SetHandleInformation(self.as_raw_socket() as c::HANDLE, c::HANDLE_FLAG_INHERIT, 0)

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

@Gankra Gankra added O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue. A-strict-provenance Area: Strict provenance for raw pointers labels Mar 30, 2022
@ChrisDenton
Copy link
Contributor

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
Copy link
Contributor Author

Gankra commented Mar 30, 2022

(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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

  • Ptr->Int: fine
  • Int->Ptr and then using that pointer: please no no no

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
Copy link
Contributor

ChrisDenton commented Mar 30, 2022

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
Copy link
Contributor Author

Gankra commented Mar 30, 2022

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

@riverar
Copy link
Contributor

riverar commented Apr 4, 2022

[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
Copy link
Contributor

ChrisDenton commented Apr 5, 2022

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
Copy link
Member

thomcc commented Oct 23, 2022

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 #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
Copy link
Member

thomcc commented Oct 23, 2022

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
Copy link
Contributor

The windows-rs definition of LocalAlloc is a separate issue with the win32metadata (see microsoft/win32metadata#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
Copy link
Contributor

riverar commented Dec 25, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-strict-provenance Area: Strict provenance for raw pointers O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants