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@xxxxxxxxx> > --- > 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
