Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

getgit: Fix user path reg query result potentially getting truncated #558

Merged
merged 4 commits into from
Jun 12, 2024

Conversation

Morilli
Copy link
Contributor

@Morilli Morilli commented Jun 6, 2024

This reg query result for the user path environment variable could potentially get truncated if a space was contained anywhere inside the path variable. This is especially problematic because this incorrect value is then written back, effectively destroying part of the users path.

For example with path containing D:\important\entry;C:\my\very spacey\path:
Before: reg query "HKEY_CURRENT_USER\Environment" //v Path 2>/dev/null | awk 'NR==3 {print $NF}' -> spacey\path

After: reg query "HKEY_CURRENT_USER\Environment" //v Path 2>/dev/null | awk 'NR==3 {print}' | cut -d ' ' -f 13- -> D:\important\entry;C:\my\very spacey\path

@dscho
Copy link
Member

dscho commented Jun 7, 2024

/updpkgsums

dscho added a commit to dscho/git-for-windows-automation that referenced this pull request Jun 7, 2024
The typo in the filename prevented the `/updpkgsums` command in
git-for-windows/build-extra#558 (comment)
from working.

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho
Copy link
Member

dscho commented Jun 7, 2024

image

Oops.

@dscho
Copy link
Member

dscho commented Jun 7, 2024

/updpkgsums

The workflow run was started.

The workflow run was started.

git-extra/getgit Outdated Show resolved Hide resolved
The reg query command used returns a multi-line output that contains the path variable's content in its third line:
    Path    REG_EXPAND_SZ    [actual path variable]
The previous code just returned the last column of that line using awk, which fails to account
for spaces in the path, in which case only the part after the last space was printed.
Now cut is used to skip the first 12 spaces and print every after it, which returns the entire path variable's content unmodified.

Signed-off-by: Moritz Bender <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
git-extra/getgit Outdated
userpath=$(reg query "HKEY_CURRENT_USER\Environment" //v Path 2>/dev/null | awk 'NR==3 {print $NF}')
userpath=$(reg query "HKEY_CURRENT_USER\Environment" //v Path 2>/dev/null | awk 'NR==3 {print}' | cut -d ' ' -f 13-)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the following work, too?

reg query "HKEY_CURRENT_USER\Environment" //v Path | sed -n 's/^ *Path *REG_EXPAND_SZ *//p'

I would have a preference for that if it worked, because:

  1. It avoids spawning two processes, and
  2. It spells out what is expected instead of relying on the magic row number 3 and the magic column number 13.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like that doesn't work all the time because the path variable may be of type REG_SZ or REG_EXPAND_SZ, depending on whether or not variables like %USERPROFILE% are used (see https://learn.microsoft.com/en-us/windows/win32/sysinfo/registry-value-types for registry value types). I assume those are the only valid types for that key that the system accepts, although REG_MULTI_SZ also sounds possible to me. If you prefer the sed approach, reg query "HKEY_CURRENT_USER\Environment" //v Path | sed -n 's/^ *Path *REG\(_EXPAND\)\?_SZ *//p' has worked for both REG_SZ and REG_EXPAND_SZ in my testing.

It's a bit unfortunate that the reg query output is inherently badly formatted for automated reading, but I think the assumptions that have worked so far are safe (value always on line 3, separators are exactly 4 spaces...).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit unfortunate that the reg query output is inherently badly formatted for automated reading, but I think the assumptions that have worked so far are safe (value always on line 3, separators are exactly 4 spaces...).

It might be better to turn this into a PowerShell scriptlet, anyway. Something like this:

userpath="$(powershell -command '(Get-ItemProperty -Path 'HKCU:\Environment' -Name Path).Path')"

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that would be ideal. Given this only runs on windows, powershell should be guaranteed to be available.

The powershell command returns the queried value directly with no additional parsing required.

Signed-off-by: Moritz Bender <[email protected]>
@dscho
Copy link
Member

dscho commented Jun 12, 2024

/updpkgsums

The workflow run was started.

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho
Copy link
Member

dscho commented Jun 12, 2024

/deploy

@dscho
Copy link
Member

dscho commented Jun 12, 2024

/deploy git-extra

The i686/x86_64 and the arm64 workflow runs were started.

@dscho dscho merged commit 636e970 into git-for-windows:main Jun 12, 2024
8 checks passed
@Morilli Morilli deleted the fix-reg-query branch June 12, 2024 18:02
ammyk9 pushed a commit to ammyk9/git-for-windows-automation that referenced this pull request Aug 8, 2024
The typo in the filename prevented the `/updpkgsums` command in
git-for-windows/build-extra#558 (comment)
from working.

Signed-off-by: Johannes Schindelin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants