Document shift-left Win32 metadata annotation proposal - #2295
Document shift-left Win32 metadata annotation proposal#2295Jevan Saks (jevansaks) wants to merge 8 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 78d1f91d-a6a2-4264-b049-36c8d156ac9d
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 78d1f91d-a6a2-4264-b049-36c8d156ac9d
Jon Wiswall (jonwis)
left a comment
There was a problem hiding this comment.
Looks great, a few comments/nits. Looking forward to the update!
|
Can we also get missing APIs like |
|
I do not wish to scope creep this too much especially for the initial proposal but I did want to mention one thing that came up in rust's std recently, There is a desire for trivially safe functions to be defined as safe by the FFI bindings themselves. By "trivially safe" I mean functions like // SAFETY: Always safe to call.
let id = unsafe { GetCurrentThreadId() };There is no safety (or even validity) invariant to justify so it would be really nice if this could be handled by the bindings themselves if that's at all possible. |
|
I would love that as well - I suspect we would need some kind of similar annotation that the headers can used to mark certain functions as "always safe". |
|
AllJoyn is deprecated - https://learn.microsoft.com/en-us/uwp/api/windows.devices.alljoyn?view=winrt-22621 - and is no longer present in the Windows SDK as far as I can tell, although the documentation remains in place. How is the parser finding them? |
Jon Wiswall (@jonwis) -- win32metadata keeps the copy from the old SDK around. Going forward I would want to use the SDK as the source of truth and if AllJoyn is gone then it would consequently be gone from win32metadata as well. FYI Rafael Rivera (@riverar) as this was something you pushed for.
Kenny Kerr (@kennykerr) what annotation(s) would you recommend go into the SDK headers to indicate this?
Seems reasonable to me, but can you create a new issue on win32metadata for it (if it's not already in the backlog)? |
|
re: AllJoyn (and P2P, etc.) headers carried forward: Downstream consumers like If metadata generation is now going to be coupled to a single SDK's headers, I'd be curious to hear more about the back-compat story here. (The Windows SDK, I believe, is supposed to be backwards compatible but I never found an actual policy nor have I seen it behave that way.) Perhaps one option would be to move obsolete APIs into a separate metadata file (e.g., |
We do strive to be source and ABI back-compat. We have removed APIs before, but only after careful investigation. The SDK and API teams are very aware of the problems removing APIs apps depend on when running on older systems. IIRC the AllJoyn implementation & API definitions were removed quite a while ago after we discovered basically zero real-world use. The SDK's policy is to be cumulative, except when we deprecate things. Usually there's a one/two update cycle time between when something gets the deprecation tag and when it's fully removed. But, as we're shipping SDKs faster and have multiple in-market (even if not fully supported) versions of Windows this gets harder to maintain. Staying pinned to the older SDK is a workaround, but that makes folks' build environments hard. If there was an "obsolete.md", would that be sufficient for your purposes? (Also, if you're seeing APIs break compat or disappear on you from the SDK without the deprecation metadata/comments, please tag me directly.) |
|
Jon Wiswall (@jonwis) So I took a peek. AllJoyn headers appear to have been yanked from the SDK after the 22000 release (so 22621). Interestingly Windows 10 21H2/22H2, still in support via ESU / LTSC channels, is now untargetable with the current SDKs. So Windows SDK 22000 remains the last SDK to support Windows 10 targets. Newer SDKs break that compatibility. (Devs are sadly none the wiser because the SDK docs/versioning fail to communicate that and will find out the hard way.)
I didn't fully think it through, so push back is welcome--but I think an |
|
My thinking is that, like the Windows SDK, we would provide not just one win32metadata but N of them -- one for each of the Windows SDK deployments. Imagine we'd have a winmd per cpp package here https://www.nuget.org/packages/Microsoft.Windows.SDK.CPP/10.0.29648.1000-preview#versions-body-tab. Now I don't know whether we'd retroactively go back and generate such winmds -- we'd probably only do it going forward. Then a consumer that wants removed APIs would reference the old nuget package in that project, and if you want what's in the latest OS (which is still backwards compatible), you would choose latest winmd.
For CsWin32 you can grab a newer win32metadata than the default, i think it's hard to grab an older one so we'll have to do some work to enable that scenario.
In what way is it broken? The SupportedOSVersion metadata is something that we'd want to do a better job of encoding to communicate these concepts. But maybe you mean something else. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6f83e332-83a4-4848-a927-34089bec698f
I think one challenge with this approach is that a dev will get stuck on an old copy of the winmd. As we've seen here with win32metadata, metadata is always getting tweaked, so devs would be at a huge disadvantage.
A snippet from the download page: Says nothing about Windows 10 support getting dropped after 22000. The docs lie and say:
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6f83e332-83a4-4848-a927-34089bec698f
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6f83e332-83a4-4848-a927-34089bec698f
What do you mean "Windows 10 support is dropped"? You can compile for 22000 and set your MinVersion to 10.0.0.0 and run on Windows 10 just fine. Are you saying there are APIs for Windows 10 that have been dropped from the SDK, and thus those APIs are unsupported/unavailable on latest SDKs? |
|
Yes, if the SDK is missing Windows 10 APIs, that SDK can't be used to target that OS. (But I recognize it may work.) |
I don't have a strong opinion. Just something consistent with existing SAL annotations or MIDL annotations that we can definitively key off like
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6f83e332-83a4-4848-a927-34089bec698f
|
Kenny Kerr (@kennykerr) Can you help me understand the 'unsafe' requirement in windows-rs a little better? E.g. something like CompareStringOrdinal fully describes the buffer requirements and guarantees that it doesn't touch the provided buffers after the call, so can't the safety be unsafe-d away by a windows-rs-generated wrapper? It sounds like that isn't happening by default. Is it intentional and expected that everyone just blindly does the 'unsafe' attestation in their own code? Or for something like GetCurrentThreadId, it just seems like it is already known to be safe. Would it be better to take a stance that everything in the Windows SDK is safe unless otherwise specified (e.g. with the new Win32_Retained annotation, that would make it unsafe). In CsWin32 perhaps we're too optimistic but basically if all the buffers / pointers are properly annotated and we can project them to safe equivalents then it's CsWin32 that attests to the safety and the caller is relieved of that obligation. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6f83e332-83a4-4848-a927-34089bec698f
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6f83e332-83a4-4848-a927-34089bec698f
|
I think there are two different cases being mixed together here.
A generator could also produce a separate safe wrapper that accepts slices, derives and validates the lengths, and keeps the buffers alive while calling the raw API. SAL and a non-retention guarantee would provide some of the information needed to generate that wrapper. What I don't think is sound is treating complete SAL, or the absence of So I would favor an explicit Assuming everything in the SDK is safe unless annotated otherwise would put the default in the wrong direction: every missing or incomplete annotation would become a potential Rust soundness bug. |
📋 API Surface Diff✅ No API differences vs the baseline.
|
I claim that any API that's not explicitly documented as retaining pointers the app gave it is flawed. In a world of incoherent threading and memory management strategies, if the API doesn't say "you must keep what this points to alive until this other operation" then it would be impossible to deliver any software if Windows secretly smuggled away your allocations/pointers. My preference is again to use something like how the cleanup-types work. Annotate the OVERLAPPED in-pointer (for example) as "you must retain this until IO completes" (Read/WriteFile & DeviceIoControl say as much...) Should we also document all the APIs as to whether they are safe (or not) to call under the loader lock? Or the threading model all APIs expect? Or should we declare abberations to well-documented defaults? |
|
Agree Jon Wiswall (@jonwis)!
If there's "hard-to-express prerequisites" (and SAL has some of these via What I don't want is to pass along |
|
Weaving together an internal discussion with Jevan Saks (@jevansaks) and Jon Wiswall (@jonwis) for the benefit of getting more eyes on the question.
Just to clarify the I agree that non-retention can be a default contract: if an API retains a pointer beyond the call, that should require explicit annotation, and projectors should be able to rely on that guarantee. My concern is the broader jump from "this pointer is not retained" to "this API is safe Rust." The SDK headers currently omit some documented memory-safety requirements. These are not cases where Windows secretly retains an undocumented pointer. They are documented contracts that are not fully represented by the header declaration. Loader-lock restrictions that only risk failure or deadlock are different and do not necessarily make a Rust function Today So in principle I agree with the destination. If the headers provide an authoritative contract where every memory-safety obligation is either represented by metadata or covered by a documented default, and the projector can translate that contract into Rust ownership, lifetimes, types, and checks, then the resulting wrappers can be safe by default. APIs with prerequisites that cannot be represented would remain explicitly I just do not think the existing Win32 headers have that completeness property today. If absence of an annotation means "safe" before the headers have been audited against a complete contract, every omission becomes a Rust soundness bug. With unsafe-by-default, the same omission costs ergonomics until it is fixed. I suggest defining narrow defaults such as non-retention, adding metadata for ownership, invalidation, callback lifetime, selector-dependent parameters, concurrency, and similar contracts, and then auditing API families against that model. Once an API family is declared conforming, projectors can safely default its enforceable projections to safe. That gets us to the goal without assuming that decades of existing Win32 headers have already undergone that audit. Chris Denton (@ChrisDenton) am I being overly cautious? |
|
Any thoughts from some more knowledgeable Rust devs? Wesley Wiser (@wesleywiser) sivadeilra Daniel Paoliello (@dpaoliello) |
|
I agree with Kenny Kerr (@kennykerr)'s analysis. The risks for assuming safety by default are too high (rust has had CVEs for less). If every function has been stringently audited for safety and the annotations are rich enough to encapsulate the requirements and new APIs undergo such an audit then I guess there's not much difference either way. But until that happens it seems infinitely better to "fail-safe" (or well, |
I just don't see how the system is usable if you do that. Ok now the callers have to put "unsafe" on everything but how do they know what to put in the SAFETY comment to indicate they've thought through all the constraints when no constraints have been documented or provided? |
|
I think this is exactly the distinction Rust makes between an You're right that callers cannot responsibly explain an The same applies to the projector. Declaring the function safe is a promise that callers have no additional obligations whose violation could cause undefined behavior. If we do not know the complete contract, the projector cannot make that promise either. Lack of documented constraints is not evidence that no constraints exist. This is also why windows-rs provides dedicated crates such as The raw binding therefore remains |
|
Just to clarify my thinking here, there are three entities involved:
The metadata in the headers asserts an API as safe if all the safety conditions are both known and fully encoded in the metadata. The language projection will look at this and, if it can automatically fulfil the safety conditions, present a safe API to the user. If it cannot fulfil one or more of the conditions then it'll present an unsafe API to the user. It is then responsibility of the user to ensure their use meets the rest of the safety conditions. The projections could generate documentation based on the metadata so the user knows what they must do to make it safe. If the metadata does not know the full safety conditions for an API or cannot encode them fully in metadata then the API must be considered unsafe. The language projection must also consider it unsafe but may use any safety metadata there is to remove or document some of the unsafety. It is then the responsibility of the user to figure out how to make it fully safe. So there are three potential states for safety in the metadata:
It'd be really nice if all APIs were "verified safe" even if language projections can't always encapsulate the necessary safety conditions (generating safety documentation could help bridge that gap). But getting to that point is the hard bit and in the meantime the other states will need to be considered. |
|
Given metadata may be consumed by an open set of projectors, I wonder if a single notion of "safe" can really hold up across all of them.
|
|
Agreed, that's part of what I was trying to get at above. The metadata describes the contract. The language projection consumes it and can produce a safe or unsafe interface for the language based on it. An incomplete metadata contract (e.g. there are unknown requirements) would necessarily be So, for the metadata specifically, "safe" merely means "fully described by the metadata". It could use different term(s) to avoid confusion. I would prefer not to go down the |
Jon Wiswall (jonwis)
left a comment
There was a problem hiding this comment.
Looks fine to me; see comments about "how to indicate that calling this method requires a follow-on call to this other one" for things like OVERLAPPED.
|
(I had a response drafted which I don't see posted here, oops) Sounds like we're aligned with the annotations for the existing win32metadata, so I'm going to move forward with merging this in and starting to migrate the annotations. I'm open to adding more annotations for more preconditions/rules (e.g. a buffer parameter must not be accessible to other threads), but my short-term goal is getting us moved over to the Rust version of the parser/metadata generator tool and getting back to (as close to) parity with win32metadata. |
Summary
Key decisions captured
_Win32_prefix, such as_Win32_Retval_and_Win32_Retained_Agilemetadata rather than moving it into SDK headers_Win32_RAIIFree_(Closer, ...)HANDLEdeclarations and put lifecycle metadata only on producer returns and output parametersThis PR is documentation-only and is intended as the review package for the SDK and windows-rs design discussion.