diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 41f6a149a..117c87126 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -165,6 +165,23 @@ jobs: RUST_BACKTRACE: 1 RUST_LIB_BACKTRACE: 1 run: cargo test --workspace --locked --no-fail-fast + # The UNC current-directory test is ignored by default because it needs a + # reachable administrative share (`\\localhost\C$`), which an ordinary + # developer account does not have -- running it by default would report an + # environment limitation as a defect. The runner is an administrator, so + # the coverage is deterministic HERE, which is where it has to be: this is + # the only test that puts `GetFullPathNameW` under a UNC current directory, + # and the guarantee it pins ("a root-relative path takes the ROOT of the + # current directory, which is a share root and not a drive") is public. + # + # A FAILURE HERE IS A RESULT. If the share stops being reachable on the + # runner the test says so in those terms, and the answer is to provision a + # UNC fixture rather than to delete the assertion. + - name: cargo test (namespace-request, UNC current directory) + env: + RUST_BACKTRACE: 1 + RUST_LIB_BACKTRACE: 1 + run: cargo test -p windows-namespace-request-sys --locked --test unc_current_directory -- --include-ignored # windows-file-watcher-example-test-harness is a normal workspace member # that depends on windows-file-watcher with the test-util feature, so # the two `--workspace` steps above unify that feature onto diff --git a/CHECKLIST-thread-ambient.md b/CHECKLIST-thread-ambient.md index 1f0dd6377..ecdddab31 100644 --- a/CHECKLIST-thread-ambient.md +++ b/CHECKLIST-thread-ambient.md @@ -160,7 +160,7 @@ pool, or async anywhere near it. The family grows by one entry per Win32 call. | 6 | `GetFileInformationByHandle` (non-Ex) | watcher | `BY_HANDLE_FILE_INFORMATION`; a distinct call, not a class of entry 5 | | 7 | `GetFinalPathNameByHandleW` | watcher directly, Globazog via `std::fs::canonicalize` | `VOLUME_NAME_DOS \| FILE_NAME_NORMALIZED` | | 8 | `GetVolumeInformationByHandleW` | watcher | handle-based, not the path-based `GetVolumeInformationW` | -| 9 | `GetFullPathNameW` | enumeration | lexical only | +| 9 | `GetFullPathNameW` | enumeration | result not verified | Four audit findings that shape the milestones below, recorded because each contradicts an assumption the first draft of this plan was written on. @@ -417,9 +417,13 @@ Entries 5-9 of the audited list. All but the last take a handle, so all but the filesystem name. Handle-based; the path-based `GetVolumeInformationW` is deliberately not in round one because no audited consumer calls it. -- [x] **M26.5** -- The `GetFullPathNameW` entry. Lexical only: it resolves relative components and `.`/`..` - and never expands a drive letter, so it does **not** close the session-relative hazard from M20.1, and - its documentation must say which problem it solves and which it leaves standing. +- [x] **M26.5** -- The `GetFullPathNameW` entry. Does not verify its result: it collapses `.`/`..` + lexically and roots most paths that are not fully qualified against process state -- the current + directory, or for a drive-relative path naming another drive the entry recorded for that drive, + while on the current drive that entry makes no difference -- and never expands a + drive letter, so it + does **not** close the session-relative hazard from M20.1, and its documentation must say which + problem it solves and which it leaves standing. - [x] **M26.6** -- Acceptance, in **two** parts, because the audit had two purposes and checking only the first is how the coverage question got missed once already. diff --git a/CHECKLIST.md b/CHECKLIST.md index be63f3ad9..4d01818e8 100644 --- a/CHECKLIST.md +++ b/CHECKLIST.md @@ -59,8 +59,8 @@ they are deliberately separate from building the new facility, which cannot star - [ ] **M20.1** -- Decide what the namespace facility does with a session-relative drive letter, and record it as a decision rather than leaving the absence of one implicit. Path resolution follows the impersonated token's logon session (measured: under a token from another logon session with unchanged - local access, the global `C:` resolved and a `subst` letter did not), and `GetFullPathNameW` is lexical - so submission-time canonicalisation does not expand the letter. `QueryDosDeviceW` distinguishes a real + local access, the global `C:` resolved and a `subst` letter did not), and `GetFullPathNameW` never + expands a drive letter, so submission-time canonicalisation does not expand it either. `QueryDosDeviceW` distinguishes a real local volume, a `subst`, and a network mapping cheaply, so detection is settled and only the response is open: expand to a session-independent form at submission, or reject at admission with a typed error. Expansion is not uniform -- a network mapping becomes a UNC path, a local volume becomes a device path @@ -195,3 +195,24 @@ Ungated work with no identified predecessor deliverable. probe because a `LOGON32_LOGON_NEW_CREDENTIALS` token answered the question with a passing control, so the fallback was redundant -- not because the crash was understood. Parked rather than dropped so the unexplained result is not mistaken for a tested one. + +- [ ] **M-inf.2** -- Archive the eight completed milestone groups in + [CHECKLIST-thread-ambient.md](CHECKLIST-thread-ambient.md) into + [COMPLETED-CHECKLIST.md](COMPLETED-CHECKLIST.md). + + **Raised by a review that named one item, and measured to be eight groups.** The comment asked for + M26.5's completed multi-line body to be replaced by a one-line stub, per the checklist-hygiene rule + that an active checklist is an action queue. That rule is right and the file does violate it -- but + M26.5 is not exceptional: its five siblings in M26 are written the same way, so stubbing only the + reported item would have made it inconsistent with the group it belongs to rather than more + consistent with the rule. + + Counted rather than assumed, every group in the file is complete and due for migration under the + "move the completed group" rule: M22 (8 items), M23 (6), M24 (6), M25 (7), M26 (6), M27 (6), + M28 (4) and M29 (5). Only `M26+` has open items, and it is what keeps the file alive. + + Not taken in PR #86 because that branch corrects `GetFullPathNameW` documentation and touched + M26.5 only to fix one technical premise inside it. Migrating roughly 400 lines of another feature's + bookkeeping through it would bury the change it exists to make. The migration is mechanical, is its + own commit, and needs the group headings dated per the archive format -- date-only on the `## Moved` + line, with any precise timestamp reserved for an anchored item heading. diff --git a/DESIGN-NOTES.md b/DESIGN-NOTES.md index 5cb85f68c..2ce928d50 100644 --- a/DESIGN-NOTES.md +++ b/DESIGN-NOTES.md @@ -1459,9 +1459,18 @@ Two corollaries that decide the design: - **Submission-time canonicalisation does not close this.** The path must be resolved on the calling thread at submission, because the process current directory is mutable by any thread and even perfect remoting would be racy -- - but `GetFullPathNameW` is *lexical*. It resolves relative components and - `.`/`..` and never expands a drive letter, so the "canonical" path still - carries a session-relative reference. + but `GetFullPathNameW` never expands a drive letter, so the "canonical" path + still carries a session-relative reference. (It is not *lexical* as a whole + either, which matters elsewhere but not here: it collapses `.`/`..` lexically + but roots most paths that are not fully qualified against process state -- the + current directory, or for a drive-relative path naming a drive OTHER than the + current one the entry recorded for that drive, which moves independently of + it; on the current drive that entry makes no difference and the process + current directory wins -- and that rooting is the property + submission-time resolution buys. See + `windows-namespace-request-sys`' + [DESIGN-NOTES.md](crates/windows-namespace-request-sys/DESIGN-NOTES.md) -> + `D-18`.) - **The extended-length prefix does not help.** `\\?\Z:\dir` still resolves `Z:` through the device map; the prefix skips Win32 normalisation, not object-manager resolution. Only UNC, `\\?\Volume{GUID}\`, and diff --git a/crates/windows-namespace-request-sys/CHECKLIST.md b/crates/windows-namespace-request-sys/CHECKLIST.md new file mode 100644 index 000000000..fd0864cd5 --- /dev/null +++ b/crates/windows-namespace-request-sys/CHECKLIST.md @@ -0,0 +1,11 @@ +# Checklist: windows-namespace-request-sys + +Design decisions are in [DESIGN-NOTES.md](DESIGN-NOTES.md), and how they were +reached in [DESIGN-RATIONALE.md](DESIGN-RATIONALE.md). This crate's *creation* is +tracked separately, in the workspace +[CHECKLIST-thread-ambient.md](../../CHECKLIST-thread-ambient.md) milestones +M24-M26; that file is feature-scoped and is deleted when its feature completes, +so durable follow-up work for the crate belongs here instead. + +No open milestones. Completed work is in +[COMPLETED-CHECKLIST.md](COMPLETED-CHECKLIST.md). diff --git a/crates/windows-namespace-request-sys/COMPLETED-CHECKLIST.md b/crates/windows-namespace-request-sys/COMPLETED-CHECKLIST.md new file mode 100644 index 000000000..01500bc5a --- /dev/null +++ b/crates/windows-namespace-request-sys/COMPLETED-CHECKLIST.md @@ -0,0 +1,59 @@ +# Completed checklists: windows-namespace-request-sys + +Append-only. Newest groups at the bottom. + +## Moved 2026-09-10 -- NR-1: the per-drive current-directory arm, pinned + +### NR-1.1 -- Pin the `=X:` arm of drive-relative rooting. *(completed 2026-09-10 12:24:46 UTC-04:00)* + +- [x] **NR-1.1** -- Decide how this crate's tests may control process-global + state, then pin the `=X:` arm of drive-relative rooting. + + **Completed in the round that raised it, because the blocker turned out not to + exist.** The item was written to defer the work: pinning the arm needs a + controlled `=X:`, and the two routes to one -- mutating process-global state + that other test threads share, or spawning a child process with a crafted + environment -- looked like a decision about this crate's test shape rather than + something to take in passing. + + Measurement dissolved the first route's objection within the hour. + `GetFullPathNameW` **itself** writes the `=X:` entry -- when resolving for a + drive other than the current one, and when the recorded entry is absent or + rejected, in which case it is written as the drive root. (An accepted entry is + left alone, and the current-drive form writes nothing.) The code under test + therefore already mutates that state on the very path these tests exercise, so + a test that sets it first introduces no hazard that resolving alone did not, + and there was nothing left to decide. Isolation across the tests comes from + their disjoint drive letters. + + `a_drive_relative_path_uses_that_drives_entry_verbatim_and_rewrites_a_bad_one` + in [drive_entry.rs](src/full_path/tests/drive_entry.rs) now pins all three behaviours: an entry + naming an existing directory is honoured verbatim (onto a *different* drive, + which is what makes "that drive's own current directory" a convention rather + than a guarantee); an entry naming nothing is rejected in favour of the drive + root; and the call writes the entry back, creating it on a host that had none. + It draws its drive letter from a list disjoint from every sibling test's, so + no two can select the same one and race under libtest's thread-per-test model. + + *(Later correction, recorded here because the archive is history and the + history was briefly wrong: this entry described the selection as a `W`/`U` + **pair**, matching the helper as written. That helper validated only its first + letter and returned the second unchecked, so the guarantee the pair implied did + not hold. The lists are now three letters each, every candidate is checked + against both the current drive and the probe drive, and the lists themselves + live in one table in [drive_entry.rs](src/full_path/tests/drive_entry.rs) with a test enforcing + that they stay disjoint and long enough. This note deliberately does NOT + enumerate them: an earlier version did, and named five lists after a sixth had + been added -- so a reader picking letters for a seventh would have consulted + an inventory missing three of the letters already in use. The table is the + inventory.)* + + The sibling test keeps its weaker form and now says so: without controlling + the entry it can only bound the arm. + + *(Later correction: that sibling was named + `a_drive_relative_path_is_rooted_at_that_drive_and_not_the_process_directory`, + and the name claimed two things its assertions do not reach -- `ends_with` + accepts any base, including the process directory, and an accepted entry is + used verbatim so the result need not be on that drive at all. It is now + `a_drive_relative_path_carries_its_component_and_the_current_drive_uses_the_process_directory`.)* diff --git a/crates/windows-namespace-request-sys/COMPLETED-PLANS.md b/crates/windows-namespace-request-sys/COMPLETED-PLANS.md new file mode 100644 index 000000000..8a3ca5ac6 --- /dev/null +++ b/crates/windows-namespace-request-sys/COMPLETED-PLANS.md @@ -0,0 +1,5 @@ +# Completed plans: windows-namespace-request-sys + +| Path to CHECKLIST.md | Completion Date | Brief description | Design Notes | +|---|---|---|---| +| [CHECKLIST.md](CHECKLIST.md) | 2026-09-10 | NR-1: pinned the `=X:` arm of drive-relative rooting. Raised as deferred work and completed the same round, because the blocker dissolved on measurement -- `GetFullPathNameW` writes the per-drive entry itself, so a test that sets it adds no hazard. The test now pins verbatim honouring, drive-root fallback, and the write-back. | [DESIGN-NOTES.md](DESIGN-NOTES.md#d-18), [DESIGN-RATIONALE.md](DESIGN-RATIONALE.md) | diff --git a/crates/windows-namespace-request-sys/Cargo.toml b/crates/windows-namespace-request-sys/Cargo.toml index 0dc6fa943..7cf955800 100644 --- a/crates/windows-namespace-request-sys/Cargo.toml +++ b/crates/windows-namespace-request-sys/Cargo.toml @@ -39,6 +39,21 @@ wtf-string = { version = "0.1.0", path = "../wtf-string" } [dev-dependencies] windows-thread-ambient-sys = { version = "0.2.0", path = "../windows-thread-ambient-sys" } +# Test-only, for the hidden `=X:` per-drive current-directory entries. Rust's +# `std::env` cannot reach them at all -- it rejects a key containing `=` -- and +# they are the state `GetFullPathNameW` reads and rewrites when it roots a +# drive-relative path, so a test that does not control them can only bound that +# behaviour rather than pin it. A dev-dependency, so no consumer inherits the +# feature. +[dev-dependencies.windows-sys] +version = "0.61.2" +default-features = false +# `Win32_Foundation` is named here rather than inherited from the dependency +# below: reading an entry has to clear and re-read the last error to tell an +# empty value from an absent one, and a test target must not depend on a +# feature staying enabled for a reason unrelated to it. +features = ["Win32_Foundation", "Win32_System_Environment"] + [dependencies.windows-sys] version = "0.61.2" default-features = false diff --git a/crates/windows-namespace-request-sys/DESIGN-NOTES.md b/crates/windows-namespace-request-sys/DESIGN-NOTES.md index a911d2830..6353f89c5 100644 --- a/crates/windows-namespace-request-sys/DESIGN-NOTES.md +++ b/crates/windows-namespace-request-sys/DESIGN-NOTES.md @@ -534,11 +534,142 @@ proven correct -- a bad trade on a crate whose buffers are handed to the kernel. If it is ever done, it must be done together with the size passed to Win32, never to one side alone. +## D-18: `GetFullPathNameW` is not lexical, and the genuinely lexical alternative is the wrong call + +**The correction.** This crate described `GetFullPathNameW` as **lexical** in +**nine places across five files**, in a sentence that then went on to say it +resolves against the process current directory. Those two claims disagree, and +the call is better described as doing two separable things. It collapses +`.`/`..` and normalizes separators, which *is* lexical -- `C:\a\..\b` becomes +`C:\b` whatever the current directory is, and whether or not `C:\a` exists. It +*also* **roots** most paths that are not fully qualified, and that reads mutable +process state. Three forms use it three different ways -- though not from three +different sources, since the first two both derive from the process current +directory and so does the current-drive case of the third: a relative path +takes the current directory; a root-relative path like `\foo` takes only that +directory's *root*, which is `\\server\share\` when the current directory is a +UNC path and so is not a drive at all; and a drive-relative path such as +`C:foo` takes the entry recorded for that drive -- usually that drive's own +current directory, though the entry is used verbatim and an accepted one may +name a directory on another drive entirely. That rule has two arms: +for a drive other than the current one Windows reads the hidden `=C:` entry +recorded for it, which moves independently of the process current directory; +for the *current* drive the entry makes no difference to the result and the +process current directory wins. Measured -- setting `=Q:` while the process is +on `Q:` changes nothing. ("Makes no difference" rather than "is ignored" for the +same reason the probe no longer says "without consulting a device": an entry +that is read and then discarded is indistinguishable from one never read.) So the call is not lexical *as a whole*, and the claim that +holds unqualified is that it **does not verify what it produces** -- the +documented guarantee, and narrower than the "touches no filesystem" an earlier +draft claimed. A black-box success cannot establish that broader claim, and it +did not need to: the drive-relative form *disproves* it outright, as the +measurement below records. + +**"Most" rather than "every", because a legacy device name short-circuits the +rooting.** `CON` resolves to `\\.\CON` and is not rooted, so it is an +unqualified input that is nonetheless invariant -- which is why the rooting +clause cannot be stated unconditionally. The form is looser than exact match: +`CON:`, `CON.`, `con` and `CONIN$` all map, while `CON.txt`, `a\CON` and +`CON:x` root normally. This matters to a crate that prepares paths on a +caller's behalf: `prepare("CON")` returns a device. + +**`NUL` is the one member that clause does not describe**, and the clause was +written from `CON` and stated of the whole set. Measured across all eight +accepted names: seven root normally once anything precedes them, and `NUL` +short-circuits as the final component of any path -- `\NUL`, `.\NUL`, `a\NUL` +and `C:\NUL` all reach `\\.\NUL`, where the `CON` spellings root. Only a suffix +(`NUL.txt`, `NUL:x`) takes it out. So a **fully qualified** path can still +resolve to a device, which is the part a caller needs: a rooted result is not by +itself evidence that a name refers to a file on that volume. Pinned by +`nul_is_the_one_device_word_a_path_around_it_does_not_save`. + +Keeping the two halves apart matters, because the decision below turns on the +rooting half alone. Saying the call resolves `.`/`..` "against the current +directory" -- as a first draft of this correction did -- attributes process-state +dependence to the one operation that has none, which is the same imprecision +running the other way. + +The wrong word had spread well beyond the one file that was reported. The sweep +that found the rest, and its arithmetic, are Tier 2: +[DESIGN-RATIONALE.md](DESIGN-RATIONALE.md) -> `D-18`. + +**The decision: keep `GetFullPathNameW`.** Two canonicalizers that do not root +exist -- `PathCchCanonicalizeEx` and `PathAllocCanonicalize`. They are the wrong +call here for a semantic reason: resolving against the current directory *at +submission* is what this crate is buying. A canonicalizer that does not root +would leave a relative path relative, so its meaning would be settled on the +worker at execution time, against a current directory any thread may have +changed in between -- which is exactly the race preparation exists to close. What +they omit is the part that is wanted. + +**No cost comparison is claimed, and that is deliberate.** An earlier draft of +this decision called the alternatives "cheaper". Nothing here benchmarks them, +Microsoft documents behaviour rather than relative cost, and +`PathAllocCanonicalize` allocates its own result -- so the word was a guess +wearing the clothes of a measurement, in a decision whose whole subject is not +doing that. It is also unnecessary: the rooting semantics decide this alone. +Neither is reliably free of process state either, since +`PATHCCH_ALLOW_LONG_PATHS` makes `PathCchCanonicalizeEx` consult the process +long-path setting unless the FORCE variant is used. + +Recorded with the alternatives named so the next reader does not re-derive it. +If the reasoning is ever wrong -- a consumer wanting a pure string operation, +having resolved relativity another way -- they are named here. + +**It does touch the filesystem, on one form -- measured, after four drafts said +otherwise.** Resolving a drive-relative path for a drive that is *not* the +current one checks that drive's `=X:` entry against the filesystem *and* against +a required shape. An accepted entry is used **verbatim** and need not be on that +drive -- with `=X:` set to `C:\Windows`, `X:foo` is `C:\Windows\foo`. Anything +rejected is replaced by the drive root, and the entry is **written** back there +-- created when absent, so this happens on a pristine host too. + +Both halves of the gate were measured, and one of them refuted a draft of this +very decision. Existence matters: a missing directory and an existing *file* are +each rejected. Shape matters too, and independently -- `C:/Windows/System32`, +`C:\Windows\System32\.`, `C:\Windows\System32\..\System32` and +`\\?\C:\Windows\System32` were all rejected while naming the same existing +directory that `C:\Windows\System32` was accepted for. So the draft calling this +"a filesystem query rather than a syntax test" named a mechanism the evidence +contradicts: it is both, and the list is observation rather than specification. The rewrite mutates the process environment +block as a side effect of what reads like a pure query. For the current drive +the entry makes no difference to the result and is not rewritten -- stated as +those two measured effects rather than as "not consulted", because installing an +entry and observing the outcome cannot separate "not read" from "read and +ignored", and a draft of this very paragraph said "not consulted" anyway. + +Earlier drafts concluded the opposite by reasoning that the current directory +lives in the PEB and the `=X:` variables in the environment block, so both are +ordinary process memory. The reasoning was sound and the conclusion wrong. That +is the failure this decision exists to name: a mechanism argued from where the +data lives rather than measured. It also means the guarantee this crate relies +on has to be the narrow one -- the call does not *verify* what it produces -- +because the broad one is not merely unproven but false. + +**The constraint this decision carries, and not just its conclusion:** state +only what the evidence reaches. Nine drafts of this entry each named a +mechanism it did not -- the call's nature, what a number measured, what the +alternatives cost, whether any filesystem was touched. The wordings differ; the +error does not. A reader taking only "it is not lexical" away from D-18 has the +answer without the thing that kept producing wrong ones. + +The constraint reaches further than prose, and the sharpest case was not about +this call at all: a test helper collapsed an *empty* `=X:` entry into an absent +one on a comment that called the equivalence measured, when the measurement had +never cleared the last error that distinguishes them. So **"measured" is itself +a claim, and a procedure can be wrong in ways its result never shows.** The +practical consequence was that restoring a borrowed empty entry deleted it. See +[DESIGN-RATIONALE.md](DESIGN-RATIONALE.md) -> "The measurement that was itself +unmeasured". + +The drafts themselves, and why each failed, are Tier 2: +[DESIGN-RATIONALE.md](DESIGN-RATIONALE.md) -> `D-18`. + ## Open, and inherited rather than introduced - **Path resolution under a captured identity.** A path must be resolved on the calling thread, because the process current directory is mutable by any - thread -- but `GetFullPathNameW` is lexical and never expands a drive letter, + thread -- but `GetFullPathNameW` never expands a drive letter, and drive-letter resolution follows the *impersonated* token's logon session. So a root resolved on a submitter and opened on a worker under a captured token can name a different device. The workspace has this as an open decision; diff --git a/crates/windows-namespace-request-sys/DESIGN-RATIONALE.md b/crates/windows-namespace-request-sys/DESIGN-RATIONALE.md new file mode 100644 index 000000000..26bbe2f0a --- /dev/null +++ b/crates/windows-namespace-request-sys/DESIGN-RATIONALE.md @@ -0,0 +1,219 @@ +# Design rationale: windows-namespace-request-sys + +Tier 2. How the decisions in [DESIGN-NOTES.md](DESIGN-NOTES.md) were reached -- +alternatives considered, drafts that were wrong, and the reasoning that got +discarded along the way. Consulted for "why", never for "what is true now": if +this file and Tier 1 disagree, **Tier 1 wins**. + +Cross-referenced by decision ID. + +## D-18: `GetFullPathNameW` is not lexical + +The decision is in [DESIGN-NOTES.md](DESIGN-NOTES.md) -> `D-18`. What follows is +the record of getting there, which is unusually worth keeping because the same +mistake recurred in nine successive wordings, enumerated below. + +### The shape of the error, which never changed + +Every wrong draft did the same thing: **named a mechanism the evidence did not +reach.** The subject moved -- what the call does, what a number measures, which +alternatives cost less -- but the failure did not. That is why the decision +records the constraint rather than only the conclusion: a reader who takes only +"it is not lexical" away from D-18 has the answer without the thing that keeps +producing wrong answers. + +### The drafts + +1. **"This call is lexical."** The original text, contradicted by its own next + sentence, which said it resolves against the process current directory. A + lexical canonicalizer is a pure function of its input; this reads mutable + process state. + +2. **"It resolves relative components and `.`/`..` against the process current + directory."** The first correction, which overshot. Collapsing `.`/`..` has + an output that is a function of the input alone -- `C:\a\..\b` becomes + `C:\b` under any current directory, and whether or not `C:\a` exists. + Measured under two different current directories. + + **This entry itself carried the error it describes, for twenty-three + rounds.** It read "pure string work and reads no process state at all", and + two different current directories giving the same answer shows the output + does not depend on that state, not that nothing was read. Every correction in + this file was aimed at the *rooting* half; the overreach in the **lexical** + half was the sentence doing the correcting, which is why nobody looked at it. + Whether rooting reads process state is a separate question, and it is + answered observably: changing the current directory changes the result. + +3. **"The probe measures roughly 212 ns per resolution."** It does not. The + probe reports a construct-and-drop cycle whose total contains an allocation + and a drop, and its own output says so. + +4. **"The probe deliberately declines to decompose that total."** An + over-correction of (3). It does decompose: the report states that recycling + an already-resolved path pays ~42 ns against ~210 ns and attributes the + difference to the resolution. What the probe withholds is the **mechanism** -- + whether any of it is a kernel transition -- not the division. + +5. **"Roughly 168 ns is this call's measured share."** Taking (4)'s split at face + value. The subtraction spans the whole preparation step, which makes two heap + allocations of the crate's own against the clone's one, plus a builder chain. + Timed directly with no allocation in the loop, the call is about 110 ns -- + the draft overstated it by roughly half. + +6. **"Either alternative is cheaper."** Said of `PathCchCanonicalizeEx` and + `PathAllocCanonicalize` from the first commit onwards, and never measured. + Nothing in this repository benchmarks them, Microsoft documents behaviour + rather than relative cost, and `PathAllocCanonicalize` allocates its own + result. Removed rather than substantiated, because the decision rests on + rooting semantics and never needed it. + +7. **"Touches no filesystem."** The claim that survived longest, because it is + nearly right. What Microsoft documents is that the function does not verify + that the resulting path is valid or names an existing file -- a statement + about *verification*, not about I/O. + +8. **"Observation cannot close that gap."** The correction to (7), and wrong in + a more interesting way: observation *did* close it, in the affirmative + direction, the moment anyone looked at the right form. Resolving `X:foo` for + a non-current drive distinguishes an existing directory from an existing + *file* from a missing one, and rewrites the `=X:` entry when it is not a + directory. That is a filesystem query and a write to the process environment, + in a call four drafts had described as reading process memory. + + The reasoning behind those drafts was sound -- the current directory is in + the PEB, the `=X:` variables are in the environment block, both are process + memory -- and the conclusion was false. It is the cleanest example in this + file of the standing failure: a mechanism argued from where the data lives + rather than measured. Every earlier draft at least *knew* it was asserting a + mechanism; this one thought it was declining to. + +9. **"A filesystem query rather than a syntax test."** Written in the paragraph + correcting (8), and wrong the same way within a single round. Having found + that an entry naming a missing directory or an existing *file* is rejected, + the draft concluded the gate was existence and not shape. Measured, shape + gates it too and independently: `C:/Windows/System32`, + `C:\Windows\System32\.`, `C:\Windows\System32\..\System32` and + `\\?\C:\Windows\System32` are each rejected while naming the same existing + directory that `C:\Windows\System32` is accepted for. Acceptance is also + literal -- `C:\Windows\` yields `C:\Windows\\foo`, with no normalisation at + the join. + + The tell was the word *rather*. Ruling an alternative out is a strictly + stronger claim than establishing the one you measured, and needs its own + evidence; three observations of the existence check said nothing about + shape. Both halves are now pinned by + `a_rejected_drive_entry_is_replaced_by_the_drive_root` so the next draft + cannot restate this from memory. + +### Two facts that were measured, then asserted too narrowly + +Both were found by review after the correction had already shipped, and both +were *enumerations* -- which is the form this kind of error likes. + +- **The rooting forms.** Stated first as one case (the current directory), then + two, and finally three: a relative path takes the current directory, a + root-relative path takes only that directory's *root* (which is + `\\server\share\` under a UNC current directory, so "current drive" was + wrong), and a drive-relative path naming a drive OTHER than the current one + takes the entry recorded for that drive from `=C:`, while on the current drive + that entry makes no difference and the process directory wins. The + current-drive arm is a fourth correction to the same enumeration, found the + same way as the first three and after them: the count "three forms" was itself + one of the things stated more confidently than measured. + +- **The device set.** The short-circuit was first described as "exact-match + only", which `CON:` disproves; then enumerated as `CON`/`NUL`/`PRN`/`AUX`/ + `COM1`-`9`/`LPT1`-`9`/`CONIN$`/`CONOUT$`, which omits the superscript + spellings `COM\u{00b9}`, `COM\u{00b2}`, `COM\u{00b3}` and their `LPT` + equivalents. Those are exactly the members a hand-written denylist misses, and + the documentation asserted a closed list without them until a review measured + it. The tests in [tests.rs](src/full_path/tests.rs) now pin every documented + spelling so the next omission fails CI instead of a review -- each positive + spelling against the device path it produces, and each negative one against + its full rooted result. + + **A third correction, and the one that shows why "the device set" was the + wrong unit all along.** A review asked for the documented `\CON` case to be + pinned, since the doc stated it and no test covered it. Writing that + assertion as the general rule the request was phrased in -- "a root-relative + device word roots at the current directory's root" -- would have pinned a + false claim. Measured across all eight accepted names, `NUL` alone + short-circuits as the final component of ANY path: `\NUL`, `.\NUL`, `a\NUL` + and `C:\NUL` all reach `\\.\NUL`, while every `CON`, `PRN`, `AUX`, `CONIN$`, + `CONOUT$`, `COM1` and `LPT1` spelling roots. So a fully qualified path can + still name a device. + + Every earlier entry here is a claim generalised from too few observations. + This one was generalised from one MEMBER to a set, which is the same error in + a dimension nobody had checked -- the enumeration of the set was audited + twice, and the assumption that its members behave alike never was. + + That distinction between kinds of pinning was itself a later correction, and + it is the reason the sentence now says which KIND of pinning each case gets. The claim "pins every + documented spelling" was written while one negative spelling, `CON:x`, was + covered only by `!starts_with("\\.\\")` -- a predicate the unrooted literal + satisfies. So the claim was true of the list and false of the strength, which + is the same shape as the trimming generalisation two entries below. + +### The measurement that was itself unmeasured + +The sharpest instance in this whole sequence is not about `GetFullPathNameW` at +all. The test helper that reads a `=X:` entry folded "empty value" and "absent +name" into one answer, and said so in a comment that called the equivalence +*measured*: `SetEnvironmentVariableW(name, "")` was reported to succeed and then +read back exactly as a name that was never set. + +It does not. `GetEnvironmentVariableW` returns `0` for both, and the last error +is the only thing that separates them -- so a measurement that never cleared the +last error first could read nothing but whatever an earlier call had left there. +Cleared and re-measured, the two are distinct, for an ordinary name and an `=X:` +name alike: + +| state | return | last error | +|---|---|---| +| set to `""` | `0` | `ERROR_SUCCESS` | +| deleted | `0` | `ERROR_ENVVAR_NOT_FOUND` | + +An earlier note also recorded that `ERROR_ENVVAR_NOT_FOUND` "never surfaced even +for genuinely absent variables", which has the same cause and one more: the +deletion under test had not happened, because the null that deletes an entry had +been marshalled as an empty string instead. Two layers of the harness agreeing +with each other is not a measurement. + +The consequence was live rather than cosmetic. `BorrowedDriveEntry` restores a +borrowed entry on unwind precisely so a panicking test cannot leak process +state; with the two answers collapsed, restoring an inherited *empty* entry +**deleted** it. The guard destroyed the state it existed to preserve, in exactly +one case, and only that case. + +Pinned by `an_empty_drive_entry_is_distinguished_from_an_absent_one` in +[drive_entry.rs](src/full_path/tests/drive_entry.rs), and verified by re-introducing the collapse, +which makes it fail. + +The general form is worth keeping separately from the specific fact: **a +comment that says "measured" is a claim about a procedure, and the procedure can +be wrong in ways the result never reveals.** Every other entry in this file is an +assertion that outran its evidence. This one had evidence, and the evidence was +of something else. + +### The sweep, and its arithmetic + +The wrong word had spread well beyond where it was reported. The consuming +probe's checklist item named [full_path.rs](src/full_path.rs) only; that file +held **three of the nine** on its own -- the module doc and both doc examples -- +with two more in [path.rs](src/path.rs), two in +[tests.rs](src/full_path/tests.rs), one in an acceptance comment and one in +[DESIGN-NOTES.md](DESIGN-NOTES.md). Nine across five files, from a report naming +one. The reported site was a sample, not the population. + +### Why the alternatives were never seriously in contention + +`PathCchCanonicalizeEx` and `PathAllocCanonicalize` canonicalize without +rooting, which sounds like a strictly smaller job and therefore an easy win. It +is the wrong trade for this crate: a path that is still relative has its meaning +settled on the worker thread at execution time, against a current directory any +thread may have changed in between. That is precisely the race preparation +exists to close, so the "smaller job" omits the part being bought. + +Recorded here rather than only in Tier 1 so the next reader who notices the +cheaper-looking API does not have to re-derive why it was declined. diff --git a/crates/windows-namespace-request-sys/PLANS.md b/crates/windows-namespace-request-sys/PLANS.md index de5fb4ac0..f1b36375e 100644 --- a/crates/windows-namespace-request-sys/PLANS.md +++ b/crates/windows-namespace-request-sys/PLANS.md @@ -2,12 +2,20 @@ Design decisions are in [DESIGN-NOTES.md](DESIGN-NOTES.md). -This crate's work is planned in a feature-scoped checklist at the workspace root -rather than a local one, because it lands alongside a sibling crate and a set of -workspace-level corrections, and the workspace root is their lowest common -source-component. That file is not the workspace +This crate is planned in two files, and the split is deliberate. Its *creation* +lives in a feature-scoped checklist at the workspace root, because it lands +alongside a sibling crate and a set of workspace-level corrections whose lowest +common source-component is the workspace root; that file is deleted when its +feature completes. *Durable follow-up work* therefore cannot live there, and has +a local checklist instead. Neither is the workspace [CHECKLIST.md](../../CHECKLIST.md), which holds unrelated deferred work. +The local [CHECKLIST.md](CHECKLIST.md) has no open milestone, so it has no row +below: its one plan so far is finished and recorded in +[COMPLETED-PLANS.md](COMPLETED-PLANS.md). The file stays because the queue is +durable even when it is empty, and a row here would give the same path two +incompatible states across the two indexes. + | Path to CHECKLIST.md | Status | Brief description | Design Notes | |---|---|---|---| | [../../CHECKLIST-thread-ambient.md](../../CHECKLIST-thread-ambient.md) | in progress | **This crate's part (M24-M26) is complete**: the foundations (owned handle duplication, security attributes, path preparation, the faithful-execution contract), the four handle-producing entries, the five query entries, a test seam, and an acceptance pass over both operation and scenario coverage. The checklist itself stays open for M27 (`windows-platform-probes`) and the `M26+` items gated on this branch merging with `main` -- including `M26+.3`, the merge-or-delete decision on this crate's duplicated path preparation. | [DESIGN-NOTES.md](DESIGN-NOTES.md) | diff --git a/crates/windows-namespace-request-sys/src/full_path.rs b/crates/windows-namespace-request-sys/src/full_path.rs index b3f79a06b..34222dc89 100644 --- a/crates/windows-namespace-request-sys/src/full_path.rs +++ b/crates/windows-namespace-request-sys/src/full_path.rs @@ -7,15 +7,235 @@ //! //! # What it solves, and what it leaves standing //! -//! This call is **lexical**. It resolves relative components and `.`/`..` -//! against the process current directory, and it touches no filesystem: it will -//! happily resolve a path to something that does not exist. +//! This call **does not verify what it produces**: it will happily resolve a +//! path to something that does not exist, and it reports no error for one. +//! +//! That is the documented guarantee, and it is deliberately narrower than +//! "touches no filesystem", which earlier revisions of this doc claimed. +//! Microsoft specifies that the function does not verify that the resulting +//! path and file name are valid or that they name an existing file; it does not +//! specify that no I/O occurs. +//! +//! **And on one form it demonstrably does touch the filesystem.** Resolving a +//! drive-relative path for a drive other than the current one validates that +//! drive's recorded entry against the filesystem, and rewrites it when the +//! entry does not name an existing directory -- see "The drive-relative form +//! writes process state" below. So the narrow guarantee is the one to rely on +//! precisely because the broad one is false, not merely unproven. A caller +//! wanting existence must still open. +//! +//! It does **two** things, and keeping them apart is the whole reason this +//! entry exists: +//! +//! 1. It rewrites the string. `.` and `..` are collapsed, `/` becomes `\`, and +//! trailing dots and spaces are trimmed -- but **not uniformly across +//! components**, and an earlier revision of this list said so without +//! qualification. Measured: the *final* component loses any run of trailing +//! dots and spaces (`C:\name...` and `C:\name ` both become `C:\name`), +//! while an *intermediate* component loses a single trailing dot and nothing +//! else -- `C:\a.\b` becomes `C:\a\b`, but `C:\a...\b` and `C:\a \b` are +//! returned unchanged. This part's **output is a function of the input +//! alone**: `C:\a\..\b` becomes `C:\b` whatever the current directory +//! happens to be, and whether or not `C:\a` exists. +//! +//! Stated that way deliberately. Earlier revisions said it "reads no process +//! state", which the evidence does not reach: varying the current directory +//! and getting the same answer shows the output does not DEPEND on it, not +//! that nothing was read. That is the same overreach this doc removes from +//! the current-drive entry below, and it sat here in the positive half while +//! seven reviews corrected the negative one. Invariance is the whole claim, +//! and it is also all a caller needs: this half can be reasoned about +//! without knowing the process's state. +//! 2. It **roots** a path that is not fully qualified, using mutable process +//! state -- and on one form it also *changes* that state. There are three +//! such forms: +//! +//! * A relative path like `rel.txt` is rooted at the *process current +//! directory*. +//! * A root-relative path like `\foo` takes only the *root* of that +//! directory, giving `C:\foo` rather than its subtree -- and +//! `\\server\share\foo` when the current directory is a UNC path, which +//! is why this says root and not drive. +//! * A drive-relative path like `C:foo` is rooted at the entry Windows +//! keeps for that drive in the hidden `=C:` environment variables. For +//! the *current* drive that entry makes no difference to the result and +//! the process current directory wins. +//! +//! **A whole class of input short-circuits both.** When the input names a +//! legacy device and nothing else, it resolves into the device namespace and is +//! not rooted at all: `CON` becomes `\\.\CON`, not a file under the current +//! directory. +//! +//! "And nothing else" is doing real work, and is looser than it first looks. +//! These all reach a device: a bare name (`CON`), a trailing colon (`CON:`, +//! `CON::`), trailing dots or spaces (`CON.`, `CON `), and any casing +//! (`con`). These do not, and root normally: anything with more of a path +//! around it (`CON.txt`, `a\CON`, `.\CON`, `CON:x`), and `\CON`, which +//! becomes `Q:\CON` for a current directory on `Q:`. +//! +//! **`NUL` does not follow that second list, and it is the only member that +//! does not.** The paragraph above was written from `CON` and stated of the +//! whole set; measured across all eight accepted names, seven behave as it +//! says and `NUL` short-circuits as the *final component of any path*, +//! however much path is in front of it: +//! +//! | input | `CON` | `NUL` | +//! |---|---|---| +//! | `X` | `\\.\CON` | `\\.\NUL` | +//! | `\X` | `Q:\CON` | `\\.\NUL` | +//! | `.\X` | `Q:\...\CON` | `\\.\NUL` | +//! | `a\X` | `Q:\...\a\CON` | `\\.\NUL` | +//! | `C:\X` | `C:\CON` | `\\.\NUL` | +//! | `X.txt` | rooted | rooted | +//! | `X:x` | rooted | rooted | +//! +//! So a *fully qualified* path can still resolve to a device, which is the +//! part worth knowing: `prepare(r"C:\NUL")` yields `\\.\NUL`, and a caller +//! treating a rooted path as proof it names a file on that volume is wrong for +//! this one name. Only a suffix (`NUL.txt`, `NUL:x`) takes it out. +//! +//! **Everything above describes what this call returns, and a review asked +//! whether that is a safe boundary for what a later `CreateFileW` does.** +//! Measured on this build, at the open rather than the resolver: creating +//! `\NUL` returns a `FILE_TYPE_CHAR` handle and leaves nothing on disk, +//! while `\CON`, `\CON.txt` and `\NUL.txt` each create an +//! ordinary `FILE_TYPE_DISK` file. The two layers agree -- the reservation +//! lives in *rooting*, so a name that roots normally opens normally, and `NUL` +//! reaches the device at both layers. +//! +//! That agreement is a measurement of one build, not a guarantee this crate +//! makes. The durable statement is the narrower one: these paragraphs describe +//! the RESOLVER's output. A caller sanitising untrusted names should decide +//! against what it will do with the result, not infer open-time safety from a +//! resolved spelling. +//! +//! **Do not build a name filter from the list below.** The accepted names are +//! `CON`, `NUL`, `PRN`, `AUX`, `CONIN$`, `CONOUT$`, and `COM`/`LPT` +//! followed by a single digit -- where "digit" includes the *superscripts* +//! `COM\u{00b9}`, `COM\u{00b2}` and `COM\u{00b3}` as well as `1`-`9`. Those are +//! written as Rust escapes deliberately: spelled `COM^1` with a caret, as an +//! earlier revision had them, a reader copying the text gets an ordinary +//! filename rather than a device. +//! An exhaustive scan of the character after `COM` accepts exactly +//! U+0031-U+0039, U+00B2, U+00B3 and U+00B9 on the tested build; `COM0` and +//! `COM10` are not devices. The superscripts are precisely the sort of member a +//! hand-written denylist omits, and this documentation asserted a list without +//! them until a review measured it -- so treat the set as *observed on one +//! build*, and prefer letting this call answer the question over reimplementing +//! its judgement. +//! +//! So the call is **not** lexical as a whole, and describing it that way -- as +//! an earlier revision of this doc did, in the sentence immediately before the +//! one describing the current directory it reads -- loses exactly the half that +//! matters here. A fully-qualified input resolves to the same output every +//! time; an input that is rooted resolves to different outputs in the same +//! process at different times, and pinning *that* is the property being bought. +//! (Not every unqualified input is rooted, which is the point of the device +//! short-circuit above: `CON` is unqualified and yet invariant.) //! //! So it solves exactly one problem -- the process current directory is shared //! mutable state that any thread can change, so a relative path means something //! different depending on *when* it is resolved. Performing this on the //! submitting thread pins that meaning. //! +//! # Why not a genuinely lexical canonicalizer +//! +//! Two exist: `PathCchCanonicalizeEx` and `PathAllocCanonicalize`. Both +//! canonicalize the string without rooting it. +//! +//! **They are the wrong call here, and the reason is a semantic difference, not +//! a cost one.** Resolving against the current directory *at submission* is what +//! this crate is buying. A lexical canonicalizer would leave a relative path +//! still relative, so its meaning would be decided on the worker thread at +//! execution time, against a current directory any thread may have changed in +//! between -- reintroducing exactly the race preparation exists to close. What +//! they omit is the part that is wanted. +//! +//! **No cost comparison is claimed, deliberately.** Nothing in this repository +//! benchmarks either alternative, Microsoft documents behaviour rather than +//! relative cost, and `PathAllocCanonicalize` allocates its own result -- so +//! "cheaper" would be a guess. It is also not needed: the decision rests on the +//! rooting semantics alone. Nor is either one reliably free of process state, +//! since `PATHCCH_ALLOW_LONG_PATHS` makes `PathCchCanonicalizeEx` consult the +//! process long-path setting unless the FORCE variant is used. +//! +//! Recorded so the next reader does not re-derive it. If this reasoning is ever +//! wrong -- for a consumer that genuinely wants a pure string operation and has +//! resolved relativity some other way -- the alternatives are named here. +//! +//! # The drive-relative form writes process state, and touches the filesystem +//! +//! Measured, and it overturns what four earlier revisions of this doc asserted. +//! Resolving `X:foo` for a drive that is **not** the current one does not +//! merely read the `=X:` entry: +//! +//! * An **accepted** entry is used **verbatim**, including a directory on a +//! *different* drive. With `=X:` set to `C:\Windows`, `X:foo` resolves to +//! `C:\Windows\foo`, so "that drive's own current directory" describes the +//! convention the entry usually holds, not a guarantee about the result. +//! Verbatim really means verbatim: `C:\Windows\` yields `C:\Windows\\foo`, +//! with no normalisation at the join. +//! * Otherwise the entry is **written** to the drive root and that is used -- +//! created when absent, so this happens on a pristine host and not only on +//! one carrying a stale entry. The write mutates the process environment +//! block as a side effect of what reads like a pure query. +//! +//! **Acceptance needs both a shape and an existence check, and the observed +//! necessary conditions are worth listing because they are not guessable.** An +//! entry naming a directory that exists is still rejected unless it is already +//! in fully-qualified `X:\...` form: measured on one build, `C:/Windows/System32`, +//! `C:\Windows\System32\.`, `C:\Windows\System32\..\System32` and +//! `\\?\C:\Windows\System32` were each rejected while naming the same existing +//! directory that `C:\Windows\System32` was accepted for. An existing *file* and +//! a missing directory are rejected too, so existence is checked as well -- but +//! saying the gate is "a filesystem query rather than a syntax test", as a draft +//! of this doc did, states a mechanism the evidence contradicts. It is both, and +//! this list is a set of observations rather than a specification. +//! +//! For the current drive neither happens, and the guarantee is stated at the +//! boundary observation can actually reach: **the entry makes no difference to +//! the result, and is not rewritten.** Both halves are measured -- an entry the +//! non-current arm would honour verbatim is installed and the process directory +//! wins anyway, and an entry the non-current arm would replace is left +//! untouched. Whether Windows *reads* it internally is not established, because +//! setting a value and observing the result cannot separate "not read" from +//! "read and ignored". An earlier revision said "not consulted", which is the +//! same overreach this section corrects two paragraphs above. +//! +//! This is why the "does not verify what it produces" guarantee above is worth +//! stating narrowly. The broad reading -- that the call touches no filesystem -- +//! is not merely unproven, it is false here. Earlier revisions said the +//! opposite, reasoning that the current directory lives in the PEB and the +//! `=X:` variables in the environment block and that both are ordinary process +//! memory. The reasoning was sound and the conclusion wrong, which is the +//! standing hazard this crate keeps meeting: a mechanism argued from the data +//! sources rather than measured. +//! +//! # What a resolution costs +//! +//! The figure the repo's own instrument produces is a **bound, not this call's +//! cost**, and the difference matters. On x86_64 `probe-request-cost` measures +//! building an open request as a construct-and-drop cycle at roughly 210 ns and +//! cloning an already-resolved path at roughly 45 ns. The ~165 ns between them +//! is what recycling a resolved path recovers, and that is all it is. +//! +//! **That probe exercises [`crate::path::prepare`], not this module**, and the +//! two have different allocation shapes -- which is itself why the gap cannot be +//! read as this call's cost. `prepare` copies the input and then allocates a +//! `MAX_PATH` output buffer, so two allocations against the clone's one, and the +//! builder chain sits on top. [`ResolveFullPath`] takes its input already owned +//! and allocates one buffer per attempt instead. Either way the allocator work +//! is the crate's, not `GetFullPathNameW`'s, and attributing the gap to the call +//! -- as a draft of this doc did -- credits it with the work the same sentence +//! is busy excluding. +//! +//! Timed on its own -- input already marshalled, output buffer pre-allocated, +//! so no allocation is in the loop -- the call costs about **110 ns** on this +//! host, roughly two thirds of that gap. That measurement is a direct one taken +//! for this note and is *not* something the probe reports; no instrument in +//! this repository isolates the call, and the honest reading of +//! `probe-request-cost` alone is an upper bound. +//! //! It does **not** solve the session-relative drive-letter hazard, and saying //! so plainly matters more than the part it does solve. `GetFullPathNameW` //! never expands a drive letter, and a drive letter is resolved against the @@ -103,7 +323,8 @@ impl From for FullPathError { /// use windows_namespace_request_sys::full_path::ResolveFullPath; /// use wtf_string::Wtf16String; /// -/// // Lexical: `.` and `..` are resolved without touching the filesystem. +/// // `.` and `..` are collapsed as string work, with no component verified: +/// // this holds whether or not `C:\Windows\System32` exists. /// let resolved = ResolveFullPath::new(Wtf16String::from(r"C:\Windows\System32\..\.\Temp")) /// .perform()? /// .to_string_lossy(); @@ -118,9 +339,9 @@ impl From for FullPathError { /// use windows_namespace_request_sys::full_path::ResolveFullPath; /// use wtf_string::Wtf16String; /// -/// // A path to nothing resolves perfectly happily, because the call is -/// // lexical. A consumer wanting a verified path wants an open plus -/// // GetFinalPathNameByHandleW instead. +/// // A path to nothing resolves perfectly happily, because the call +/// // does not verify it. A consumer wanting a verified path wants an open +/// // plus GetFinalPathNameByHandleW instead. /// let resolved = ResolveFullPath::new(Wtf16String::from(r"C:\no-such-directory\..\file.txt")) /// .perform()? /// .to_string_lossy(); diff --git a/crates/windows-namespace-request-sys/src/full_path/tests.rs b/crates/windows-namespace-request-sys/src/full_path/tests.rs index a8d734ea0..d938f4219 100644 --- a/crates/windows-namespace-request-sys/src/full_path/tests.rs +++ b/crates/windows-namespace-request-sys/src/full_path/tests.rs @@ -2,17 +2,21 @@ //! Tests for the `GetFullPathNameW` entry. //! -//! The negatives matter more than the positives here: this call is lexical, and -//! a suite that only ever resolved existing paths would leave a reader -//! believing it verifies something. +//! The negatives matter more than the positives here: this call does not verify +//! what it produces, and a suite that only ever resolved existing paths would +//! leave a reader believing it does. -use windows_sys::Win32::Foundation::ERROR_INSUFFICIENT_BUFFER; +use windows_sys::Win32::Foundation::{ + ERROR_ENVVAR_NOT_FOUND, ERROR_INSUFFICIENT_BUFFER, ERROR_SUCCESS, +}; use wtf_string::Wtf16String; use super::{FullPathError, ResolveFullPath}; use crate::handle::tests::handle_allocation; use crate::outcome::Win32Error; +mod drive_entry; + fn resolve(path: &str) -> String { ResolveFullPath::new(Wtf16String::from(path)) .perform() @@ -45,12 +49,31 @@ fn an_already_absolute_path_is_returned_unchanged() { #[test] fn a_path_that_does_not_exist_resolves_perfectly_happily() { - // The call is lexical and touches no filesystem. A consumer wanting a - // verified path wants an open plus GetFinalPathNameByHandleW. - assert_eq!( - resolve(r"C:\no-such-directory\..\nothing-here.txt"), - r"C:\nothing-here.txt" + // Resolution does not verify the result: no error, and no check that any + // component exists. A consumer wanting a verified path wants an open plus + // GetFinalPathNameByHandleW. + // + // The components are process-specific and their absence is asserted first. + // A hard-coded literal is only missing until some host happens to have it, + // and this test would then be demonstrating that an EXISTING path resolves + // -- which every other test here already covers. + // `Path::exists` goes through `CreateFileW` on Windows, so it allocates a + // handle and this suite serialises that -- see `ProbeDir::_allocating`. + // This test predates the fixture and was the one filesystem call in the + // module not covered by it. + let _allocating = handle_allocation() + .read() + .expect("the lock is not poisoned"); + + let missing = std::env::temp_dir().join(format!("wnrs-{}-absent", std::process::id())); + let missing = missing.to_str().expect("the temp path is UTF-8"); + assert!( + !std::path::Path::new(missing).exists(), + "precondition: {missing} must not exist" ); + + let doubled = format!(r"{missing}\also-absent\..\leaf.txt"); + assert_eq!(resolve(&doubled), format!(r"{missing}\leaf.txt")); } #[test] @@ -214,3 +237,343 @@ fn a_resolution_performs_the_same_way_on_another_thread() { assert_eq!(resolved, r"C:\Windows\System32"); } + +/// The current directory this process is running in, as `GetFullPathNameW` +/// would use it. +/// +/// Read through the same API rather than `std::env::current_dir`, because the +/// two can disagree: `std` normalizes, and what these tests need is exactly the +/// string the call under test roots against. +fn current_directory() -> String { + // A lone `.` is rooted at the current directory and then collapses to it. + resolve(".") +} + +#[test] +fn a_relative_path_is_rooted_at_the_process_current_directory() { + // The rooting half of the documented contract. Asserted as a RELATION to + // the current directory rather than against a literal, so it holds on any + // machine and in any working directory. + let expected = format!(r"{}\rel.txt", current_directory().trim_end_matches('\\')); + assert_eq!(resolve("rel.txt"), expected); +} + +#[test] +fn a_root_relative_path_takes_the_root_and_not_the_whole_directory() { + // `\foo` is documented as taking only the ROOT of the current directory, + // which is what distinguishes it from an ordinary relative path. Calling it + // "the current drive" was wrong -- under a UNC current directory there is + // no drive at all -- so this pins the property the doc actually claims. + let cwd = current_directory(); + let resolved = resolve(r"\foo"); + + // Computed, not inferred. An earlier version of this test derived the root + // by trimming the result it was checking, and guarded its one distinguishing + // assertion behind a length comparison -- so with the current directory at a + // drive root, where a root-relative path and a relative one coincide, the + // guard was false and the test passed having asserted almost nothing. A + // check whose strength depends on where it runs is the vacuous pass this + // repository keeps paying for. + let root = root_of(&cwd); + assert_eq!( + resolved, + format!(r"{root}foo"), + "a root-relative path is rooted at the root of the current directory \ + ({cwd}, root {root}), and carries none of its subtree" + ); +} + +/// The root of an absolute Windows path, including its trailing separator. +/// +/// C:\a\b gives C:\, and \\server\share\a gives \\server\share\ -- which +/// is why this returns a *root* rather than a drive: under a UNC current +/// directory there is no drive letter to return. +fn root_of(path: &str) -> String { + if let Some(rest) = path.strip_prefix(r"\\") { + // server\share, then everything after it. + let mut parts = rest.splitn(3, '\\'); + let server = parts.next().unwrap_or_default(); + let share = parts.next().unwrap_or_default(); + return format!(r"\\{server}\{share}\"); + } + // Stated rather than assumed: the only caller passes a resolved absolute + // path, which is always at least `C:\`, but `split_at` would panic on an + // index rather than say why. + assert!( + path.len() >= 2 && path.is_char_boundary(2), + "root_of expects an absolute path, got {path:?}" + ); + let (drive, _) = path.split_at(2); + format!(r"{drive}\") +} + +#[test] +fn a_legacy_device_name_short_circuits_rooting() { + // The exception to "roots a path that is not fully qualified", and the one + // a caller passing an untrusted name has to know about: these do not become + // files under the current directory. + // + // **Asserted as the complete mapping, not as "somewhere in the device + // namespace".** `starts_with(r"\\.\")`, which this used, would pass if + // `CON` resolved to `\\.\NUL` -- and the contract names which device each + // spelling reaches, so a test binding only to the prefix does not bind to + // the contract. Measured: the name is carried through unchanged. + for name in [ + "CON", "NUL", "PRN", "AUX", "CONIN$", "CONOUT$", "COM1", "LPT9", + ] { + assert_eq!( + resolve(name), + format!(r"\\.\{name}"), + "{name} reaches ITS device, not merely some device" + ); + } +} + +#[test] +fn the_device_form_accepts_trailing_colons_dots_spaces_and_any_casing() { + // Every spelling the module doc claims reaches a device. A filter written + // from a narrower reading of the rule would let these through. + // + // The full result again rather than the prefix, which also pins what the + // device form does to the spelling: trailing colons, dots and spaces are + // dropped, and case is carried through untouched. + for (spelling, expected) in [ + ("CON", r"\\.\CON"), + ("CON:", r"\\.\CON"), + ("CON::", r"\\.\CON"), + ("CON.", r"\\.\CON"), + ("CON ", r"\\.\CON"), + ("con", r"\\.\con"), + ("cOn:", r"\\.\cOn"), + ] { + assert_eq!( + resolve(spelling), + expected, + "{spelling:?} is a device spelling, and reaches this exact device" + ); + } +} + +#[test] +fn superscript_digits_are_device_names_too() { + // The members a hand-written denylist omits, and which this crate's own + // documentation asserted did not exist until a review measured them. If a + // future Windows build stops accepting them this test says so, which is the + // whole reason it is here rather than left as prose. + for spelling in [ + "COM\u{00b9}", + "COM\u{00b2}", + "COM\u{00b3}", + "LPT\u{00b9}", + "LPT\u{00b2}", + "LPT\u{00b3}", + ] { + let resolved = resolve(spelling); + assert!( + resolved.starts_with(r"\\.\"), + "{spelling:?} uses a superscript digit and still names a device: {resolved}" + ); + } +} + +#[test] +fn a_device_name_with_anything_around_it_is_rooted_normally() { + // The other half, and the one that keeps the rule from being read as "any + // input containing a device name". Without these the test above would pass + // just as well against an implementation that mapped far too much. + for spelling in [ + "CON.txt", r"a\CON", r".\CON", "CON:x", "COM0", "COM10", "CONIN", + ] { + let resolved = resolve(spelling); + assert!( + !resolved.starts_with(r"\\.\"), + "{spelling:?} is not a bare device name, so it must be rooted: {resolved}" + ); + } +} + +#[test] +fn a_fully_qualified_path_is_unaffected_by_the_current_directory() { + // The lexical half, stated as the invariance the rooting half lacks: this + // is what makes "not lexical as a whole" a claim about the OTHER half only. + // `C:\a` need not exist, which is the same fact the existence test pins. + assert_eq!(resolve(r"C:\a\..\b"), r"C:\b"); + assert_eq!(resolve("C:/a/b//c"), r"C:\a\b\c"); +} + +#[test] +fn trailing_dot_and_space_trimming_differs_between_final_and_intermediate_components() { + // The module doc says the rewrite trims trailing dots and spaces. Until now + // that was only exercised through a final `.` component (which is the + // separate `.`-collapsing rule) and through device spellings (which take + // the short-circuit and never reach the ordinary path). Neither pins this. + // + // **An earlier version of this test generalised the rule, and the + // generalisation is false.** It carried one intermediate case, `C:\a.\b` -> + // `C:\a\b`, under the comment "trimming applies per component, not only at + // the end of the path". Measured, a component's position decides what it + // loses: + // + // final: C:\name... -> C:\name (a RUN of dots goes) + // C:\name -> C:\name (trailing spaces go) + // intermediate: C:\a.\b -> C:\a\b (ONE trailing dot goes) + // C:\a.b.\c -> C:\a.b\c (inner dots are not special) + // C:\a...\b -> unchanged (a run does NOT go) + // C:\a \b -> unchanged (a space does NOT go) + // C:\a. \b -> unchanged + // + // **The preservation cases are the load-bearing half**, and their absence + // is what let the wrong generalisation stand: with only the transforming + // inputs, an implementation trimming every component's trailing dots and + // spaces satisfies the entire test while being wrong about three of the + // five intermediate spellings. A reviewer demonstrated exactly that against + // the previous seven assertions. + // + // Every case measured before being written down -- which was also claimed + // last time, and was true of the cases present. What was not measured was + // the sentence generalising them. + for (input, expected) in [ + // The final component loses any run of trailing dots and spaces. + (r"C:\name.", r"C:\name"), + (r"C:\name ", r"C:\name"), + (r"C:\name...", r"C:\name"), + (r"C:\name ", r"C:\name"), + (r"C:\name. ", r"C:\name"), + // An extension is not special: the trailing dot goes, the rest stays. + (r"C:\name.txt.", r"C:\name.txt"), + // An intermediate component loses a single trailing dot ... + (r"C:\a.\b", r"C:\a\b"), + (r"C:\a.b.\c", r"C:\a.b\c"), + // ... and nothing else. These are the cases that fail the broad rule. + (r"C:\a...\b", r"C:\a...\b"), + (r"C:\a \b", r"C:\a \b"), + (r"C:\a. \b", r"C:\a. \b"), + (r"C:\a b \c", r"C:\a b \c"), + ] { + assert_eq!(resolve(input), expected, "trimming {input:?}"); + } +} + +#[test] +fn a_name_containing_a_device_word_is_rooted_under_the_current_directory() { + // Strengthens the device-negative control. Asserting only "not `\\.\`" is + // too weak: an implementation that returned every relative input unchanged + // would satisfy it while rooting nothing. These assert the full resolved + // path, so the rooting guarantee is actually covered. + // + let base = current_directory(); + let base = base.trim_end_matches('\\'); + // `CON:x` is here because it was NOT, and the negative test alone cannot + // carry it: that test asserts only `!starts_with("\\\\.\\")`, which the + // unrooted literal `CON:x` satisfies. So the one spelling whose rooting is + // least obvious -- a device word followed by a colon, where `CON:` itself + // IS a device -- was the one nothing pinned. Measured: `\CON:x`, with + // the colon carried through untouched. + for name in ["CON.txt", "CONIN", "COM0", "COM10", r"a\CON", "CON:x"] { + assert_eq!( + resolve(name), + format!(r"{base}\{name}"), + "{name:?} is not a bare device name, so it roots under the current \ + directory rather than merely avoiding the device namespace" + ); + } + + // `.\CON` is asserted separately rather than excluded from the loop, which + // is what an earlier version did on the grounds that the `.` collapses and + // the expected form would need a special case. It needs one, so it gets + // one: with the case left to the device-NEGATIVE test alone, the only claim + // made about `.\CON` was that it does not start with `\\.\` -- and an + // implementation returning it unchanged satisfies that, which is precisely + // the too-weak assertion this test exists to strengthen. + assert_eq!( + resolve(r".\CON"), + format!(r"{base}\CON"), + "the `.` collapses and the result roots under the current directory, so \ + a leading `.\\` is enough to take the name out of the device \ + short-circuit without taking it out of ordinary rooting" + ); + + // The root-relative form, which the module doc states and nothing pinned. + // Its expectation is the current directory's ROOT rather than `base`, so it + // cannot join the loop above. + let root = root_of(¤t_directory()); + assert_eq!( + resolve(r"\CON"), + format!("{root}CON"), + "a leading separator roots the device word at the current directory's \ + root instead of reaching the device" + ); +} + +#[test] +fn nul_is_the_one_device_word_a_path_around_it_does_not_save() { + // **The exception to the test above, and it was found by trying to write + // the general rule.** A review asked for the `\CON` case on the grounds + // that the doc states it; the doc stated it of the whole device SET, having + // been written from `CON` alone. Measured across all eight accepted names, + // seven root normally once anything precedes them and `NUL` does not -- + // `NUL` short-circuits as the final component of any path at all. + // + // Had the requested assertion been written as the general rule it was + // phrased as ("a root-relative device word roots at the root"), it would + // have pinned a false claim, which is the failure this branch exists to + // stop rather than repeat. + let base = current_directory(); + let base = base.trim_end_matches('\\'); + let root = root_of(¤t_directory()); + + // The seven that behave as the doc says, in the form that separates them. + for name in ["CON", "PRN", "AUX", "CONIN$", "CONOUT$", "COM1", "LPT1"] { + assert_eq!( + resolve(&format!(r".\{name}")), + format!(r"{base}\{name}"), + "{name:?} stops being a device once a path precedes it" + ); + assert_eq!( + resolve(&format!(r"C:\{name}")), + format!(r"C:\{name}"), + "{name:?} is an ordinary component of a fully-qualified path" + ); + } + + // And `NUL`, which does not -- INCLUDING from a fully-qualified path, so a + // rooted result is not by itself evidence that a path names a file. + for input in [r"\NUL", r".\NUL", r"a\NUL", r"C:\NUL"] { + assert_eq!( + resolve(input), + r"\\.\NUL", + "{input:?} still reaches the device: NUL is not saved by a path in \ + front of it, where every other device word is" + ); + } + + // A suffix is what takes it out, which is the boundary of the exception -- + // and the doc names both spellings, so both are asserted. The root-relative + // form alone would leave the bare ones uncovered, which is how `CON:x` came + // to be pinned by a predicate the wrong answer satisfied. + assert_eq!( + resolve(r"\NUL.txt"), + format!("{root}NUL.txt"), + "an extension takes even NUL out of the device namespace" + ); + for (input, expected) in [("NUL.txt", "NUL.txt"), ("NUL:x", "NUL:x")] { + assert_eq!( + resolve(input), + format!(r"{base}\{expected}"), + "{input:?} is a suffixed name, so it roots under the current \ + directory like any other" + ); + } + + // And the boundary is a suffix, not merely "more characters": a trailing + // colon or space still reaches the device, exactly as for `CON`. Without + // these the rule above would read as "anything after NUL saves it". + for input in ["NUL::", "NUL "] { + assert_eq!( + resolve(input), + r"\\.\NUL", + "{input:?} is still the device: trailing colons and spaces are part \ + of the device form, not a suffix that escapes it" + ); + } +} diff --git a/crates/windows-namespace-request-sys/src/full_path/tests/drive_entry.rs b/crates/windows-namespace-request-sys/src/full_path/tests/drive_entry.rs new file mode 100644 index 000000000..91fd779f8 --- /dev/null +++ b/crates/windows-namespace-request-sys/src/full_path/tests/drive_entry.rs @@ -0,0 +1,897 @@ +// Copyright (c) Mike Grier. +// Split from tests.rs at 3e48630. + +//! The per-drive current-directory (`=X:`) arm, and the fixture that controls +//! it. +//! +//! Separated from the parent because it is the one cluster here with a fixture +//! of its own: a probe directory, a drive-letter allocator, a reader and writer +//! for the hidden entries, and a guard that puts a borrowed one back. Every +//! test below either sets that state or is about what the call does with it, +//! and nothing outside this module needs any of it. +//! +//! The two drive-relative ROOTING tests moved with the fixture rather than +//! staying beside the other rooting forms. That is a privacy constraint, not a +//! preference: a child module can see its ancestors' private items, but a +//! parent cannot see its child's, so a test left in the parent could not reach +//! the helpers here. They are also the two tests that are about this arm, so +//! the constraint and the responsibility agree. + +use super::*; + +#[test] +fn a_drive_relative_path_carries_its_component_and_the_current_drive_uses_the_process_directory() { + // **The name says what the two assertions reach, and an earlier one did + // not.** This was + // `a_drive_relative_path_is_rooted_at_that_drive_and_not_the_process_directory`, + // which claims more than anything here shows, in two separate ways. The + // other-drive assertion is `ends_with("\\foo")`, which accepts ANY base + // including the process directory -- so it cannot say "not the process + // directory". And "rooted at that drive" is not even true in general: an + // accepted entry is used verbatim and may name a directory on a different + // drive entirely, which is the sibling test's whole point. + // + // The body already said it only bounds the arm. The name did not, and the + // name is what a reader takes away -- the same defect as the test called + // `..._is_neither_consulted_nor_rewritten` before it was renamed. + // + // The third rooting form, with two arms, which is the part that gets + // missed: + // + // * For a drive OTHER than the current one, Windows reads the hidden + // `=X:` entry recorded for it. + // * For the CURRENT drive the entry makes no difference to the result and + // the process current directory wins. Measured: setting `=Q:` while the + // process is on `Q:` changes nothing. + // + // **This test does not mutate `=X:`, but the call it exercises may.** + // Measured: resolving `X:foo` for a non-current drive checks that drive's + // entry and WRITES it to `X:\` when the entry is absent or rejected. An + // accepted entry is left alone, and the current-drive form writes nothing + // -- so this is not "every resolution", but it does mean an ordinary host + // with no entry has one written merely by running this test. That is + // a property of the call, documented in the module doc; it is noted here so + // the next reader does not take "reads process state" at face value, as + // four revisions of that doc did. + // + // **This test only BOUNDS the other-drive arm**, because it does not control + // the entry: with no `=X:` set, an implementation that always used the + // drive root would satisfy everything here. The arm is pinned properly by + // `a_drive_relative_path_uses_that_drives_entry_verbatim_and_rewrites_a_bad_one`, + // which sets the entry and draws its letter from a candidate list disjoint + // from this one's, so the two cannot race. + let cwd = current_directory(); + let cwd_drive = cwd.chars().next().filter(char::is_ascii_alphabetic); + + // The other-drive arm needs no drive letter from the current directory -- + // under a UNC current directory every letter is "other" -- so it runs + // unconditionally and this test never degenerates to a silent skip. + let other = probe_drive_from(probe_drives::ROOTED_AT_THAT_DRIVE, None); + + // This test controls no entry, but the CALL does: resolving for a + // non-current drive writes `=X:` whenever the recorded entry is absent or + // rejected, and on most hosts a letter chosen for being unused has no entry + // at all. So merely observing the arm mutates process-global state, and + // this was the one mutating case here without a guard -- the borrow is + // needed exactly because the mutation is not the test's own doing. + let _restore = BorrowedDriveEntry::take(other); + let resolved = resolve(&format!("{other}:foo")); + + // Only what is invariant without controlling the entry. An earlier version + // required the result to start with `X:\`, which the verbatim rule breaks; + // its replacement compared against the current directory, which `other` + // differs from by construction, so it could fire only if the entry happened + // to equal the process directory exactly -- the same vacuity, respelled. + // What survives every entry value is that the component is carried through. + assert!( + resolved.ends_with(r"\foo"), + "the component is carried through whatever the entry holds: {resolved}" + ); + + // The current-drive arm, where the process directory wins over any `=X:`. + // Only expressible when the current directory has a drive letter at all. + if let Some(drive) = cwd_drive { + assert_eq!( + resolve(&format!("{drive}:foo")), + format!(r"{}\foo", cwd.trim_end_matches('\\')), + "on the current drive, the per-drive entry makes no difference to \ + the result and the process directory is used" + ); + } +} + +#[test] +fn the_current_drives_entry_does_not_affect_resolution_and_is_not_rewritten() { + // The module doc states both halves of the current-drive arm as fact. Until + // now nothing pinned either, and the assertion just above -- which looks + // like it does -- cannot: it resolves `X:foo` WITHOUT controlling the entry + // and compares against the process directory, and Windows keeps the current + // drive's entry equal to that directory. So it reads the same either way. + // Vacuous in precisely the way this crate keeps rediscovering, and the + // reason the two arms need opposite fixtures: the sibling tests must AVOID + // the current drive, and this one must be on it. + // + // **The name says what is observable, and an earlier one did not.** This + // was `..._is_neither_consulted_nor_rewritten`, which claims the entry is + // not READ -- and installing a value and watching the outcome cannot + // separate "not read" from "read and ignored". That is the same overreach + // this branch removed from the probe's "without consulting a device", and + // the test correcting it committed it in its own name. What the two + // assertions below reach is the pair of observable effects: the entry makes + // no difference to the result, and it is not written back. + let cwd = current_directory(); + let Some(drive) = cwd.chars().next().filter(char::is_ascii_alphabetic) else { + // A UNC current directory has no drive letter, so there is no + // current-drive arm to exercise. Not a skip of something testable. + return; + }; + let process_directory = format!(r"{}\foo", cwd.trim_end_matches('\\')); + + let probe_dir = probe_directory("current-drive"); + let probe = probe_dir.path.to_str().expect("the probe path is UTF-8"); + let _restore = BorrowedDriveEntry::take(drive); + + // The anti-vacuity check, made permanent rather than performed once by + // hand: unless the two arms would give DIFFERENT answers, every assertion + // below passes without distinguishing them, which is the failure this test + // was written to correct. + assert_ne!( + process_directory, + format!(r"{probe}\foo"), + "precondition: the entry must name somewhere other than the process \ + directory, or honouring it and ignoring it look identical" + ); + + // No difference to the result. The entry is one the OTHER arm would honour + // verbatim -- an existing directory in canonical form -- and it names + // somewhere the process directory cannot be, because this test just created + // it under a process-unique name. If the entry were HONOURED, the result + // would be under `probe`. + set_drive_entry(drive, Some(probe)); + assert_eq!( + resolve(&format!("{drive}:foo")), + process_directory, + "the current drive's entry was set to {probe}, an entry the non-current \ + arm honours verbatim, and the process directory won anyway" + ); + + // Not rewritten, which needs a REJECTED entry to be visible: an accepted one + // is left alone on both arms, so leaving it alone shows nothing. A child of + // the probe directory cannot exist, and on the non-current arm that is + // replaced by the drive root. + let missing = probe_dir.path.join("no-such-child"); + let missing = missing.to_str().expect("the probe path is UTF-8"); + set_drive_entry(drive, Some(missing)); + let _ = resolve(&format!("{drive}:foo")); + assert_eq!( + drive_entry(drive).map(|v| v.to_string_lossy()).as_deref(), + Some(missing), + "an entry the non-current arm would have replaced with {drive}:\\ is \ + left untouched on the current drive" + ); +} + +/// A directory that exists, is in canonical `X:\...` form, and is neither a +/// drive root nor the process current directory. +/// +/// **The form is enforced, not assumed, and that distinction has now cost two +/// rounds.** An earlier version derived the probe from `current_directory()`, +/// which yields `C:` at a drive root -- drive-relative, not a directory -- and +/// both tests failed there. Its replacement used `std::env::temp_dir()`, which +/// is `%TMP%`/`%TEMP%` verbatim and carries no guarantee of a drive letter: with +/// temp redirected to a share, the probe is a UNC path, which +/// `GetFullPathNameW` rejects as an entry *on shape* -- the very rule the +/// caller is trying to pin. Folder redirection makes that an ordinary +/// configuration, not a contrived one. +/// +/// So the temp directory is used only when it is drive-rooted, and otherwise +/// the fallback is `%SystemRoot%`, which is guaranteed to exist, to be +/// canonical, and not to be a drive root. Nothing is created in the fallback +/// case, so `created` records whether there is anything to remove. +/// +/// Removal is a [`Drop`], matching this crate's own `Fixture` in +/// [`crate::handle`]'s tests. An earlier version cleaned up with a statement at +/// the end of each test and argued that a guard writing during unwinding could +/// panic and abort -- which is true of the `=X:` entry restore beside it, and +/// not of removing a directory. Conflating the two left a directory behind +/// after every failing assertion. +struct ProbeDir { + path: std::path::PathBuf, + created: bool, + + /// Held for the fixture's whole lifetime, because this fixture opens + /// handles and this suite serialises that. + /// + /// `handle_allocation()`'s read guard means "I may open handles"; its write + /// guard means "no other test may, while I reason about a specific handle + /// value". Eleven tests across four modules take the write guard to assert + /// things like a closed handle's value not being reused, and any test + /// allocating a handle beside them can make those assertions fail for a + /// reason that has nothing to do with what they pin. + /// + /// This fixture allocates: `create_dir` and `remove_dir` here, and the + /// `Path::exists` / `is_file` preconditions its tests run while it is + /// alive -- `std`'s Windows metadata goes through `CreateFileW`. Holding + /// the guard on the fixture covers all of them for the whole test, which a + /// guard taken at each call site would not. + /// + /// A test holding one of these must not take a second read guard: `std` + /// does not promise recursive read locking is deadlock-free. + _allocating: std::sync::RwLockReadGuard<'static, ()>, +} + +impl ProbeDir { + /// The drive letter this probe lives on, if it has one. + /// + /// The caller needs it to pick a probe drive that is *not* this one: + /// resolving `W:foo` against an entry naming a directory that is itself on + /// `W:` cannot demonstrate that an accepted entry is used verbatim across + /// drives, which is the property being pinned. + fn drive(&self) -> Option { + self.path + .as_os_str() + .to_string_lossy() + .chars() + .next() + .filter(char::is_ascii_alphabetic) + } +} + +impl Drop for ProbeDir { + fn drop(&mut self) { + if self.created { + let _ = std::fs::remove_dir(&self.path); + } + } +} + +fn probe_directory(tag: &str) -> ProbeDir { + // Taken before the first filesystem call, and handed to the fixture so it + // outlives this function. See `ProbeDir::_allocating`. + let _allocating = handle_allocation() + .read() + .expect("the lock is not poisoned"); + + // The full shape an accepted `=X:` entry must have, not just its first + // three characters: rooted at `X:\`, with no `.` or `..` component and no + // forward slash. `a_rejected_drive_entry_is_replaced_by_the_drive_root` + // shows each of those spellings is rejected while naming the same existing + // directory, so a base carrying one would turn that test's CONTROL + // assertion -- "the same directory in canonical form is accepted" -- into a + // rejection, and it would fail for a reason unrelated to what it pins. + // + // **A review read that as reachable through a non-canonical `%TMP%`. It is + // not, and the measurement is here so the next reader need not repeat it.** + // `std::env::temp_dir` goes through `GetTempPath2W`, which normalises what + // it finds: `C:/Users/.../Temp`, `...\Temp\.`, `...\Temp\..\Temp`, + // `...\Temp\\` and even the drive-relative `C:Users\...` all came back as + // `C:\Users\...\Temp\`. The one spelling passed through verbatim is + // `\\?\C:\...`, which is not drive-rooted and so takes the fallback below. + // + // The check is widened anyway. It costs nothing, it covers the fallback + // base too, and it is the difference between a precondition that is + // enforced and one that is argued -- which is the distinction this whole + // change exists to hold. + let canonical_drive_rooted = |p: &std::path::Path| { + let s = p.as_os_str().to_string_lossy().into_owned(); + let mut chars = s.chars(); + matches!( + (chars.next(), chars.next(), chars.next()), + (Some(d), Some(':'), Some('\\')) if d.is_ascii_alphabetic() + ) && !s.contains('/') + && !s.split('\\').any(|c| c == "." || c == "..") + }; + + let temp = std::env::temp_dir(); + if canonical_drive_rooted(&temp) { + let path = temp.join(format!("wnrs-{}-{tag}", std::process::id())); + + // `create_dir`, not `create_dir_all`, and the difference is ownership + // rather than parents. `create_dir_all` SUCCEEDS on a directory that + // already exists, so setting `created` after it recorded a claim this + // fixture had not established -- and `Drop` then removes the path on + // the strength of that claim. The name is `%TEMP%\wnrs--`, + // which an interrupted earlier run leaves behind and which Windows can + // hand back to a later process when it reuses the PID. The blast radius + // is small, because `remove_dir` refuses a non-empty directory -- but + // "small" is not the point. Deleting something on an ownership claim + // nothing checked is the same defect as asserting a mechanism nothing + // measured, and this file exists to stop doing that. + // + // An existing directory is still perfectly usable as a probe; it is + // just not ours to remove. + let created = match std::fs::create_dir(&path) { + Ok(()) => true, + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => false, + Err(e) => panic!("create the probe directory {}: {e}", path.display()), + }; + + return ProbeDir { + path, + created, + _allocating, + }; + } + + // Not creating anything here, so no write permission is needed on a host + // whose temp directory is redirected off a drive letter. + // + // **It must also differ from the process current directory**, which the + // temp branch gets for free -- it creates a uniquely named child -- and this + // branch does not. Cargo launched from `%SystemRoot%` on a host with a UNC + // temp directory would otherwise hand back the current directory itself, + // and a probe indistinguishable from the current directory cannot separate + // "the entry was honoured" from "the entry was ignored". `System32` is the + // second candidate for the same reason `probe_drive_from` takes a list: + // one value that is usually right is not a guarantee. + let system_root = std::path::PathBuf::from( + std::env::var("SystemRoot").expect("SystemRoot is set on Windows"), + ); + let cwd = current_directory(); + let cwd = cwd.trim_end_matches('\\'); + let distinct = |p: &std::path::Path| { + !p.as_os_str() + .to_string_lossy() + .trim_end_matches('\\') + .eq_ignore_ascii_case(cwd) + }; + + let path = [system_root.clone(), system_root.join("System32")] + .into_iter() + .find(|p| canonical_drive_rooted(p) && distinct(p)) + .unwrap_or_else(|| { + panic!( + "no fallback probe directory is both canonical and distinct \ + from the current directory {cwd}" + ) + }); + + ProbeDir { + path, + created: false, + _allocating, + } +} +/// The candidate drive letters, one list per test that mutates a `=X:` entry. +/// +/// **Centralised so the properties these tests depend on are CHECKED rather +/// than restated.** Both were previously prose -- a doc comment saying the +/// lists are disjoint, and an archived note enumerating them -- and prose +/// drifted: the archive named five lists after the sixth had been added, so a +/// reader picking letters for a seventh would have consulted an inventory +/// missing three of the eighteen letters already in use. Nothing checked +/// either claim, because nothing could: the lists were literals at six call +/// sites with no table to read. +/// +/// [`the_probe_drive_candidate_lists_are_disjoint_and_large_enough`] now reads +/// this table, so adding a list that collides -- or one too short for +/// [`probe_drive_from`]'s guarantee -- fails a test instead of a review. +mod probe_drives { + pub const ROOTED_AT_THAT_DRIVE: &[char] = &['X', 'Y', 'P']; + pub const VERBATIM_ENTRY: &[char] = &['W', 'U', 'N']; + pub const REJECTED_ENTRY: &[char] = &['V', 'T', 'M']; + pub const LONG_ENTRY: &[char] = &['R', 'S', 'K']; + pub const BORROW_GUARD: &[char] = &['G', 'H', 'J']; + pub const EMPTY_VS_ABSENT: &[char] = &['E', 'F', 'B']; + + /// Every list above. A new list that is not added here is not covered by + /// the disjointness test, so keep them together. + pub const ALL: &[(&str, &[char])] = &[ + ("ROOTED_AT_THAT_DRIVE", ROOTED_AT_THAT_DRIVE), + ("VERBATIM_ENTRY", VERBATIM_ENTRY), + ("REJECTED_ENTRY", REJECTED_ENTRY), + ("LONG_ENTRY", LONG_ENTRY), + ("BORROW_GUARD", BORROW_GUARD), + ("EMPTY_VS_ABSENT", EMPTY_VS_ABSENT), + ]; +} + +#[test] +fn the_probe_drive_candidate_lists_are_disjoint_and_large_enough() { + for (name, list) in probe_drives::ALL { + // `probe_drive_from` excludes at most two letters -- the current drive + // and the probe directory's drive -- so three candidates guarantee a + // survivor. This is the premise of the panic in that function, checked + // here rather than left to the caller as the doc comment used to. + assert!( + list.len() >= 3, + "{name} has {} candidates, and at most two can be excluded, so \ + fewer than three cannot guarantee a survivor", + list.len() + ); + + let mut seen = list.to_vec(); + seen.sort_unstable(); + seen.dedup(); + assert_eq!(seen.len(), list.len(), "{name} repeats a letter"); + } + + for (a_name, a) in probe_drives::ALL { + for (b_name, b) in probe_drives::ALL { + if a_name == b_name { + continue; + } + let shared: Vec = a + .iter() + .copied() + .filter(|c| b.iter().any(|d| d.eq_ignore_ascii_case(c))) + .collect(); + assert!( + shared.is_empty(), + "{a_name} and {b_name} share {shared:?}, so the two tests can \ + select the same drive and race under libtest's \ + thread-per-test model" + ); + } + } +} + +/// A drive letter to probe with, drawn from `candidates` and guaranteed to be +/// neither the current drive nor `avoid`. +/// +/// **Every candidate is checked, which an earlier version did not do.** It took +/// a preferred letter and a fallback, tested only the preferred one, and +/// returned the fallback unvalidated -- so when the preferred letter was +/// excluded the caller could still be handed the current drive. Measured: with +/// the process on `U:` and `%TEMP%` on a `subst`-ed `W:`, the verbatim test +/// selected `U` and then asserted the *other-drive* contract while exercising +/// the *current-drive* arm, which is the one case where the entry makes no +/// difference to the result. +/// It failed, but the mode is worse than a failure: the helper's own doc +/// promised a guarantee it never enforced. +/// +/// Three candidates against at most two exclusions, so one always survives. The +/// panic remains because that argument is about the caller's list, which this +/// function cannot see -- but the argument is no longer only an argument: +/// [`the_probe_drive_candidate_lists_are_disjoint_and_large_enough`] checks it +/// against every list in [`probe_drives`]. +/// +/// Callers pass disjoint lists, so no two tests can select the same letter and +/// race under libtest's thread-per-test model. +fn probe_drive_from(candidates: &[char], avoid: Option) -> char { + let cwd = current_directory(); + // A UNC current directory has no drive letter, so nothing collides there. + let current = cwd.chars().next().filter(char::is_ascii_alphabetic); + let taken = |c: char| { + current.is_some_and(|d| d.eq_ignore_ascii_case(&c)) + || avoid.is_some_and(|d| d.eq_ignore_ascii_case(&c)) + }; + + *candidates.iter().find(|&&c| !taken(c)).unwrap_or_else(|| { + panic!( + "every candidate of {candidates:?} is excluded by the current \ + drive ({current:?}) or the probe drive ({avoid:?})" + ) + }) +} +/// Reads one of the hidden `=X:` per-drive current-directory entries. +/// +/// Through Win32 rather than `std::env`, which rejects a key containing `=` +/// outright and so cannot address these at all. +fn drive_entry(drive: char) -> Option { + let name = Wtf16String::from(format!("={drive}:").as_str()); + // Start small and grow to whatever Windows asks for. The API's two return + // conventions differ: on success it reports the units written EXCLUDING the + // terminator, and on an undersized buffer it reports the capacity REQUIRED + // INCLUDING it. Treating the second as the first indexes past the buffer and + // panics -- while trying to preserve a legitimate long entry, so the failure + // would land before the test could restore the process state it borrowed. + let mut buffer = vec![0u16; 256]; + loop { + // Zero is TWO different answers, and the last error is the only thing + // that separates them -- so it is cleared first, because the value left + // by some earlier call would otherwise be read as this one's. + // + // SAFETY: no preconditions. + unsafe { windows_sys::Win32::Foundation::SetLastError(ERROR_SUCCESS) }; + + // SAFETY: the name is NUL-terminated and the buffer is writable for + // the length passed. + let written = unsafe { + windows_sys::Win32::System::Environment::GetEnvironmentVariableW( + name.as_terminated_ptr(), + buffer.as_mut_ptr(), + u32::try_from(buffer.len()).unwrap_or(u32::MAX), + ) + }; + let written = written as usize; + if written == 0 { + // **An earlier version of this comment claimed, as measured, that + // an empty value and an absent name are the same state and cannot + // be told apart. That was wrong, and wrong in this crate's + // signature way: the measurement behind it never cleared the last + // error, so it could only ever have seen whatever was already + // there.** Cleared first and re-measured, the two are distinct, for + // an ordinary name and an `=X:` name alike: + // + // set to "" -> returns 0, last error ERROR_SUCCESS + // deleted -> returns 0, last error ERROR_ENVVAR_NOT_FOUND + // + // The difference is not academic here. Collapsing both to `None` + // makes the restoration in `BorrowedDriveEntry` DELETE an inherited + // empty entry rather than put it back -- losing exactly the process + // state the guard exists to preserve. + // + // SAFETY: no preconditions. + let last = unsafe { windows_sys::Win32::Foundation::GetLastError() }; + return match last { + ERROR_SUCCESS => Some(Wtf16String::from_units(&[])), + ERROR_ENVVAR_NOT_FOUND => None, + // Anything else is neither answer, and folding it into "absent" + // would make the guard delete an entry over a transient error. + other => panic!("reading ={drive}: failed with error {other}"), + }; + } + if written < buffer.len() { + // Kept as WTF-16 units rather than going through String: a lossy + // conversion would replace an unpaired surrogate, so restoring the + // entry afterwards would write back something the process did not + // start with. + return Some(Wtf16String::from_units(&buffer[..written])); + } + buffer = vec![0u16; written]; + } +} +/// Sets or clears one of the hidden `=X:` entries. +fn set_drive_entry(drive: char, value: Option<&str>) { + set_drive_entry_units(drive, value.map(Wtf16String::from).as_ref()); +} + +/// [`set_drive_entry`], taking the exact units a [`drive_entry`] read returned. +/// +/// Restoration goes through this rather than through `&str`, so an entry +/// containing an unpaired surrogate is put back byte for byte. +fn set_drive_entry_units(drive: char, value: Option<&Wtf16String>) { + assert!( + try_set_drive_entry_units(drive, value), + "set ={drive}: entry" + ); +} + +/// [`set_drive_entry_units`] without the assertion, reporting success instead. +/// +/// Separate because the restoration in [`BorrowedDriveEntry`] runs during +/// unwinding, where a panic would abort the process and destroy the report of +/// the failure that started the unwind. +fn try_set_drive_entry_units(drive: char, value: Option<&Wtf16String>) -> bool { + let name = Wtf16String::from(format!("={drive}:").as_str()); + let value_ptr = value + .as_ref() + .map_or(core::ptr::null(), |v| v.as_terminated_ptr()); + // SAFETY: both pointers are NUL-terminated; a null value clears the entry. + let ok = unsafe { + windows_sys::Win32::System::Environment::SetEnvironmentVariableW( + name.as_terminated_ptr(), + value_ptr, + ) + }; + ok != 0 +} + +/// Borrows one drive's `=X:` entry and puts it back when the test ends, +/// **whether or not the test panicked**. +/// +/// Restoring on the last line of the test is not enough, and the hazard is not +/// theoretical: `=X:` is process-global, `cargo test` runs tests as threads in +/// ONE process, and every assertion between the save and the restore is a place +/// the entry can be abandoned. What a sibling test would then inherit is not +/// merely a stale value but one no host would produce -- a path to a directory +/// that no longer exists once the probe directory is removed, or the 1200-unit +/// value that `a_long_drive_entry_round_trips_through_the_reader` installs on +/// purpose. The reader above already names this ("the entry is then never +/// restored") without defending against it; this is the defence. +/// +/// A failed restore is dropped rather than asserted, for the reason given on +/// [`try_set_drive_entry_units`]. +struct BorrowedDriveEntry { + drive: char, + saved: Option, +} + +impl BorrowedDriveEntry { + fn take(drive: char) -> Self { + Self { + drive, + saved: drive_entry(drive), + } + } +} + +impl Drop for BorrowedDriveEntry { + fn drop(&mut self) { + let restored = try_set_drive_entry_units(self.drive, self.saved.as_ref()); + + // Silence is bought only where it buys something. While unwinding, a + // panic here aborts the process and destroys the report of the failure + // that started the unwind, so a failed restore is worth less than the + // diagnosis it would replace. On the ordinary path there is no such + // trade: staying quiet would let the suite carry on with corrupted + // process-global state and fail somewhere unrelated, which is the + // hardest kind of failure to read. + assert!( + restored || std::thread::panicking(), + "restoring ={}: failed, leaving process-global state corrupted for \ + every test that follows", + self.drive + ); + } +} + +#[test] +fn a_drive_relative_path_uses_that_drives_entry_verbatim_and_rewrites_a_bad_one() { + // The arm the sibling test can only BOUND. Without controlling the entry, + // an implementation that always used the drive root would satisfy every + // assertion there, because a host with no `=X:` entry cannot tell the two + // rules apart. + // + // **Controlling the entry is not the hazard it looks like**, and that is + // what unblocked this test. The objection was that `=X:` is process-global + // while these tests share a process. But the call under test writes that + // entry itself whenever it is absent or rejected, so this state is already + // mutated by the code being exercised. What keeps the tests from + // interfering is not that -- it is that each takes a drive letter no other + // one can choose. + // + // The probe directory is created FIRST so the letter can avoid its drive as + // well as the current one: an entry naming a directory on the same drive it + // is recorded for would be honoured, the test would pass, and the + // cross-drive property below would go unexercised. + let probe_dir = probe_directory("verbatim"); + let probe = probe_dir.path.to_str().expect("the probe path is UTF-8"); + let drive = probe_drive_from(probe_drives::VERBATIM_ENTRY, probe_dir.drive()); + let _restore = BorrowedDriveEntry::take(drive); + + assert_ne!( + Some(drive.to_ascii_uppercase()), + probe_dir.drive().map(|d| d.to_ascii_uppercase()), + "the probe directory must be on a different drive, or the assertion \ + below cannot show the entry is honoured ACROSS drives" + ); + + set_drive_entry(drive, Some(probe)); + assert_eq!( + resolve(&format!("{drive}:foo")), + format!(r"{probe}\foo"), + "an entry naming an existing directory is honoured verbatim, even onto \ + a different drive -- so \"that drive's own current directory\" is the \ + convention the entry usually holds, not a guarantee about the result" + ); + + // Verbatim means verbatim, including the join. The module doc records that + // an accepted entry ending in a separator yields a DOUBLED one, and nothing + // pinned it -- so the observation could have stopped being true without CI + // noticing, which is the drift this change exists to close rather than + // commit again. + set_drive_entry(drive, Some(&format!(r"{probe}\"))); + assert_eq!( + resolve(&format!("{drive}:foo")), + format!(r"{probe}\\foo"), + "an entry is accepted with a trailing separator and concatenated \ + without normalising the join" + ); + + // An entry that does not name an existing directory is rejected, and the + // call rewrites it to the drive root rather than leaving it stale. + // + // Derived from the probe directory rather than hard-coded: a literal like + // `C:\no-such-directory-for-this-test` is only missing until some host + // happens to have it, and the test would then assert the rejected case + // against an accepted one. A child of a directory this test just created + // cannot exist unless something else creates it in between. + let missing = probe_dir.path.join("no-such-child"); + assert!( + !missing.exists(), + "precondition: the rejected entry must name nothing: {}", + missing.display() + ); + set_drive_entry( + drive, + Some(missing.to_str().expect("the probe path is UTF-8")), + ); + assert_eq!( + resolve(&format!("{drive}:foo")), + format!(r"{drive}:\foo"), + "an entry that names nothing is rejected in favour of the drive root" + ); + assert_eq!( + drive_entry(drive).map(|v| v.to_string_lossy()).as_deref(), + Some(format!(r"{drive}:\").as_str()), + "and the call REWROTE the entry: this is a query that mutates the \ + process environment block" + ); + + // Absent entirely, the entry is created rather than merely read. + set_drive_entry(drive, None); + assert_eq!(drive_entry(drive), None, "precondition: entry cleared"); + let _ = resolve(&format!("{drive}:foo")); + assert_eq!( + drive_entry(drive).map(|v| v.to_string_lossy()).as_deref(), + Some(format!(r"{drive}:\").as_str()), + "resolving created the entry on a host that had none" + ); +} + +#[test] +fn a_rejected_drive_entry_is_replaced_by_the_drive_root() { + // Acceptance needs BOTH a shape and an existence check, and a draft of the + // module doc claimed it was "a filesystem query rather than a syntax test" + // -- having measured only the existence half. Every value below names an + // existing directory, so anything rejected here is rejected on shape alone. + // + // Pinned because the distinction is not guessable and the doc asserts it. + let probe_dir = probe_directory("shape"); + let accepted = probe_dir.path.to_str().expect("the probe path is UTF-8"); + let drive = probe_drive_from(probe_drives::REJECTED_ENTRY, probe_dir.drive()); + let _restore = BorrowedDriveEntry::take(drive); + + // The control: this exact directory IS accepted in canonical form, so the + // rejections below cannot be blamed on the directory itself. + set_drive_entry(drive, Some(accepted)); + assert_eq!( + resolve(&format!("{drive}:foo")), + format!(r"{accepted}\foo"), + "control: the same directory in canonical form is accepted" + ); + + // Same directory, spellings that are not fully-qualified `X:\...` form. + // Each names something that exists; each is rejected anyway. + for spelling in [ + accepted.replace('\\', "/"), + format!(r"{accepted}\."), + format!( + r"{accepted}\..\{}", + accepted.rsplit('\\').next().unwrap_or("") + ), + format!(r"\\?\{accepted}"), + ] { + set_drive_entry(drive, Some(&spelling)); + assert_eq!( + resolve(&format!("{drive}:foo")), + format!(r"{drive}:\foo"), + "{spelling:?} names an existing directory but is rejected on shape" + ); + assert_eq!( + drive_entry(drive).map(|v| v.to_string_lossy()).as_deref(), + Some(format!(r"{drive}:\").as_str()), + "and the rejected entry is written back as the drive root" + ); + } + + // The type check, which is a separate necessary condition from both the + // shape above and the existence check in the sibling test. The module doc + // has listed an existing FILE among the rejections since the drive-entry + // work, and nothing pinned it -- so the one observation distinguishing + // "names a directory" from "names something" lived only in prose. + // + // Not a file this test creates: `ProbeDir` may be the read-only + // `%SystemRoot%` fallback, where creating one needs privileges the suite + // must not assume. This one is present on every Windows host by + // construction, and setting an entry to a file does not touch the file. + let system_file = std::path::PathBuf::from( + std::env::var("SystemRoot").expect("SystemRoot is set on Windows"), + ) + .join("System32") + .join("kernel32.dll"); + assert!( + system_file.is_file(), + "precondition: the rejected entry must name an existing FILE: {}", + system_file.display() + ); + let system_file = system_file.to_str().expect("the system path is UTF-8"); + + set_drive_entry(drive, Some(system_file)); + assert_eq!( + resolve(&format!("{drive}:foo")), + format!(r"{drive}:\foo"), + "{system_file} exists and is canonical, and is rejected anyway because \ + it is not a directory -- so existence alone is not the gate" + ); + assert_eq!( + drive_entry(drive).map(|v| v.to_string_lossy()).as_deref(), + Some(format!(r"{drive}:\").as_str()), + "and an entry naming a file is written back as the drive root too" + ); +} + +#[test] +fn a_long_drive_entry_round_trips_through_the_reader() { + // The reader grows its buffer, and this is what proves it. Windows reports + // an undersized buffer by returning the REQUIRED capacity rather than the + // units written, so a reader that treats the two alike slices past its own + // buffer and panics -- while preserving a legitimate entry, which is the + // worst moment for it, because the entry is then never restored. + // + // 1200 units is comfortably past the 256 the reader starts with and past + // the 1024 an earlier fixed-size version used, and is a legitimate value: a + // per-drive entry is a path, and long paths reach far beyond this. + let drive = probe_drive_from(probe_drives::LONG_ENTRY, None); + let _restore = BorrowedDriveEntry::take(drive); + + let long = format!(r"C:\{}", "a".repeat(1200)); + set_drive_entry(drive, Some(&long)); + + let read_back = drive_entry(drive).expect("the entry was just set"); + assert_eq!( + read_back.to_string_lossy(), + long, + "a long entry survives the read, so the buffer grew instead of truncating" + ); +} + +#[test] +fn a_borrowed_drive_entry_is_restored_even_when_the_borrower_panics() { + // The guard exists for the unwinding path, and a suite that passes never + // takes it -- so trusting it would mean shipping an untested defence + // against the exact failure it is there for. This takes the path on + // purpose. + let drive = probe_drive_from(probe_drives::BORROW_GUARD, None); + + // The outer guard is not ceremony. This test installs a sentinel to watch + // the inner guard put back, and without it that install would destroy + // whatever the process inherited -- so the test for not losing borrowed + // state would itself lose some. The inner guard still takes the unwinding + // path; the outer one covers this test's own borrow. + let _outer = BorrowedDriveEntry::take(drive); + let sentinel = format!(r"C:\borrowed-entry-{}", std::process::id()); + set_drive_entry(drive, Some(&sentinel)); + + let outcome = std::panic::catch_unwind(|| { + let _restore = BorrowedDriveEntry::take(drive); + set_drive_entry(drive, Some(r"C:\the-borrowed-value")); + panic!("expected: this panic exercises the restore-on-unwind path"); + }); + assert!( + outcome.is_err(), + "precondition: the borrower must actually panic, or the unwinding path \ + is not the thing being measured" + ); + + assert_eq!( + drive_entry(drive).map(|v| v.to_string_lossy()).as_deref(), + Some(sentinel.as_str()), + "the guard put the entry back while unwinding, where an end-of-test \ + restore would have been skipped" + ); +} + +#[test] +fn an_empty_drive_entry_is_distinguished_from_an_absent_one() { + // **This pins a correction, not a discovery.** The reader used to fold both + // into `None`, on a recorded measurement that an empty value and an absent + // name are the same state. They are not; the measurement behind that claim + // never cleared the last error, so it could only have read whatever an + // earlier call left behind -- the same "stated more precisely than the + // evidence reaches" failure this crate keeps meeting, committed inside the + // comment that called itself measured. + // + // The consequence is what makes it worth a test rather than a fix: with the + // two collapsed, restoring an inherited EMPTY entry deletes it, so the guard + // written to preserve process state destroys it in exactly one case. + let drive = probe_drive_from(probe_drives::EMPTY_VS_ABSENT, None); + let _outer = BorrowedDriveEntry::take(drive); + + set_drive_entry(drive, Some("")); + let empty = drive_entry(drive); + assert_eq!( + empty.as_ref().map(|v| v.to_string_lossy()), + Some(String::new()), + "an entry set to the empty string reads back as PRESENT and empty" + ); + + set_drive_entry(drive, None); + assert_eq!( + drive_entry(drive), + None, + "and a deleted entry reads back as absent, which is the answer the \ + empty one must not be confused with" + ); + + // The two are distinct in the round trip as well as in the read, which is + // the property restoration actually depends on. + set_drive_entry_units(drive, empty.as_ref()); + assert_eq!( + drive_entry(drive).as_ref().map(|v| v.to_string_lossy()), + Some(String::new()), + "restoring an empty entry puts back an empty entry, not an absent one" + ); +} diff --git a/crates/windows-namespace-request-sys/src/path.rs b/crates/windows-namespace-request-sys/src/path.rs index 2ef29b2ed..e211783a2 100644 --- a/crates/windows-namespace-request-sys/src/path.rs +++ b/crates/windows-namespace-request-sys/src/path.rs @@ -15,8 +15,34 @@ //! //! # A resolved path is not a session-independent path //! -//! `GetFullPathNameW` is **lexical**. It resolves relative components and -//! `.`/`..` and never expands a drive letter, and a drive letter resolves +//! `GetFullPathNameW` **does not verify what it produces** -- though for a +//! drive-relative path naming another drive it does query the filesystem, and +//! may rewrite that drive's recorded entry (see [`crate::full_path`]). It +//! collapses `.`/`..` +//! lexically, and it *additionally* roots most paths that are not fully +//! qualified against process state -- the current directory, or for a +//! root-relative path that directory's *root*, or for a drive-relative path +//! naming another drive the entry recorded for it in the `=C:` environment +//! variables -- used verbatim when accepted, so it need not even be on that +//! drive, and replaced by the drive root when not (for the current drive the +//! process directory is used and the entry makes no difference). It is +//! therefore not a lexical call as a whole, which is what makes resolving on +//! the submitting thread meaningful. +//! +//! **"Most" rather than "every", because a legacy device name short-circuits +//! the rooting entirely.** [`prepare`] hands the input to that call without +//! device handling of its own, so `prepare("CON")` yields `\\.\CON` -- a device, +//! not a file under the current directory -- and the same holds for `CON:`, +//! `NUL`, `LPT1:` and the rest of the legacy set. A caller passing through an +//! untrusted name should know that. Anything with more after it (`CON.txt`, +//! `a\CON`) roots normally -- **except for `NUL`, the one member a path in +//! front of it does not save**: `prepare(r"C:\NUL")` is `\\.\NUL`, so a fully +//! qualified path is not by itself evidence that a name refers to a file on +//! that volume. Only a suffix (`NUL.txt`) takes it out. The full shape is in +//! [`crate::full_path`], which documents the call itself. +//! +//! What it never does is expand +//! a drive letter, and a drive letter resolves //! against the *logon session* of whatever token is in effect. So a path //! prepared on a submitting thread and opened on a worker under a captured //! token from another session can name a different device. Preparation closes @@ -177,7 +203,7 @@ impl std::error::Error for PathError { /// Preparation resolves against the *process* current directory, on the /// calling thread, which is what stops the meaning of a relative path changing /// between submission and execution. It does **not** expand a drive letter, -/// because `GetFullPathNameW` is lexical and never does -- and a drive letter +/// because `GetFullPathNameW` never does -- and a drive letter /// is resolved against the logon session of the token in effect at open time. /// A prepared path carried to a worker running under a captured token from /// another logon session can therefore still name a different device. That diff --git a/crates/windows-namespace-request-sys/tests/acceptance/operations.rs b/crates/windows-namespace-request-sys/tests/acceptance/operations.rs index d6b2dc6e0..3ff154fc5 100644 --- a/crates/windows-namespace-request-sys/tests/acceptance/operations.rs +++ b/crates/windows-namespace-request-sys/tests/acceptance/operations.rs @@ -275,7 +275,13 @@ fn watcher_getvolumeinformationbyhandle_shape_is_reachable() { #[test] fn enumeration_getfullpathname_shape_is_reachable() { - // path.rs:149 -- lexical resolution with a null file-part out-param. + // enumeration path.rs:149 -- resolution with a null file-part out-param. + // Qualified by crate because this is the one consumer filename that also + // exists in THIS crate, and a reviewer resolved it against the local + // `src/path.rs` -- where line 149 is unrelated -- and reported the citation + // as stale. It is not. The neighbouring "watcher directory.rs" / + // "enumeration native.rs" citations already use this form for the same + // reason. let resolved = ResolveFullPath::new(Wtf16String::from(r"C:\Windows\System32\..\.\Temp")) .perform() .expect("the enumeration crate's resolution shape") diff --git a/crates/windows-namespace-request-sys/tests/unc_current_directory.rs b/crates/windows-namespace-request-sys/tests/unc_current_directory.rs new file mode 100644 index 000000000..aaa999677 --- /dev/null +++ b/crates/windows-namespace-request-sys/tests/unc_current_directory.rs @@ -0,0 +1,216 @@ +// Copyright (c) Mike Grier. + +//! Root-relative rooting under a UNC current directory. +//! +//! An integration test, and a two-process one, because the property is not +//! expressible any other way. The current directory is **process-wide**, so a +//! test that wants to observe what `GetFullPathNameW` does under a UNC one has +//! to *be* a process that has a UNC one -- and it must not impose that on the +//! rest of the suite, which runs as threads in the same process. +//! +//! What it pins is the branch the module doc calls out and nothing else +//! reached: a root-relative path (`\foo`) is rooted at the **root of the +//! current directory**, which under a UNC current directory is the share root +//! `\\server\share\` and not a drive. The unit test for the same rule runs +//! under whatever directory launched the suite, so on ordinary drive-rooted CI +//! it only ever exercises the `C:\foo` half; it computes the UNC expectation +//! but never puts the call in a position to produce one. +//! +//! The child is this same test binary, re-executed with a marker in its +//! environment and a filter naming the one test to run. That keeps the +//! assertion beside the thing it asserts rather than in a fixture binary whose +//! drift nothing would catch. + +use windows_namespace_request_sys::ResolveFullPath; +use wtf_string::Wtf16String; + +/// Set on the child, absent on the parent, and carrying the directory the +/// parent placed it in. +/// +/// **The value is load-bearing, and an earlier version ignored it.** Presence +/// alone was the signal, so a variable of this name already in the environment +/// -- a leftover from debugging the child by hand, say -- made the PARENT take +/// the child branch, in the ordinary drive-rooted directory cargo starts it in. +/// The test then failed on the UNC precondition, blaming the crate for an +/// environment collision. +/// +/// Carrying the intended directory fixes that and buys a real check besides: +/// the child now confirms it is where the parent put it, rather than inferring +/// it from the shape of whatever directory it happens to hold. +const CHILD_MARKER: &str = "WNRS_UNC_CURRENT_DIRECTORY_CHILD"; + +/// The administrative share, which is the only UNC path a test can rely on +/// existing without creating one -- and creating one needs privileges the suite +/// must not assume it has. +const UNC_ROOT: &str = r"\\localhost\C$"; + +/// The child runs in a SUBDIRECTORY of the share, not at its root, and the +/// distinction is the whole test. +/// +/// **At the share root the two rooting rules coincide**, so the first version of +/// this test could not tell them apart. Rooting `\foo` at the *root of* the +/// current directory and rooting it at the *whole* current directory both give +/// `\\localhost\C$\foo` when the current directory IS the root -- which is the +/// exact distinction the module doc cites this test as pinning. Measured one +/// level down, they separate: `\foo` gives `\\localhost\C$\foo` while `foo` +/// gives `\\localhost\C$\Windows\foo`. +/// +/// Candidates rather than one name, because the share is `C:` and Windows need +/// not be installed there. Any enterable subdirectory works; these three are +/// the ones a Windows system drive has. +const UNC_SUBDIRECTORY_CANDIDATES: &[&str] = &["Windows", "Users", "ProgramData"]; + +const TEST_NAME: &str = "a_root_relative_path_takes_the_share_root_under_a_unc_current_directory"; + +// Ignored by default and run explicitly in CI, which is this repository's +// existing "ignored tier" pattern rather than a new one. +// +// `C$` is an ADMINISTRATIVE share. It is reachable for a member of +// Administrators and not for an ordinary user, and it can be switched off +// entirely -- so running by default would turn `cargo test` red for a +// non-administrator developer on a perfectly good host, reporting an +// environment limitation as a defect in the crate. Provisioning a share of our +// own is not an escape: creating one needs the same privileges. +// +// The alternative -- skipping quietly when the share is missing -- is the one +// thing that must not happen, because this is the ONLY coverage of the UNC +// branch and a silent skip would leave it unpinned while reporting otherwise. +// `#[ignore]` keeps that visible: an ignored test is counted and named in the +// output, where a skip inside a passing test is not. +#[test] +#[ignore = "needs a reachable administrative share for a UNC current directory; run in CI with --include-ignored"] +fn a_root_relative_path_takes_the_share_root_under_a_unc_current_directory() { + if let Some(placed_at) = std::env::var_os(CHILD_MARKER) { + let placed_at = placed_at + .to_str() + .expect("the parent passes a UTF-8 directory") + .to_owned(); + assert_root_relative_takes_the_share_root(&placed_at); + return; + } + + // The precondition is asserted rather than silently skipped. A test that + // quietly passes when it could not run is the vacuity this crate keeps + // finding, and it is worse here than elsewhere: this is the only coverage + // of the branch, so a silent skip would leave the doc unpinned while + // reporting that it is pinned. + assert!( + std::path::Path::new(UNC_ROOT).is_dir(), + "{UNC_ROOT} is not reachable, so no UNC current directory can be \ + established -- an environment limitation, not a failure of the \ + behaviour under test" + ); + + let working_directory = UNC_SUBDIRECTORY_CANDIDATES + .iter() + .map(|name| format!(r"{UNC_ROOT}\{name}")) + .find(|path| std::path::Path::new(path).is_dir()) + .unwrap_or_else(|| { + panic!( + "none of {UNC_SUBDIRECTORY_CANDIDATES:?} exists under {UNC_ROOT}, \ + so the child cannot be placed BELOW the share root -- and at the \ + root the two rooting rules give the same answer, which is what \ + this test exists to separate" + ) + }); + + let exe = std::env::current_exe().expect("the test binary knows its own path"); + let status = std::process::Command::new(exe) + .arg("--exact") + .arg(TEST_NAME) + .arg("--nocapture") + // Without this the child runs zero tests and exits 0 -- the parent + // would report success having measured nothing at all. The test it is + // told to run is the ignored one, so the filter alone is not enough. + .arg("--include-ignored") + .env(CHILD_MARKER, &working_directory) + .current_dir(&working_directory) + .status() + .expect("re-execute this test binary with a UNC current directory"); + + assert!( + status.success(), + "the child, running with {working_directory} as its current directory, \ + did not pass: {status}" + ); +} + +fn assert_root_relative_takes_the_share_root(placed_at: &str) { + let cwd = std::env::current_dir().expect("the child has a current directory"); + let cwd = cwd.to_str().expect("the current directory is UTF-8"); + + // Proving the child is where the parent put it, against the parent's own + // record rather than against the shape of the directory. Two things can go + // wrong and this separates them: a process can LOSE a UNC current directory + // (`cmd.exe` refuses one and falls back to the Windows directory), and this + // branch can be entered by something that is not the parent's child at all, + // when `CHILD_MARKER` is already set in the environment. + assert!( + cwd.eq_ignore_ascii_case(placed_at.trim_end_matches('\\')), + "the child is at {cwd}, not the {placed_at} the parent chose. If no \ + parent placed it there, {CHILD_MARKER} is set in this environment and \ + should be unset -- that is a collision, not a defect in the crate" + ); + + assert!( + cwd.starts_with(r"\\"), + "precondition: the child must actually hold a UNC current directory, \ + not a drive-rooted one: {cwd}" + ); + + // **And below the share root**, which the first version of this test did + // not check because it did not need to -- it ran AT the root, where the two + // rules being separated give the same answer. This assertion is the one + // that keeps the test from going vacuous again if the parent's directory + // choice ever regresses. + assert_ne!( + cwd.trim_end_matches('\\'), + UNC_ROOT, + "precondition: the child must be BELOW the share root. At the root, \ + rooting at the root of the current directory and rooting at the whole \ + current directory agree, so neither assertion below can tell them apart" + ); + + let resolved = ResolveFullPath::new(Wtf16String::from(r"\foo")) + .perform() + .expect("a root-relative path resolves"); + + // **Compared case-sensitively on purpose, and a review read that as a + // portability bug.** The concern was that a host recording the share as + // `LOCALHOST` would fail this equality against the lowercase literal. It + // cannot, because both sides descend from `UNC_ROOT`: the parent builds the + // child's working directory from it, and the root this call returns is the + // root of that directory. Measured -- a process started with + // `\\LOCALHOST\C$\Windows` reports exactly that back from + // `current_dir()`, and likewise for `\\localhost\c$\Windows`, so Windows + // preserves the spelling it was given rather than canonicalising it. + // + // The precondition above is case-INSENSITIVE for a different reason: it + // compares against a value that may have crossed a process boundary by a + // route this test does not control, so it accepts what a Windows path + // comparison would. + assert_eq!( + resolved.to_string_lossy(), + format!(r"{UNC_ROOT}\foo"), + "a root-relative path takes the ROOT of the current directory, which \ + under a UNC current directory is the share root -- so \"the current \ + drive\", as a draft of the module doc said, has no referent here" + ); + + // The contrast, and the reason the assertion above means anything. An + // ordinary relative path takes the WHOLE current directory, so the two + // resolutions differ by everything below the share root. Without this, a + // resolver that ignored the leading separator entirely would still satisfy + // the equality above whenever the child happened to sit at the root. + let relative = ResolveFullPath::new(Wtf16String::from("foo")) + .perform() + .expect("a relative path resolves"); + + assert_eq!( + relative.to_string_lossy(), + format!(r"{cwd}\foo"), + "an ordinary relative path takes the whole current directory, not its \ + root -- the two rules are distinguishable here and identical at the \ + share root" + ); +} diff --git a/crates/windows-platform-probes/CHECKLIST.md b/crates/windows-platform-probes/CHECKLIST.md index e7360a478..6aebfbfc6 100644 --- a/crates/windows-platform-probes/CHECKLIST.md +++ b/crates/windows-platform-probes/CHECKLIST.md @@ -88,37 +88,7 @@ speculative list to extend by imagination -- a fourth is added when a fourth con explicitly after being pointed at the question. Nothing in the suite decides it either way, which is itself the argument for the oracle. -- [ ] **M2.6** -- Say precisely what `GetFullPathNameW` does, in the crate that owns it, and decide - whether it is still the call `prepare` wants. Two successive descriptions in the cost probe were - each wrong in the same direction: *a syscall cost*, which a timing loop cannot establish, and then - *lexical*, which it also is not. The probe now states the cost and declines the mechanism, which is - honest but leaves the question open one layer down. - - [../windows-namespace-request-sys/src/full_path.rs](../windows-namespace-request-sys/src/full_path.rs) - carries the same imprecision, and is the crate that owns the answer: its module doc says "This call - is **lexical**. It resolves relative components and `.`/`..` against the process current - directory". Those two sentences disagree -- consulting the current directory is process state, and - for a drive-relative path (`C:foo`) it also reads the per-drive current directory held in the - `=C:` environment variables. "Touches no filesystem" is the claim that holds; "lexical" is not. - - **The mono-repo rule says fix the layer, so the correction belongs in - `windows-namespace-request-sys`, not in the probe that consumes it.** It is queued rather than - taken because that crate is outside this peel and is release-managed, so a docs change there is its - own commit with its own scope. - - The decision half is the part worth an engineer's attention rather than a sweep. A genuinely - lexical canonicalizer exists -- `PathCchCanonicalizeEx`, or `PathAllocCanonicalize` -- and would be - cheaper, with no process state read at all. **It is very likely the wrong call anyway**, because - resolving against the current directory *at submission* is the property the namespace design is - buying: the CWD is shared mutable state, so a relative path means something different depending on - when it is resolved, and pinning that on the submitting thread is the whole point. Record that - conclusion explicitly, with the alternative named, so the next reader does not re-derive it -- and - if it is wrong, the cheaper call is sitting there. - - Also worth settling while the question is open: whether `GetFullPathNameW` can enter the kernel at - all on any path this crate takes. The probe measured ~212 ns for a build on x86_64 and declines to - say what that is made of; the owning crate could say, and a reader of either would then stop - guessing. +- [x] **M2.6** -- Say what `GetFullPathNameW` does, in the crate that owns it, and whether it stays. -> [completed 2026-09-09](COMPLETED-CHECKLIST.md#m26) - [ ] **M2.7** -- Decide whether the other nine probe steps in CI should carry `if: '!cancelled()'`, and apply or record the decision. @@ -156,3 +126,26 @@ speculative list to extend by imagination -- a fourth is added when a fourth con operation completed successfully" under a message saying something failed. And attach it only to a condition that is genuinely an OS failure: a call that returned a size rather than an error should not carry one, since `GetLastError` says nothing about it. + +- [ ] **M2.9** -- Stop `request_cost` calling a cross-host ratio "the finding". + + [src/request_cost.rs](src/request_cost.rs) ends its module doc with "Absolute values are + host-specific; the **ratios against the doorbell and the atomic** are the finding." The ratios that + [src/bin/request_cost.rs](src/bin/request_cost.rs) actually prints divide THIS host's measurement by + `DOORBELL_NS_REFERENCE` / `ATOMIC_NS_REFERENCE`, which are constants measured on the Snapdragon X2 + development machine. A ratio with this host's numerator and another host's denominator is neither a + same-host ratio nor a portable finding, and the emitted report says as much two lines later: + "re-read that probe on this host before trusting them". So the module doc promotes to "the finding" + exactly the number its own output tells the reader not to trust. + + **Pre-existing, and deliberately not fixed in the `GetFullPathNameW` peel (PR #86) that found it.** + It arrived in `ae1e39f`, is already on `main`, and is outside that branch's diff; folding it in + would have put an unrelated behavioural change into a documentation peel that had already run to + nineteen review rounds. + + The fix is a decision, not a sweep, which is why this is queued rather than taken: either compute + both figures on the same host and run (the probe would have to measure the doorbell itself, or read + a companion artifact), or keep the fixed references and demote them in the prose from "the finding" + to a labelled cross-host comparison. The first is more useful and more work; the second is honest + and cheap. Same defect class as PR #86's subject -- a claim stated more strongly than the evidence + supports -- so whichever is chosen, the wording has to end up matching what the numbers can carry. diff --git a/crates/windows-platform-probes/COMPLETED-CHECKLIST.md b/crates/windows-platform-probes/COMPLETED-CHECKLIST.md index 02129fa4a..9e8797022 100644 --- a/crates/windows-platform-probes/COMPLETED-CHECKLIST.md +++ b/crates/windows-platform-probes/COMPLETED-CHECKLIST.md @@ -93,3 +93,63 @@ piece of work rather than a correction to that one. The reason any of it works is that Rust's `Stdout` wraps a `LineWriter` and flushes at each newline even when redirected -- had stdout been block-buffered this milestone would have needed a per-line flush too. + +## Moved 2026-09-09 -- M2.6: what `GetFullPathNameW` does, and whether it stays + +### M2.6 -- Say what `GetFullPathNameW` does, in the crate that owns it, and whether it stays. *(completed 2026-09-09 22:54:01 UTC-04:00)* + +**Resolved.** The correction and the decision both landed in the owning crate as `D-18` in +[../windows-namespace-request-sys/DESIGN-NOTES.md](../windows-namespace-request-sys/DESIGN-NOTES.md): +`GetFullPathNameW` collapses `.`/`..` lexically but roots most paths that are not fully qualified against +process state, so it is not a lexical call as a whole; `PathCchCanonicalizeEx` does not root, and is +the wrong call for that reason, because rooting at submission is the property being bought. No cost +comparison is claimed -- the item below asked whether the alternative "would be cheaper", and the +answer recorded in D-18 is that nothing measures it, so the decision rests on semantics alone. +The mechanism question this item raised is answered rather than left open: resolving a +drive-relative path for another drive checks that drive's recorded entry against the filesystem +and writes the entry back, so the call does touch the filesystem on that form. The item's body below is the +request as it was written, and quotes the module doc as it read before the correction. + +- [x] **M2.6** -- Say precisely what `GetFullPathNameW` does, in the crate that owns it, and decide + whether it is still the call `prepare` wants. Two successive descriptions in the cost probe were + each wrong in the same direction: *a syscall cost*, which a timing loop cannot establish, and then + *lexical*, which it also is not. The probe now states the cost and declines the mechanism, which is + honest but leaves the question open one layer down. + + [../windows-namespace-request-sys/src/full_path.rs](../windows-namespace-request-sys/src/full_path.rs) + carries the same imprecision, and is the crate that owns the answer: its module doc says "This call + is **lexical**. It resolves relative components and `.`/`..` against the process current + directory". Those two sentences disagree -- consulting the current directory is process state, and + for a drive-relative path (`C:foo`) it also reads the per-drive current directory held in the + `=C:` environment variables. "Touches no filesystem" is the claim that holds; "lexical" is not. + + *(Later correction: the second half stood, the first did not. "Touches no filesystem" was measured + false while carrying out this item -- resolving `X:foo` for a non-current drive distinguishes an + existing directory from an existing file from a missing one, and rewrites the `=X:` entry when that + check REJECTS it -- an accepted entry is left alone, so the write is conditional rather than part of + every such resolution. What + Microsoft documents is only that the call does not VERIFY its result. Two smaller things in the + paragraph above also turned out to be stated too broadly: the per-drive entry is consulted for a + drive OTHER than the current one, and on the current drive it makes no difference to the result -- + and "reads" is a mechanism word that observation cannot reach either way. See + [../windows-namespace-request-sys/DESIGN-NOTES.md](../windows-namespace-request-sys/DESIGN-NOTES.md) + -> `D-18`.)* + + **The mono-repo rule says fix the layer, so the correction belongs in + `windows-namespace-request-sys`, not in the probe that consumes it.** It is queued rather than + taken because that crate is outside this peel and is release-managed, so a docs change there is its + own commit with its own scope. + + The decision half is the part worth an engineer's attention rather than a sweep. A genuinely + lexical canonicalizer exists -- `PathCchCanonicalizeEx`, or `PathAllocCanonicalize` -- and would be + cheaper, with no process state read at all. **It is very likely the wrong call anyway**, because + resolving against the current directory *at submission* is the property the namespace design is + buying: the CWD is shared mutable state, so a relative path means something different depending on + when it is resolved, and pinning that on the submitting thread is the whole point. Record that + conclusion explicitly, with the alternative named, so the next reader does not re-derive it -- and + if it is wrong, the cheaper call is sitting there. + + Also worth settling while the question is open: whether `GetFullPathNameW` can enter the kernel at + all on any path this crate takes. The probe measured ~212 ns for a build on x86_64 and declines to + say what that is made of; the owning crate could say, and a reader of either would then stop + guessing. diff --git a/crates/windows-platform-probes/src/bin/device_map.rs b/crates/windows-platform-probes/src/bin/device_map.rs index bb11ba70f..332a95a5b 100644 --- a/crates/windows-platform-probes/src/bin/device_map.rs +++ b/crates/windows-platform-probes/src/bin/device_map.rs @@ -119,7 +119,7 @@ fn render(out: &mut dyn std::fmt::Write) { ); let _ = writeln!( out, - " different device -- which is why lexical resolution does not" + " different device -- which is why resolving the path does not" ); let _ = writeln!(out, " close that hazard."); } else { diff --git a/crates/windows-platform-probes/src/bin/request_cost.rs b/crates/windows-platform-probes/src/bin/request_cost.rs index 6a2f45edf..58a74b74f 100644 --- a/crates/windows-platform-probes/src/bin/request_cost.rs +++ b/crates/windows-platform-probes/src/bin/request_cost.rs @@ -282,7 +282,7 @@ fn render(out: &mut dyn std::fmt::Write) { { let _ = writeln!( out, - "\n WHERE THE TIME ACTUALLY GOES, and it is not the allocator:" + "\n WHERE THE TIME ACTUALLY GOES, and it is not only the allocator:" ); // "resolves against process state" -- not "a syscall", and not // "lexical" either. @@ -290,8 +290,14 @@ fn render(out: &mut dyn std::fmt::Write) { // The first was wrong because a timing loop cannot establish a kernel // transition. The second, which replaced it, is wrong for a symmetric // reason: `GetFullPathNameW` consults the process current directory, - // and for a drive-relative path the per-drive current directory held in - // the `=C:` environment variables, so it is not pure string work. A + // and for a drive-relative path naming a drive OTHER than the current + // one the per-drive current directory held in the `=C:` environment + // variables, so it is not pure string work. (On the current drive that + // entry makes no difference to the result; whether it is read is not + // observable and is not claimed. The qualifier was missing here for two + // rounds after the module doc above gained it -- the sweep reached the + // module doc and the emitted report and stopped short of this inline + // comment.) A // genuinely lexical canonicalizer is a different call // (`PathCchCanonicalizeEx`), and it is deliberately NOT the one // `prepare` wants -- resolving against the CWD at submission is the @@ -301,31 +307,48 @@ fn render(out: &mut dyn std::fmt::Write) { // two successive attempts to name one were each wrong in the same way. let _ = writeln!( out, - " `prepare` calls GetFullPathNameW to resolve the path against the" + " `prepare` calls GetFullPathNameW, which roots MOST paths that are not fully" ); let _ = writeln!( out, - " process working directory, because the CWD is mutable by any thread" + " qualified against process state -- most, because a legacy device name such" ); let _ = writeln!( out, - " and resolving later would be racy. That reads process state and" + " as CON short-circuits rooting entirely. The CWD is mutable by any thread, so" ); let _ = writeln!( out, - " touches no filesystem -- and it is not an allocation, so most of the" + " resolving later would be racy. BOTH SAMPLES HERE ARE FULLY QUALIFIED, so" ); let _ = writeln!( out, - " cost above is work no allocation scheme can remove. Whether any of" + " that rooting is the motivation for resolving at submission and is not" ); + let _ = writeln!(out, " what these numbers measure."); let _ = writeln!( out, - " it enters the kernel is not something this run measured." + " The gap between building and cloning bounds the resolution step from" ); let _ = writeln!( out, - " Two different schemes recover two different things, and this said" + " above. It is not the call's own cost: it also spans ONE NET allocation" + ); + let _ = writeln!( + out, + " of this crate's own -- prepare allocates twice against the clone's once," + ); + let _ = writeln!( + out, + " so the subtraction cancels one -- and the builder chain." + ); + let _ = writeln!( + out, + " Whether any of it enters the kernel is not something this run measured." + ); + let _ = writeln!( + out, + " Two different schemes recover different things, and this said" ); let _ = writeln!( out, @@ -333,16 +356,16 @@ fn render(out: &mut dyn std::fmt::Write) { ); let _ = writeln!( out, - " of {build:.0} ns, so it recovers {:.0} ns -- but that saving is the Win32", + " of {build:.0} ns, so it recovers {:.0} ns -- but that saving is the", build - clone ); let _ = writeln!( out, - " resolution, not an allocation, and only a caller that can reuse a" + " RESOLUTION STEP plus that net allocation, and only a caller that can" ); let _ = writeln!( out, - " resolved path gets it. INLINE STORAGE removes the allocation and" + " reuse a resolved path gets it. INLINE STORAGE removes the allocation and" ); let _ = writeln!( out, diff --git a/crates/windows-platform-probes/src/request_cost.rs b/crates/windows-platform-probes/src/request_cost.rs index 1f7bedf52..bcaa957a1 100644 --- a/crates/windows-platform-probes/src/request_cost.rs +++ b/crates/windows-platform-probes/src/request_cost.rs @@ -55,25 +55,58 @@ //! copy. So "what does a request cost" is not only an allocation question, and //! measuring only the path would understate it. //! -//! # Preparing a path is a Win32 call, not an allocation +//! # Preparing a path is a Win32 call as well as an allocation //! //! This probe was written expecting `prepare` to be an allocation and a copy. -//! It is not: it calls **`GetFullPathNameW`** to resolve the path against the +//! It is not *only* that -- the qualifier matters, because the decomposition +//! below charges the measured gap with one net allocation, so a heading saying +//! flatly "not an allocation" contradicts it. This heading said exactly that +//! for two rounds after the sentence you are reading was written to explain why +//! it must not: the correction went into the prose and stopped one line short +//! of the heading above it. What `prepare` additionally does is call +//! **`GetFullPathNameW`** to resolve the path against the //! process working directory, because [the namespace session] settled that the //! path is resolved at submission -- the process CWD is mutable by any thread, //! so even perfect remoting would be racy. //! -//! That work reads **process state**: it resolves against the current -//! directory, and for a drive-relative path against the per-drive current -//! directory held in the `=C:` environment variables. So the measured remainder -//! is path resolution, not allocation -- and naming a *mechanism* for it has -//! now been got wrong twice. Calling it a *syscall cost* claimed a kernel -//! transition a timing loop cannot establish; calling it *lexical*, which -//! replaced it, claimed pure string work it equally is not. A genuinely lexical -//! canonicalizer is a different call (`PathCchCanonicalizeEx`) and is +//! That call reads **process state** when it has to root a path -- the current +//! directory, or for a drive-relative path naming a drive OTHER than the +//! current one the entry recorded for that drive in the `=C:` environment +//! variables -- on the current drive that entry makes no difference and the +//! process directory wins. **Neither sample here is rooted**: both are +//! fully qualified, so the rooting is why resolution happens at submission and +//! is not what these timings contain. +//! +//! So the measured remainder is the **resolution step**, which is an upper +//! bound on the call and not the call itself: it also spans one net allocation +//! of this crate's own and the builder chain. **Net**, because the subtraction +//! cancels one -- `prepare` allocates twice, an input copy and a `MAX_PATH` +//! output buffer, against the clone's one, so what survives the subtraction is +//! the difference and not both. Calling it "two allocations", as a draft did, +//! charges the gap with allocator work the subtraction has already removed. +//! Saying the remainder *is* the resolution, +//! as an earlier revision did, hands the call credit for the allocator work the +//! same sentence sets out to exclude. +//! +//! Naming a *mechanism* for it has now been got wrong repeatedly. A *syscall +//! cost* claimed a kernel transition a timing loop cannot establish; *lexical*, +//! which replaced it, claimed pure string work it equally is not. A genuinely +//! lexical canonicalizer is a different call (`PathCchCanonicalizeEx`) and is //! deliberately not the one wanted here, because resolving against the CWD at //! submission is the property being bought. What survives either way is the -//! part that matters: an allocator cannot remove it. +//! part that matters: an allocator cannot remove all of it. +//! +//! The owning crate now settles both halves rather than leaving them to be +//! re-derived from a probe: see `windows-namespace-request-sys`' +//! [DESIGN-NOTES.md](../../windows-namespace-request-sys/DESIGN-NOTES.md) -> +//! `D-18`, which states what the call actually does and records keeping it over +//! the canonicalizers that do not root. It claims no cost comparison against +//! those alternatives, because nothing here measures them -- but it does settle +//! the mechanism question this module once left open: resolving a +//! drive-relative path for another drive checks that drive's recorded entry +//! against the filesystem, and writes the entry back **when that check rejects +//! it** -- an accepted entry is left alone, so the write is conditional rather +//! than part of every such resolution. //! //! The two schemes that might reduce it recover different halves. **Inline //! storage** removes the allocation and copy, which is what @@ -82,6 +115,12 @@ //! place of the whole build, so it recovers the difference between them. Naming //! one figure for both -- as this did -- credits an allocator with work it //! cannot remove. Knowing which half is which is the point of measuring both. +//! +//! A link-reference definition has to be its own block: abutting the paragraph +//! above, CommonMark folds it in, so rustdoc rendered this line as literal text +//! and left the reference to it unlinked. Pre-existing, and fixed here because +//! this module doc is being rewritten around it. +//! //! [the namespace session]: ../../../design-sessions/DESIGN-SESSION-2026-08-27-pseudo-async-namespace-operations.md //! //! Each timing is reported per operation. Absolute values are host-specific; @@ -196,11 +235,17 @@ pub fn measure() -> Observation { // length that varied with the local system directory would make the figure // incomparable between the machines the report asks a reader to compare. // - // The `C:` is safe for the same reason the probe's own conclusion is: a - // fully-qualified path is normalized without consulting a device, so no - // volume is needed behind the letter. Two review passes read this as the - // portability bug fixed above, so it is now measured rather than argued -- - // see `preparing_a_path_needs_no_volume_behind_its_drive_letter`. + // The `C:` is safe, and what makes it safe is an OUTCOME, not a mechanism: + // preparation succeeds with no volume behind the drive letter. Whether a + // device is consulted is neither established here nor needed. A black-box + // success is equally compatible with a consultation whose failure is + // ignored, so claiming the letter is "normalized without consulting a + // device" -- as this comment did -- reaches past its evidence, which is the + // one overreach the owning crate's `D-18` exists to remove. + // + // Two review passes read this as the portability bug fixed above, so the + // outcome is measured rather than argued -- see + // `preparing_a_path_needs_no_volume_behind_its_drive_letter`. let long_text = format!(r"C:\{}\file.txt", vec!["directory"; 24].join("\\")); let long = Wtf16String::from(long_text.as_str()); diff --git a/crates/windows-platform-probes/src/tests.rs b/crates/windows-platform-probes/src/tests.rs index e95abd376..a53d9a232 100644 --- a/crates/windows-platform-probes/src/tests.rs +++ b/crates/windows-platform-probes/src/tests.rs @@ -4358,15 +4358,21 @@ fn a_small_handshake_completes_and_reports_a_positive_round_trip() { // // `request_cost::measure` builds its long-path sample on a hard-coded `C:`, and // two review passes read that as a portability bug: a machine with no `C:` -// volume would panic on the `expect` rather than measure. It would not, and the -// reason is the same fact the probe's own headline conclusion rests on -- that -// `GetFullPathNameW` resolves a fully-qualified path without touching the -// filesystem. A volume that does not exist is therefore not consulted. +// volume would panic on the `expect` rather than measure. It does not -- and +// that, the observable outcome, is the whole of what is claimed here. // // That was an argument, and an argument is what a reviewer had to disbelieve. -// This is the measurement. It also pins the "touches no filesystem" claim -// itself, which nothing else here does: if that claim ever stops holding, the -// probe's account of where its nanoseconds go is wrong, and this fails first. +// This is the measurement. +// +// **It pins less than two earlier versions of this comment claimed.** The first +// said it pinned "touches no filesystem"; the second said it showed the volume +// was "never consulted". Neither follows. A black-box success is equally +// compatible with a consultation whose failure is ignored, so nothing here +// reaches the mechanism -- which is the overreach the owning crate's `D-18` +// exists to remove, committed twice in the comment describing it. +// +// What this pins is the observable outcome, and it is exactly what the probe +// needs: the absence of a volume does not make preparation fail. #[test] fn preparing_a_path_needs_no_volume_behind_its_drive_letter() { @@ -4436,9 +4442,9 @@ fn preparing_a_path_needs_no_volume_behind_its_drive_letter() { assert!( windows_namespace_request_sys::prepare(&path).is_ok(), - "preparing {text} must succeed with no {absent}: volume mounted -- \ - a fully-qualified path is normalized, not resolved against a device, \ - and `request_cost` depends on that both for its long-path sample and \ - for its claim about where the measured time goes" + "preparing {text} must succeed with no {absent}: volume mounted. That \ + outcome is the whole claim -- this says nothing about whether a device \ + is consulted, because a black-box success cannot -- and it is what \ + `request_cost` depends on for its hard-coded long-path sample" ); }