Conversation
…them Distilled from #1245 by Andrew Hundt: the two exclusive-reservation helpers and their two call sites, hand-ported onto main's install.ps1. Nothing else from that file is taken -- the PR's copy also carries an older TLS block that would re-break Windows 10 (#1856), so a wholesale take was not an option. Two paths the installer used were guessable, and one of them adopted whatever it found: $TmpDir = Join-Path $env:TEMP "cbm-install-$(Get-Random)" New-Item -ItemType Directory -Path $TmpDir -Force Get-Random is seeded per process and its space is small, and -Force on an existing directory is a no-op that returns it. A directory planted at a guessed name became the staging area -- and the owner-only DACL the installer applies is set only afterwards, on a directory someone else created. The new helper names the directory with a GUID and creates it with -ErrorAction Stop, so a collision is a retry, never an adoption. $InstallerTmp = "$InstallerDest.new" was a fixed path in the install directory, reservable by anyone who could write there first. The sibling is now reserved with FileMode.CreateNew and FileShare.None on an unguessable name, which is atomic, before anything is copied into it. One deliberate difference from the PR: the sibling reservation runs INSIDE the existing try, and the cleanup checks that a path was assigned. That block is documented as best-effort ("a failure here still leaves a working install"), and a reservation that exhausts its 32 attempts should be one more best-effort miss, not a script abort under ErrorActionPreference=Stop. Verified: pure ASCII (PS 5.1 reads a BOM-less .ps1 as ANSI), LF endings matching main, both old call sites gone from live code. NOT verified: this file was not parsed by PowerShell. There is no pwsh on the macOS host, the Windows VM is down, and CI's Windows guards only check that `update` prints the install.ps1 command -- they never execute it. Opened as a draft for that reason; it needs one PowerShell parse before it merges. Co-authored-by: Andrew Hundt <ATHundt@gmail.com> Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft on purpose: this file has not been parsed by PowerShell yet. See the last section.
Distilled from #1245 by @ahundt — the two exclusive-reservation helpers and their two call sites, hand-ported onto
main'sinstall.ps1. Nothing else from that file is taken: the PR's copy carries an older TLS block that would re-break Windows 10 (#1856), so a wholesale take was never an option.What was wrong
Two paths were guessable, and one adopted whatever it found:
Get-Randomis seeded per process and its space is small;-Forceon an existing directory simply returns it. A directory planted at a guessed name became the staging area — and the owner-only DACL the installer applies is set only afterwards, on a directory someone else created.A fixed path in the install directory, reservable by anyone who could write there first.
The fix
New-Item -ErrorAction Stop, 32 attempts. A collision is a retry, never an adoption.FileMode.CreateNew+FileShare.Noneon an unguessable name — an atomic reservation before anything is copied.One deliberate difference from #1245: the sibling reservation runs inside the existing
try, and cleanup checks a path was assigned. That block is documented as best-effort ("a failure here still leaves a working install"), so exhausting 32 attempts should be one more best-effort miss, not a script abort underErrorActionPreference=Stop.Verified
.ps1as ANSI, so a stray non-ASCII byte is a silent breakage.main.Not verified — why this is a draft
No PowerShell has parsed this file. There is no
pwshon the macOS host, the Windows VM is down, and I checked CI: the Windows guards (tests/windows/test_windows_update_handoff.py) verify thatupdateprints theinstall.ps1command — they never execute it. So nothing in the pipeline would catch a syntax error here. It needs one[Parser]::ParseFileon a Windows machine before it leaves draft.