fix(shim): rewrite x64 only system program path for x86 shim#6620
Open
silver886 wants to merge 9 commits intoScoopInstaller:developfrom
Open
fix(shim): rewrite x64 only system program path for x86 shim#6620silver886 wants to merge 9 commits intoScoopInstaller:developfrom
silver886 wants to merge 9 commits intoScoopInstaller:developfrom
Conversation
fix x64 system program path resolving
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds PE machine detection and WoW64 file-system redirection when emitting shim targets: new Get-PEMachine reads PE headers; shim emission rewrites System32/SysWOW64 paths for x86 shim executables on 64-bit Windows before writing the Changes
Sequence Diagram(s)sequenceDiagram
participant ShimCreator
participant Filesystem
participant PEReader
participant OSChecker
ShimCreator->>Filesystem: copy shim executable to app dir
ShimCreator->>PEReader: Get-PEMachine(shim.exe)
PEReader-->>ShimCreator: machine (e.g., 0x014c)
ShimCreator->>OSChecker: Is64BitWindows?
OSChecker-->>ShimCreator: true
alt shim is x86 (0x014c) on x64
ShimCreator->>ShimCreator: rewrite path: System32→Sysnative, SysWOW64→System32
end
ShimCreator->>Filesystem: write .shim file with rewritten path
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/Scoop-Core.Tests.ps1 (1)
383-423: Prefer asserting actual.shimoutput instead of only string transforms.Current tests validate replacement snippets, but they don’t verify that
shim()emits rewrittenpath = ...under the x86-shim gate. An integration assertion on generated.shimcontent would better protect against regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Scoop-Core.Tests.ps1` around lines 383 - 423, Update the tests to exercise the actual shim() generator and assert on the produced .shim file content rather than only string replace behavior: call shim(...) (or the function that produces the .shim text under the x86-shim gate) with an input that would contain $sysdir or $syswow (use the same $sysdir/$syswow/testPath setup) and assert the returned/generated .shim text contains a "path = ..." line with the expected rewritten value (e.g. Sysnative\notepad.exe for System32->Sysnative and System32\notepad.exe for SysWOW64->System32), and also add a test that feeding a non-system path leaves the "path = ..." line unchanged; reference shim() and the ".shim" output when locating the code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/core.ps1`:
- Around line 24-42: Get-PEMachine can throw before $binaryReader/$fileStream
are assigned, so the finally block's unconditional $binaryReader.Close() and
$fileStream.Close() produce null method-call errors; fix by declaring and
initializing $binaryReader and $fileStream to $null before the try and guard
their Close() calls in the finally (e.g. if ($binaryReader) {
$binaryReader.Close() } and if ($fileStream) { $fileStream.Close() }) so Close()
is only invoked when the objects exist.
---
Nitpick comments:
In `@test/Scoop-Core.Tests.ps1`:
- Around line 383-423: Update the tests to exercise the actual shim() generator
and assert on the produced .shim file content rather than only string replace
behavior: call shim(...) (or the function that produces the .shim text under the
x86-shim gate) with an input that would contain $sysdir or $syswow (use the same
$sysdir/$syswow/testPath setup) and assert the returned/generated .shim text
contains a "path = ..." line with the expected rewritten value (e.g.
Sysnative\notepad.exe for System32->Sysnative and System32\notepad.exe for
SysWOW64->System32), and also add a test that feeding a non-system path leaves
the "path = ..." line unchanged; reference shim() and the ".shim" output when
locating the code to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 918915e0-63b6-4a32-b776-fd163a42f579
📒 Files selected for processing (3)
CHANGELOG.mdlib/core.ps1test/Scoop-Core.Tests.ps1
silver886
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/core.ps1 (1)
24-42:⚠️ Potential issue | 🟠 MajorGuard resource cleanup in
Get-PEMachine'sfinallyblock.If open/read fails before assignment, Line 39 and Line 40 invoke methods on
$null, producing avoidable runtime errors.💡 Proposed fix
function Get-PEMachine($filePath) { + $fileStream = $null + $binaryReader = $null try { $fileStream = [System.IO.FileStream]::new($filePath, [System.IO.FileMode]::Open, [System.IO.FileAccess]::Read) $binaryReader = [System.IO.BinaryReader]::new($fileStream) @@ } catch { return 0 } finally { - $binaryReader.Close() - $fileStream.Close() + if ($null -ne $binaryReader) { $binaryReader.Close() } + if ($null -ne $fileStream) { $fileStream.Close() } } }#!/bin/bash # Verify unguarded Close() calls exist in Get-PEMachine rg -n "function Get-PEMachine|finally|\\$binaryReader\\.Close\\(\\)|\\$fileStream\\.Close\\(\\)" lib/core.ps1 -A8 -B3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/core.ps1` around lines 24 - 42, Get-PEMachine currently always calls $binaryReader.Close() and $fileStream.Close() in the finally block which will throw if those variables were never assigned; update the finally to guard disposal by checking each variable is not $null before calling Close()/Dispose() (or use PowerShell's using / try { } finally { if ($binaryReader -ne $null) { $binaryReader.Close() } if ($fileStream -ne $null) { $fileStream.Close() } }). Reference the function name Get-PEMachine and the symbols $binaryReader.Close() and $fileStream.Close() when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/Scoop-Core.Tests.ps1`:
- Around line 383-423: The Describe block "WoW64 path rewriting in shim" is
Windows-specific but not marked as such; update the Describe invocation
(Describe 'WoW64 path rewriting in shim' -Tag 'Scoop') to include a Windows tag
(for example -Tag 'Scoop','Windows') so the three It tests (the two rewrite
checks and the "does not rewrite" check) are scoped to Windows runners and won't
run on non-Windows CI; alternatively wrap the whole Describe in an OS check to
call Set-ItResult -Skipped on non-Windows, but prefer adding the 'Windows' tag
to the existing Describe to match the review request.
---
Duplicate comments:
In `@lib/core.ps1`:
- Around line 24-42: Get-PEMachine currently always calls $binaryReader.Close()
and $fileStream.Close() in the finally block which will throw if those variables
were never assigned; update the finally to guard disposal by checking each
variable is not $null before calling Close()/Dispose() (or use PowerShell's
using / try { } finally { if ($binaryReader -ne $null) { $binaryReader.Close() }
if ($fileStream -ne $null) { $fileStream.Close() } }). Reference the function
name Get-PEMachine and the symbols $binaryReader.Close() and $fileStream.Close()
when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b2a50196-09b1-4196-abb2-198d11c44f6e
📒 Files selected for processing (3)
CHANGELOG.mdlib/core.ps1test/Scoop-Core.Tests.ps1
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/core.ps1`:
- Around line 934-952: Get-ShimTarget can fail to resolve persisted paths that
contain "\Sysnative\" because Convert-Path is run in 64-bit PowerShell with
-ErrorAction SilentlyContinue; update Get-ShimTarget so that after the initial
Convert-Path attempt (the call using Convert-Path on the shim's stored
path/$target) if it returns $null or empty and the path contains "\Sysnative\",
replace "\Sysnative\" with "\System32\" and retry Convert-Path (or
Test-Path+Get-Item) to produce the normalized target; keep existing behavior for
successful resolves so x86 shim execution still uses Sysnative while 64-bit
callers get a System32 fallback.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/Scoop-Core.Tests.ps1 (1)
383-423: Add a non-x86 guard test and alignSysnativereplacement value.This block validates replacements but doesn’t assert the x86-only gate (
$shim_machine -eq 0x014c). Also, Line 417 uses a literal'Sysnative'instead of the full$sysnativepath used in production logic.♻️ Minimal alignment patch
Describe 'WoW64 path rewriting in shim' -Tag 'Scoop', 'Windows' { + It 'does not rewrite when shim machine is not x86' { + $sysdir = [System.IO.Path]::Combine($env:SystemRoot, 'System32') + $sysnative = [System.IO.Path]::Combine($env:SystemRoot, 'Sysnative') + $rewrote_path = "$sysdir\notepad.exe" + $shim_machine = 0x8664 + + if ([System.Environment]::Is64BitOperatingSystem) { + if ($shim_machine -eq 0x014c -and $rewrote_path -like "$sysdir\*") { + $rewrote_path = $rewrote_path -replace [regex]::Escape($sysdir), $sysnative + } + $rewrote_path | Should -Be "$sysdir\notepad.exe" + } else { + Set-ItResult -Skipped -Because 'not a x64 OS' + } + } + It 'does not rewrite paths outside System32 and SysWOW64' { $sysdir = [System.IO.Path]::Combine($env:SystemRoot, 'System32') + $sysnative = [System.IO.Path]::Combine($env:SystemRoot, 'Sysnative') $syswow = [System.IO.Path]::Combine($env:SystemRoot, 'SysWOW64') $testPath = 'C:\Program Files\test\app.exe' @@ $result = $testPath if ($result -like "$sysdir\*") { - $result = $result -replace [regex]::Escape($sysdir), 'Sysnative' + $result = $result -replace [regex]::Escape($sysdir), $sysnative } elseif ($result -like "$syswow\*") { $result = $result -replace [regex]::Escape($syswow), $sysdir } $result | Should -Be $testPath } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Scoop-Core.Tests.ps1` around lines 383 - 423, Add the missing x86-shim guard and fix the Sysnative replacement: in the tests that assert WoW64 rewriting (the It blocks checking System32→Sysnative and SysWOW64→System32) ensure you check the shim machine value ($shim_machine -eq 0x014c) and skip if not x86 (use Set-ItResult -Skipped -Because 'not an x86 shim'); in the "does not rewrite paths outside System32 and SysWOW64" test replace the literal 'Sysnative' with the actual $sysnative variable so the replacement mirrors production logic and only perform replacements when $shim_machine indicates x86.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/Scoop-Core.Tests.ps1`:
- Around line 383-423: Add the missing x86-shim guard and fix the Sysnative
replacement: in the tests that assert WoW64 rewriting (the It blocks checking
System32→Sysnative and SysWOW64→System32) ensure you check the shim machine
value ($shim_machine -eq 0x014c) and skip if not x86 (use Set-ItResult -Skipped
-Because 'not an x86 shim'); in the "does not rewrite paths outside System32 and
SysWOW64" test replace the literal 'Sysnative' with the actual $sysnative
variable so the replacement mirrors production logic and only perform
replacements when $shim_machine indicates x86.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3be4905b-3eff-4ff9-b311-6858ad6417d6
📒 Files selected for processing (1)
test/Scoop-Core.Tests.ps1
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
1 participant
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.
Description
This PR rewrite system program path if the shim is x86 program.
Therefore, the shim can execute x64 only system program correctly.
Motivation and Context
Closes #6619
How Has This Been Tested?
Add the following
binblock in any manifest, install, then run the shim.Checklist:
developbranch.Summary by CodeRabbit
Bug Fixes
Tests