From: Johannes Sixt <j6t@kdbg•org>
To: Mark Levedahl <mlevedahl@gmail•com>
Cc: git@vger•kernel.org, johannes.schindelin@gmx•de
Subject: Re: [PATCH] gitk - override $PATH search only on Windows
Date: Mon, 31 Mar 2025 19:12:14 +0200 [thread overview]
Message-ID: <58556f57-698d-4f58-bbcf-c752cba00ff7@kdbg.org> (raw)
In-Reply-To: <20250331151213.274691-1-mlevedahl@gmail.com>
Am 31.03.25 um 17:12 schrieb Mark Levedahl:
> Commit 4cbe9e0e2 was written to address problems that result from Tcl's
> documented behavior on Windows where the current working directory and a
> number of Windows system directories are automatically prepended to
> $PATH when searching for executables [1]. This basic Windows behavior
> has resulted in more than one CVE against git for Windows:
> CVE-2023-23618, CVE-2022-41953 are listed on the git for Windows github
> website for the Tcl components of git (gitk, git-gui).
>
> 4cbe9e0e2 is intended to restrict the search to looking only in
> directories given in $PATH and in the given order, which is exactly the
> Tcl behavior documented to exist on non-Windows platforms [1]. Thus,
> this change could have been written to affect only Windows, leaving
> other platforms alone.
>
> However, 4cbe9e0e2 implements the override for all platforms. and
> includes specialized code for Cygwin, copied copied from git-gui prior
> to commit 6d2f9d90 on https://github.com/j6t/git-gui.git), so targets a
I can't find 6d2f9d90 anywhere. Do you have a URL?
> long retired Cygwin port of the Windows Tcl/Tk using Windows pathnames.
> Since 2012, Cygwin uses a Unix/X11 port requiring Unix pathnames,
> meaning 4cbe9e0e2 is incompatible. The patch also induces an infinite
> recursion as _which now invokes the exec wrapper that invokes _which.
> As this is part of git v2.49.0, gitk on Cygwin is broken in that
> release.
>
> Rather than fix the unnecessary override code for Cygwin, let's just
> limit the override of exec/open to Windows, leaving all other platforms
> using their native exec/open as they did prior to 4cbe9e0e2. This patch
> wraps the override code in an "if {[is_Windows]} { ... }" block while
> removing the non-Windows code added in 4cbe9e0e2.
>
> [1] see https://www.tcl-lang.org/man/tcl8.6/TclCmd/exec.htm
Thanks, nicely summarized.
>
> Signed-off-by: Mark Levedahl <mlevedahl@gmail•com>
> ---
> gitk | 146 ++++++++++++++++++++++++-----------------------------------
> 1 file changed, 58 insertions(+), 88 deletions(-)
>
> diff --git a/gitk b/gitk
> index bc9efa1..a101b07 100755
> --- a/gitk
> +++ b/gitk
> @@ -13,13 +13,6 @@ package require Tk
> ##
> ## Enabling platform-specific code paths
>
> -proc is_MacOSX {} {
> - if {[tk windowingsystem] eq {aqua}} {
> - return 1
> - }
> - return 0
> -}
> -
> proc is_Windows {} {
> if {$::tcl_platform(platform) eq {windows}} {
> return 1
> @@ -27,36 +20,16 @@ proc is_Windows {} {
> return 0
> }
>
> -set _iscygwin {}
> -proc is_Cygwin {} {
> - global _iscygwin
> - if {$_iscygwin eq {}} {
> - if {[string match "CYGWIN_*" $::tcl_platform(os)]} {
> - set _iscygwin 1
> - } else {
> - set _iscygwin 0
> - }
> - }
> - return $_iscygwin
> -}
> -
> ######################################################################
> ##
> ## PATH lookup
>
> -set _search_path {}
> -proc _which {what args} {
> - global env _search_exe _search_path
> -
> - if {$_search_path eq {}} {
> - if {[is_Cygwin] && [regexp {^(/|\.:)} $env(PATH)]} {
> - set _search_path [split [exec cygpath \
> - --windows \
> - --path \
> - --absolute \
> - $env(PATH)] {;}]
> - set _search_exe .exe
> - } elseif {[is_Windows]} {
> +if {[is_Windows]} {
> + set _search_path {}
> + proc _which {what args} {
> + global env _search_exe _search_path
> +
> + if {$_search_path eq {}} {
> set gitguidir [file dirname [info script]]
> regsub -all ";" $gitguidir "\\;" gitguidir
> set env(PATH) "$gitguidir;$env(PATH)"
> @@ -65,81 +38,78 @@ proc _which {what args} {
> set _search_path [lsearch -all -inline -not -exact \
> $_search_path ""]
> set _search_exe .exe
> - } else {
> - set _search_path [split $env(PATH) :]
> - set _search_exe {}
> }
> - }
>
> - if {[is_Windows] && [lsearch -exact $args -script] >= 0} {
> - set suffix {}
> - } else {
> - set suffix $_search_exe
> - }
> + if {[lsearch -exact $args -script] >= 0} {
> + set suffix {}
> + } else {
> + set suffix $_search_exe
> + }
Now that this code is only about Windows, _search_exe is always ".exe".
It would be great if we could remove it as well.
>
> - foreach p $_search_path {
> - set p [file join $p $what$suffix]
> - if {[file exists $p]} {
> - return [file normalize $p]
> + foreach p $_search_path {
> + set p [file join $p $what$suffix]
> + if {[file exists $p]} {
> + return [file normalize $p]
> + }
> }
> + return {}
> }
> - return {}
> -}
>
> -proc sanitize_command_line {command_line from_index} {
> - set i $from_index
> - while {$i < [llength $command_line]} {
> - set cmd [lindex $command_line $i]
> - if {[file pathtype $cmd] ne "absolute"} {
> - set fullpath [_which $cmd]
> - if {$fullpath eq ""} {
> - throw {NOT-FOUND} "$cmd not found in PATH"
> + proc sanitize_command_line {command_line from_index} {
> + set i $from_index
> + while {$i < [llength $command_line]} {
> + set cmd [lindex $command_line $i]
> + if {[file pathtype $cmd] ne "absolute"} {
> + set fullpath [_which $cmd]
> + if {$fullpath eq ""} {
> + throw {NOT-FOUND} "$cmd not found in PATH"
> + }
> + lset command_line $i $fullpath
> }
> - lset command_line $i $fullpath
> - }
>
> - # handle piped commands, e.g. `exec A | B`
> - for {incr i} {$i < [llength $command_line]} {incr i} {
> - if {[lindex $command_line $i] eq "|"} {
> - incr i
> - break
> + # handle piped commands, e.g. `exec A | B`
> + for {incr i} {$i < [llength $command_line]} {incr i} {
> + if {[lindex $command_line $i] eq "|"} {
> + incr i
> + break
> + }
> }
> }
> + return $command_line
> }
> - return $command_line
> -}
>
> -# Override `exec` to avoid unsafe PATH lookup
> + # Override `exec` to avoid unsafe PATH lookup
>
> -rename exec real_exec
> + rename exec real_exec
>
> -proc exec {args} {
> - # skip options
> - for {set i 0} {$i < [llength $args]} {incr i} {
> - set arg [lindex $args $i]
> - if {$arg eq "--"} {
> - incr i
> - break
> - }
> - if {[string range $arg 0 0] ne "-"} {
> - break
> + proc exec {args} {
> + # skip options
> + for {set i 0} {$i < [llength $args]} {incr i} {
> + set arg [lindex $args $i]
> + if {$arg eq "--"} {
> + incr i
> + break
> + }
> + if {[string range $arg 0 0] ne "-"} {
> + break
> + }
> }
> + set args [sanitize_command_line $args $i]
> + uplevel 1 real_exec $args
> }
> - set args [sanitize_command_line $args $i]
> - uplevel 1 real_exec $args
> -}
>
> -# Override `open` to avoid unsafe PATH lookup
> + # Override `open` to avoid unsafe PATH lookup
>
> -rename open real_open
> + rename open real_open
>
> -proc open {args} {
> - set arg0 [lindex $args 0]
> - if {[string range $arg0 0 0] eq "|"} {
> - set command_line [string trim [string range $arg0 1 end]]
> - lset args 0 "| [sanitize_command_line $command_line 0]"
> + proc open {args} {
> + set arg0 [lindex $args 0]
> + if {[string range $arg0 0 0] eq "|"} {
> + set command_line [string trim [string range $arg0 1 end]]
> + lset args 0 "| [sanitize_command_line $command_line 0]"
> + }
> + uplevel 1 real_open $args
> }
> - uplevel 1 real_open $args
> }
>
> # End of safe PATH lookup stuff
-- Hannes
next prev parent reply other threads:[~2025-03-31 17:12 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-03-28 12:34 git v2.49.0 - gitk regression on Cygwin Mark Levedahl
2025-03-28 17:30 ` Johannes Sixt
2025-03-29 21:49 ` Mark Levedahl
2025-03-31 15:12 ` [PATCH] gitk - override $PATH search only on Windows Mark Levedahl
2025-03-31 17:12 ` Johannes Sixt [this message]
2025-03-31 19:29 ` Mark Levedahl
2025-04-01 3:00 ` [PATCH v2 0/3] gitk: override PATH " Mark Levedahl
2025-04-01 3:01 ` [PATCH v2 1/3] gitk: override $PATH " Mark Levedahl
2025-04-01 3:01 ` [PATCH v2 2/3] gitk: _search_exe is no longer needed Mark Levedahl
2025-04-01 3:01 ` [PATCH v2 3/3] gitk: limit PATH search to bare executable names Mark Levedahl
2025-04-01 16:10 ` [PATCH v2 0/3] gitk: override PATH search only on Windows Johannes Schindelin
2025-04-01 16:44 ` Mark Levedahl
2025-04-01 16:40 ` Johannes Sixt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=58556f57-698d-4f58-bbcf-c752cba00ff7@kdbg.org \
--to=j6t@kdbg$(echo .)org \
--cc=git@vger$(echo .)kernel.org \
--cc=johannes.schindelin@gmx$(echo .)de \
--cc=mlevedahl@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox