Parse Windows' version correctly in post-install script #506
Conversation
This file contains 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
The following snippet in `post-install.bat` attempts to parse Windows'
major version number from `ver` builtin command output:
@for /F "tokens=4 delims=.[XP " %%i IN ('ver') DO @set ver=%%i
@if 10 LEQ %ver% @...
However, that code has numerous problems:
- `ver` listing varies between major Windows releases (though Git
for Windows' support for these has most likely phased out already)
and may not necessarily yield the same amount of tokens, thus it's
better to perform a two-step tokenization - string, *then* version
- while the output of `ver` hasn't been found to contain 'poisonous'
Batch characters (e.g. <>|&), it's preferred to wrap it in double
quotes to prevent the contents 'escaping' into another command and
use `~` during assignment should it contain double quotes itself
- the code won't run if `ver` encounters an error, yielding an empty
variable **or** inheriting a value from the environment the script
happened to be running in, introducing potential security issues
- numeric comparison will fail should `ver` contain an empty string,
thus the variable needs to be pre-initialized with a numeric value
- a number as the left operand obscures error messages from `if`
This can result in the following, rather cryptic error:
Line 3444: Unable to run post-install scripts:
C:\Program Files\Git> @if 10 LEQ @(
Echoed due to `%ver%` being empty because of the problems listed above.
Fixes git-for-windows/git#1585 and git-for-windows/git#4019; maybe more.
Signed-off-by: Mateusz Kazimierczuk <mataha@users.noreply.github.com>
dscho
approved these changes
Jun 19, 2023
|
/add release note bug Portable Git: The Windows version is now parsed more robustly in the post-install script. The workflow run was started |
github-actions bot
pushed a commit
that referenced
this pull request
Jun 19, 2023
Portable Git: The Windows version is now parsed [more robustly](#506) in the post-install script. Signed-off-by: gitforwindowshelper[bot] <gitforwindowshelper-bot@users.noreply.github.com>
|
Was a mismatched |
@mataha for this repository, it's preferred but not mandatory to use the real name and real email address. For git-for-windows/git, it is mandatory because all patches should be (at least theoretically) upstreamable to the Git project, which requires this. |
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.
The following snippet in
post-install.batattempts to parse Windows' major version number fromverbuiltin command output:However, that code has numerous problems:
verlisting varies between major Windows releases (though Git for Windows' support for these has most likely phased out already) and may not necessarily yield the same amount of tokens, thus it's better to perform a two-step tokenization - string, then versionverhasn't been found to contain 'poisonous' Batch characters (e.g. <>|&), it's preferred to wrap it in double quotes to prevent the contents 'escaping' into another command and use~during assignment should it contain double quotes itselfverencounters an error, yielding an empty variable or inheriting a value from the environment the script happened to be running in, introducing potential security issues (PR note: especially when a buggy AutoRun command is executed in that subshell before)vercontain an empty string, thus the variable needs to be pre-initialized with a numeric valueifThis can result in the following, rather cryptic error:
Echoed due to
%ver%being empty because of the problems listed above.Fixes git-for-windows/git#1585 and git-for-windows/git#4019; maybe more.
Ran into this while looking for clues related to git-for-windows/git#4466...