From 4774c704d20e50ad710f65756099c3eedbfbe789 Mon Sep 17 00:00:00 2001 From: Mark Levedahl Date: Wed, 20 Sep 2023 17:56:14 -0400 Subject: [PATCH 01/15] git-gui: remove Tcl 8.4 workaround on 2>@1 redirection Since b792230 ("git-gui: Show a progress meter for checking out files", 2007-07-08), git-gui includes a workaround for Tcl that does not support using 2>@1 to redirect stderr to stdout. Tcl added such support in 8.4.7, released in 2004, and this is fully supported in all 8.5 releases. As git-gui has a hard-coded requirement for Tcl >= 8.5, the workaround is no longer needed. Delete it. Signed-off-by: Mark Levedahl Signed-off-by: Johannes Sixt Signed-off-by: Taylor Blau --- git-gui.sh | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/git-gui.sh b/git-gui.sh index 3e5907a460..ca1362aa19 100755 --- a/git-gui.sh +++ b/git-gui.sh @@ -584,24 +584,9 @@ proc git {args} { proc _open_stdout_stderr {cmd} { _trace_exec $cmd if {[catch { - set fd [open [concat [list | ] $cmd] r] - } err]} { - if { [lindex $cmd end] eq {2>@1} - && $err eq {can not find channel named "1"} - } { - # Older versions of Tcl 8.4 don't have this 2>@1 IO - # redirect operator. Fallback to |& cat for those. - # The command was not actually started, so its safe - # to try to start it a second time. - # - set fd [open [concat \ - [list | ] \ - [lrange $cmd 0 end-1] \ - [list |& cat] \ - ] r] - } else { - error $err - } + set fd [open [concat [list | ] $cmd] r] + } err]} { + error $err } fconfigure $fd -eofchar {} return $fd From f9a2e8a38f524c04fc493548303488dc180b25bd Mon Sep 17 00:00:00 2001 From: Mark Levedahl Date: Fri, 2 May 2025 11:39:55 -0400 Subject: [PATCH 02/15] git-gui: remove HEAD detachment implementation for git < 1.5.3 git-gui provides an implementation to detach HEAD on Git versions prior to 1.5.3. Nobody should be using such an old version anymore. (Moreover, since 0730a5a3a, git-gui requires git v2.36 or later). Keep only the code for modern Git. Signed-off-by: Mark Levedahl [j6t: message tweaked] Signed-off-by: Johannes Sixt Signed-off-by: Taylor Blau --- lib/checkout_op.tcl | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/lib/checkout_op.tcl b/lib/checkout_op.tcl index 21ea768d80..5f7011078a 100644 --- a/lib/checkout_op.tcl +++ b/lib/checkout_op.tcl @@ -510,18 +510,8 @@ method _update_repo_state {} { delete_this } -git-version proc _detach_HEAD {log new} { - >= 1.5.3 { - git update-ref --no-deref -m $log HEAD $new - } - default { - set p [gitdir HEAD] - file delete $p - set fd [open $p w] - fconfigure $fd -translation lf -encoding utf-8 - puts $fd $new - close $fd - } +proc _detach_HEAD {log new} { + git update-ref --no-deref -m $log HEAD $new } method _confirm_reset {cur} { From 8255167b26003767b0ab50f498ffec33f80c2ef2 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Sat, 3 May 2025 13:37:35 +0200 Subject: [PATCH 03/15] git-gui: remove git config --list handling for git < 1.5.3 git-gui uses `git config --null --list` to parse configuration. Git versions prior to 1.5.3 do not have --null and need different treatment. Nobody should be using such an old version anymore. (Moreover, since 0730a5a3a, git-gui requires git v2.36 or later). Keep only the code for modern Git. Signed-off-by: Johannes Sixt Signed-off-by: Taylor Blau --- git-gui.sh | 67 ++++++++++++++++++------------------------------------ 1 file changed, 22 insertions(+), 45 deletions(-) diff --git a/git-gui.sh b/git-gui.sh index ca1362aa19..2e325b042a 100755 --- a/git-gui.sh +++ b/git-gui.sh @@ -1083,53 +1083,30 @@ unset -nocomplain idx fd ## ## config file parsing -git-version proc _parse_config {arr_name args} { - >= 1.5.3 { - upvar $arr_name arr - array unset arr - set buf {} - catch { - set fd_rc [eval \ - [list git_read config] \ - $args \ - [list --null --list]] - fconfigure $fd_rc -translation binary -encoding utf-8 - set buf [read $fd_rc] - close $fd_rc - } - foreach line [split $buf "\0"] { - if {[regexp {^([^\n]+)\n(.*)$} $line line name value]} { - if {[is_many_config $name]} { - lappend arr($name) $value - } else { - set arr($name) $value - } - } elseif {[regexp {^([^\n]+)$} $line line name]} { - # no value given, but interpreting them as - # boolean will be handled as true - set arr($name) {} - } - } +proc _parse_config {arr_name args} { + upvar $arr_name arr + array unset arr + set buf {} + catch { + set fd_rc [eval \ + [list git_read config] \ + $args \ + [list --null --list]] + fconfigure $fd_rc -translation binary -encoding utf-8 + set buf [read $fd_rc] + close $fd_rc } - default { - upvar $arr_name arr - array unset arr - catch { - set fd_rc [eval [list git_read config --list] $args] - while {[gets $fd_rc line] >= 0} { - if {[regexp {^([^=]+)=(.*)$} $line line name value]} { - if {[is_many_config $name]} { - lappend arr($name) $value - } else { - set arr($name) $value - } - } elseif {[regexp {^([^=]+)$} $line line name]} { - # no value given, but interpreting them as - # boolean will be handled as true - set arr($name) {} - } + foreach line [split $buf "\0"] { + if {[regexp {^([^\n]+)\n(.*)$} $line line name value]} { + if {[is_many_config $name]} { + lappend arr($name) $value + } else { + set arr($name) $value } - close $fd_rc + } elseif {[regexp {^([^\n]+)$} $line line name]} { + # no value given, but interpreting them as + # boolean will be handled as true + set arr($name) {} } } } From c2e8904258544f3d79dc4e96d1269c0ad8124db3 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Mon, 21 Apr 2025 17:07:10 +0200 Subject: [PATCH 04/15] git-gui: treat file names beginning with "|" as relative paths The Tcl 'open' function has a very wide interface. It can open files as well as pipes to external processes. The difference is made only by the first character of the file name: if it is "|", a process is spawned. We have a number of calls of Tcl 'open' that take a file name from the environment in which Git GUI is running. Be prepared that insane values are injected. In particular, when we intend to open a file, do not take a file name that happens to begin with "|" as a request to run a process. Signed-off-by: Johannes Sixt Signed-off-by: Taylor Blau --- git-gui.sh | 36 ++++++++++++++++++++++++------------ lib/blame.tcl | 2 +- lib/choose_repository.tcl | 14 +++++++------- lib/choose_rev.tcl | 2 +- lib/commit.tcl | 4 ++-- lib/diff.tcl | 2 +- lib/merge.tcl | 2 +- lib/mergetool.tcl | 2 +- lib/remote.tcl | 6 +++--- lib/shortcut.tcl | 4 ++-- lib/sshkey.tcl | 2 +- 11 files changed, 44 insertions(+), 32 deletions(-) diff --git a/git-gui.sh b/git-gui.sh index 2e325b042a..52b463c45f 100755 --- a/git-gui.sh +++ b/git-gui.sh @@ -170,6 +170,18 @@ proc open {args} { uplevel 1 real_open $args } +# Wrap open to sanitize arguments + +proc safe_open_file {filename flags} { + # a file name starting with "|" would attempt to run a process + # but such a file name must be treated as a relative path + # hide the "|" behind "./" + if {[string index $filename 0] eq "|"} { + set filename [file join . $filename] + } + open $filename $flags +} + ###################################################################### ## ## locate our library @@ -494,7 +506,7 @@ proc _git_cmd {name} { # Tcl on Windows doesn't know it. # set p [gitexec git-$name] - set f [open $p r] + set f [safe_open_file $p r] set s [gets $f] close $f @@ -527,7 +539,7 @@ proc _git_cmd {name} { # Test a file for a hashbang to identify executable scripts on Windows. proc is_shellscript {filename} { if {![file exists $filename]} {return 0} - set f [open $filename r] + set f [safe_open_file $filename r] fconfigure $f -encoding binary set magic [read $f 2] close $f @@ -683,7 +695,7 @@ proc sq {value} { proc load_current_branch {} { global current_branch is_detached - set fd [open [gitdir HEAD] r] + set fd [safe_open_file [gitdir HEAD] r] fconfigure $fd -translation binary -encoding utf-8 if {[gets $fd ref] < 1} { set ref {} @@ -1045,7 +1057,7 @@ You are using [git-version]: ## configure our library set idx [file join $oguilib tclIndex] -if {[catch {set fd [open $idx r]} err]} { +if {[catch {set fd [safe_open_file $idx r]} err]} { catch {wm withdraw .} tk_messageBox \ -icon error \ @@ -1382,7 +1394,7 @@ proc repository_state {ctvar hdvar mhvar} { set merge_head [gitdir MERGE_HEAD] if {[file exists $merge_head]} { set ct merge - set fd_mh [open $merge_head r] + set fd_mh [safe_open_file $merge_head r] while {[gets $fd_mh line] >= 0} { lappend mh $line } @@ -1530,7 +1542,7 @@ proc load_message {file {encoding {}}} { set f [gitdir $file] if {[file isfile $f]} { - if {[catch {set fd [open $f r]}]} { + if {[catch {set fd [safe_open_file $f r]}]} { return 0 } fconfigure $fd -eofchar {} @@ -1554,23 +1566,23 @@ proc run_prepare_commit_msg_hook {} { # it will be .git/MERGE_MSG (merge), .git/SQUASH_MSG (squash), or an # empty file but existent file. - set fd_pcm [open [gitdir PREPARE_COMMIT_MSG] a] + set fd_pcm [safe_open_file [gitdir PREPARE_COMMIT_MSG] a] if {[file isfile [gitdir MERGE_MSG]]} { set pcm_source "merge" - set fd_mm [open [gitdir MERGE_MSG] r] + set fd_mm [safe_open_file [gitdir MERGE_MSG] r] fconfigure $fd_mm -encoding utf-8 puts -nonewline $fd_pcm [read $fd_mm] close $fd_mm } elseif {[file isfile [gitdir SQUASH_MSG]]} { set pcm_source "squash" - set fd_sm [open [gitdir SQUASH_MSG] r] + set fd_sm [safe_open_file [gitdir SQUASH_MSG] r] fconfigure $fd_sm -encoding utf-8 puts -nonewline $fd_pcm [read $fd_sm] close $fd_sm } elseif {[file isfile [get_config commit.template]]} { set pcm_source "template" - set fd_sm [open [get_config commit.template] r] + set fd_sm [safe_open_file [get_config commit.template] r] fconfigure $fd_sm -encoding utf-8 puts -nonewline $fd_pcm [read $fd_sm] close $fd_sm @@ -2271,7 +2283,7 @@ proc do_quit {{rc {1}}} { if {![string match amend* $commit_type] && $msg ne {}} { catch { - set fd [open $save w] + set fd [safe_open_file $save w] fconfigure $fd -encoding utf-8 puts -nonewline $fd $msg close $fd @@ -4032,7 +4044,7 @@ if {[winfo exists $ui_comm]} { } } elseif {$m} { catch { - set fd [open [gitdir GITGUI_BCK] w] + set fd [safe_open_file [gitdir GITGUI_BCK] w] fconfigure $fd -encoding utf-8 puts -nonewline $fd $msg close $fd diff --git a/lib/blame.tcl b/lib/blame.tcl index 8441e109be..e70a079fa7 100644 --- a/lib/blame.tcl +++ b/lib/blame.tcl @@ -481,7 +481,7 @@ method _load {jump} { if {$do_textconv ne 0} { set fd [open_cmd_pipe $textconv $path] } else { - set fd [open $path r] + set fd [safe_open_file $path r] } fconfigure $fd -eofchar {} } else { diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl index d23abedcb3..ef7ad7c175 100644 --- a/lib/choose_repository.tcl +++ b/lib/choose_repository.tcl @@ -641,8 +641,8 @@ method _do_clone2 {} { set pwd [pwd] if {[catch { file mkdir [gitdir objects info] - set f_in [open [file join $objdir info alternates] r] - set f_cp [open [gitdir objects info alternates] w] + set f_in [safe_open_file [file join $objdir info alternates] r] + set f_cp [safe_open_file [gitdir objects info alternates] w] fconfigure $f_in -translation binary -encoding binary fconfigure $f_cp -translation binary -encoding binary cd $objdir @@ -727,7 +727,7 @@ method _do_clone2 {} { [cb _do_clone_tags] } shared { - set fd [open [gitdir objects info alternates] w] + set fd [safe_open_file [gitdir objects info alternates] w] fconfigure $fd -translation binary puts $fd $objdir close $fd @@ -760,8 +760,8 @@ method _copy_files {objdir tocopy} { } foreach p $tocopy { if {[catch { - set f_in [open [file join $objdir $p] r] - set f_cp [open [file join .git objects $p] w] + set f_in [safe_open_file [file join $objdir $p] r] + set f_cp [safe_open_file [file join .git objects $p] w] fconfigure $f_in -translation binary -encoding binary fconfigure $f_cp -translation binary -encoding binary @@ -823,7 +823,7 @@ method _clone_refs {} { {--format=list %(refname) %(objectname) %(*objectname)}] cd $pwd - set fd [open [gitdir packed-refs] w] + set fd [safe_open_file [gitdir packed-refs] w] fconfigure $fd -translation binary puts $fd "# pack-refs with: peeled" while {[gets $fd_in line] >= 0} { @@ -877,7 +877,7 @@ method _do_clone_full_end {ok} { set HEAD {} if {[file exists [gitdir FETCH_HEAD]]} { - set fd [open [gitdir FETCH_HEAD] r] + set fd [safe_open_file [gitdir FETCH_HEAD] r] while {[gets $fd line] >= 0} { if {[regexp "^(.{40})\t\t" $line line HEAD]} { break diff --git a/lib/choose_rev.tcl b/lib/choose_rev.tcl index 6dae7937d5..7cf9e160fa 100644 --- a/lib/choose_rev.tcl +++ b/lib/choose_rev.tcl @@ -579,7 +579,7 @@ method _reflog_last {name} { set last {} if {[catch {set last [file mtime [gitdir $name]]}] - && ![catch {set g [open [gitdir logs $name] r]}]} { + && ![catch {set g [safe_open_file [gitdir logs $name] r]}]} { fconfigure $g -translation binary while {[gets $g line] >= 0} { if {[regexp {> ([1-9][0-9]*) } $line line when]} { diff --git a/lib/commit.tcl b/lib/commit.tcl index 11379f8ad3..8d135845a5 100644 --- a/lib/commit.tcl +++ b/lib/commit.tcl @@ -225,7 +225,7 @@ A good commit message has the following format: # -- Build the message file. # set msg_p [gitdir GITGUI_EDITMSG] - set msg_wt [open $msg_p w] + set msg_wt [safe_open_file $msg_p w] fconfigure $msg_wt -translation lf setup_commit_encoding $msg_wt puts $msg_wt $msg @@ -409,7 +409,7 @@ A rescan will be automatically started now. if {$commit_type ne {normal}} { append reflogm " ($commit_type)" } - set msg_fd [open $msg_p r] + set msg_fd [safe_open_file $msg_p r] setup_commit_encoding $msg_fd 1 gets $msg_fd subject close $msg_fd diff --git a/lib/diff.tcl b/lib/diff.tcl index 871ad488c2..f089fdc46b 100644 --- a/lib/diff.tcl +++ b/lib/diff.tcl @@ -202,7 +202,7 @@ proc show_other_diff {path w m cont_info} { set sz [string length $content] } file { - set fd [open $path r] + set fd [safe_open_file $path r] fconfigure $fd \ -eofchar {} \ -encoding [get_path_encoding $path] diff --git a/lib/merge.tcl b/lib/merge.tcl index 664803cf3f..897dc2a286 100644 --- a/lib/merge.tcl +++ b/lib/merge.tcl @@ -93,7 +93,7 @@ method _start {} { set spec [$w_rev get_tracking_branch] set cmit [$w_rev get_commit] - set fh [open [gitdir FETCH_HEAD] w] + set fh [safe_open_file [gitdir FETCH_HEAD] w] fconfigure $fh -translation lf if {$spec eq {}} { set remote . diff --git a/lib/mergetool.tcl b/lib/mergetool.tcl index e688b016ef..f2fa439305 100644 --- a/lib/mergetool.tcl +++ b/lib/mergetool.tcl @@ -293,7 +293,7 @@ proc merge_tool_get_stages {target stages} { foreach fname $stages { if {$merge_stages($i) eq {}} { file delete $fname - catch { close [open $fname w] } + catch { close [safe_open_file $fname w] } } else { # A hack to support autocrlf properly git checkout-index -f --stage=$i -- $target diff --git a/lib/remote.tcl b/lib/remote.tcl index ef77ed7399..a733a0f36e 100644 --- a/lib/remote.tcl +++ b/lib/remote.tcl @@ -75,7 +75,7 @@ proc load_all_remotes {} { foreach name $all_remotes { catch { - set fd [open [file join $rm_dir $name] r] + set fd [safe_open_file [file join $rm_dir $name] r] while {[gets $fd line] >= 0} { if {[regexp {^URL:[ ]*(.+)$} $line line url]} { set remote_url($name) $url @@ -145,7 +145,7 @@ proc add_fetch_entry {r} { } } else { catch { - set fd [open [gitdir remotes $r] r] + set fd [safe_open_file [gitdir remotes $r] r] while {[gets $fd n] >= 0} { if {[regexp {^Pull:[ \t]*([^:]+):} $n]} { set enable 1 @@ -182,7 +182,7 @@ proc add_push_entry {r} { } } else { catch { - set fd [open [gitdir remotes $r] r] + set fd [safe_open_file [gitdir remotes $r] r] while {[gets $fd n] >= 0} { if {[regexp {^Push:[ \t]*([^:]+):} $n]} { set enable 1 diff --git a/lib/shortcut.tcl b/lib/shortcut.tcl index 674a41f5e0..9be79b2e89 100644 --- a/lib/shortcut.tcl +++ b/lib/shortcut.tcl @@ -83,7 +83,7 @@ proc do_macosx_app {} { file mkdir $MacOS - set fd [open [file join $Contents Info.plist] w] + set fd [safe_open_file [file join $Contents Info.plist] w] puts $fd { @@ -108,7 +108,7 @@ proc do_macosx_app {} { } close $fd - set fd [open $exe w] + set fd [safe_open_file $exe w] puts $fd "#!/bin/sh" foreach name [lsort [array names env]] { set value $env($name) diff --git a/lib/sshkey.tcl b/lib/sshkey.tcl index 589ff8f78a..2e006cb8ca 100644 --- a/lib/sshkey.tcl +++ b/lib/sshkey.tcl @@ -7,7 +7,7 @@ proc find_ssh_key {} { ~/.ssh/id_rsa.pub ~/.ssh/identity.pub } { if {[file exists $name]} { - set fh [open $name r] + set fh [safe_open_file $name r] set cont [read $fh] close $fh return [list $name $cont] From 4f3e0a4bcef2c6caff68f96137d9914c5f2f98c2 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Mon, 21 Apr 2025 18:14:54 +0200 Subject: [PATCH 05/15] git-gui: sanitize 'exec' arguments: simple cases Tcl 'exec' assigns special meaning to its argument when they begin with redirection, pipe or background operator. There are a number of invocations of 'exec' which construct arguments that are taken from the Git repository or a user input. However, when file names or ref names are taken from the repository, it is possible to find names that have these special forms. They must not be interpreted by 'exec' lest it redirects input or output, or attempts to build a pipeline using a command name controlled by the repository. Introduce a helper function that identifies such arguments and prepends "./" to force such a name to be regarded as a relative file name. Convert those 'exec' calls where the arguments can simply be packed into a list. Note that most commands containing the word 'exec' route through console::exec or console::chain, which we will treat in another commit. Signed-off-by: Johannes Sixt Signed-off-by: Taylor Blau --- git-gui.sh | 42 ++++++++++++++++++++++++++++++++++++------ lib/diff.tcl | 2 +- lib/shortcut.tcl | 8 ++++---- lib/win32.tcl | 9 +++++---- 4 files changed, 46 insertions(+), 15 deletions(-) diff --git a/git-gui.sh b/git-gui.sh index 52b463c45f..a6be30e295 100755 --- a/git-gui.sh +++ b/git-gui.sh @@ -170,7 +170,35 @@ proc open {args} { uplevel 1 real_open $args } -# Wrap open to sanitize arguments +# Wrap exec/open to sanitize arguments + +# unsafe arguments begin with redirections or the pipe or background operators +proc is_arg_unsafe {arg} { + regexp {^([<|>&]|2>)} $arg +} + +proc make_arg_safe {arg} { + if {[is_arg_unsafe $arg]} { + set arg [file join . $arg] + } + return $arg +} + +proc make_arglist_safe {arglist} { + set res {} + foreach arg $arglist { + lappend res [make_arg_safe $arg] + } + return $res +} + +# executes one command +# no redirections or pipelines are possible +# cmd is a list that specifies the command and its arguments +# calls `exec` and returns its value +proc safe_exec {cmd} { + eval exec [make_arglist_safe $cmd] +} proc safe_open_file {filename flags} { # a file name starting with "|" would attempt to run a process @@ -182,6 +210,8 @@ proc safe_open_file {filename flags} { open $filename $flags } +# End exec/open wrappers + ###################################################################### ## ## locate our library @@ -282,11 +312,11 @@ unset oguimsg if {[tk windowingsystem] eq "aqua"} { catch { - exec osascript -e [format { + safe_exec [list osascript -e [format { tell application "System Events" set frontmost of processes whose unix id is %d to true end tell - } [pid]] + } [pid]]] } } @@ -571,7 +601,7 @@ proc _lappend_nice {cmd_var} { if {![info exists _nice]} { set _nice [_which nice] - if {[catch {exec $_nice git version}]} { + if {[catch {safe_exec [list $_nice git version]}]} { set _nice {} } elseif {[is_Windows] && [file dirname $_nice] ne [file dirname $::_git]} { set _nice {} @@ -667,9 +697,9 @@ proc kill_file_process {fd} { catch { if {[is_Windows]} { - exec taskkill /pid $process + safe_exec [list taskkill /pid $process] } else { - exec kill $process + safe_exec [list kill $process] } } } diff --git a/lib/diff.tcl b/lib/diff.tcl index f089fdc46b..cfa8a414d3 100644 --- a/lib/diff.tcl +++ b/lib/diff.tcl @@ -226,7 +226,7 @@ proc show_other_diff {path w m cont_info} { $ui_diff insert end \ "* [mc "Git Repository (subproject)"]\n" \ d_info - } elseif {![catch {set type [exec file $path]}]} { + } elseif {![catch {set type [safe_exec [list file $path]]}]} { set n [string length $path] if {[string equal -length $n $path $type]} { set type [string range $type $n end] diff --git a/lib/shortcut.tcl b/lib/shortcut.tcl index 9be79b2e89..d437ea6933 100644 --- a/lib/shortcut.tcl +++ b/lib/shortcut.tcl @@ -30,8 +30,8 @@ proc do_cygwin_shortcut {} { global argv0 _gitworktree oguilib if {[catch { - set desktop [exec cygpath \ - --desktop] + set desktop [safe_exec [list cygpath \ + --desktop]] }]} { set desktop . } @@ -50,14 +50,14 @@ proc do_cygwin_shortcut {} { "CHERE_INVOKING=1 \ source /etc/profile; \ git gui"} - exec /bin/mkshortcut.exe \ + safe_exec [list /bin/mkshortcut.exe \ --arguments $shargs \ --desc "git-gui on $repodir" \ --icon $oguilib/git-gui.ico \ --name $fn \ --show min \ --workingdir $repodir \ - /bin/sh.exe + /bin/sh.exe] } err]} { error_popup [strcat [mc "Cannot write shortcut:"] "\n\n$err"] } diff --git a/lib/win32.tcl b/lib/win32.tcl index db91ab84a5..3aedae2f13 100644 --- a/lib/win32.tcl +++ b/lib/win32.tcl @@ -2,11 +2,11 @@ # Copyright (C) 2007 Shawn Pearce proc win32_read_lnk {lnk_path} { - return [exec cscript.exe \ + return [safe_exec [list cscript.exe \ /E:jscript \ /nologo \ [file join $::oguilib win32_shortcut.js] \ - $lnk_path] + $lnk_path]] } proc win32_create_lnk {lnk_path lnk_exec lnk_dir} { @@ -15,12 +15,13 @@ proc win32_create_lnk {lnk_path lnk_exec lnk_dir} { set lnk_args [lrange $lnk_exec 1 end] set lnk_exec [lindex $lnk_exec 0] - eval [list exec wscript.exe \ + set cmd [list wscript.exe \ /E:jscript \ /nologo \ [file nativename [file join $oguilib win32_shortcut.js]] \ $lnk_path \ [file nativename [file join $oguilib git-gui.ico]] \ $lnk_dir \ - $lnk_exec] $lnk_args + $lnk_exec] + safe_exec [concat $cmd $lnk_args] } From e883ceb1222c29f8a2325e1b4c2778b5259a3725 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Sat, 26 Apr 2025 18:46:06 +0200 Subject: [PATCH 06/15] git-gui: sanitize 'exec' arguments: background As in the previous commits, introduce a function that sanitizes arguments intended for the process, but runs the process in the background. Convert 'exec' calls to use this new function. Signed-off-by: Johannes Sixt Signed-off-by: Taylor Blau --- git-gui.sh | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/git-gui.sh b/git-gui.sh index a6be30e295..6ecddfdd0a 100755 --- a/git-gui.sh +++ b/git-gui.sh @@ -200,6 +200,14 @@ proc safe_exec {cmd} { eval exec [make_arglist_safe $cmd] } +# executes one command in the background +# no redirections or pipelines are possible +# cmd is a list that specifies the command and its arguments +# calls `exec` and returns its value +proc safe_exec_bg {cmd} { + eval exec [make_arglist_safe $cmd] & +} + proc safe_open_file {filename flags} { # a file name starting with "|" would attempt to run a process # but such a file name must be treated as a relative path @@ -2202,7 +2210,7 @@ proc do_gitk {revs {is_submodule false}} { unset env(GIT_DIR) unset env(GIT_WORK_TREE) } - eval exec $cmd $revs "--" "--" & + safe_exec_bg [concat $cmd $revs "--" "--"] set env(GIT_DIR) $_gitdir set env(GIT_WORK_TREE) $_gitworktree @@ -2239,7 +2247,7 @@ proc do_git_gui {} { set pwd [pwd] cd $current_diff_path - eval exec $exe gui & + safe_exec_bg [concat $exe gui] set env(GIT_DIR) $_gitdir set env(GIT_WORK_TREE) $_gitworktree @@ -2270,16 +2278,18 @@ proc get_explorer {} { proc do_explore {} { global _gitworktree - set explorer [get_explorer] - eval exec $explorer [list [file nativename $_gitworktree]] & + set cmd [get_explorer] + lappend cmd [file nativename $_gitworktree] + safe_exec_bg $cmd } # Open file relative to the working tree by the default associated app. proc do_file_open {file} { global _gitworktree - set explorer [get_explorer] + set cmd [get_explorer] set full_file_path [file join $_gitworktree $file] - exec $explorer [file nativename $full_file_path] & + lappend cmd [file nativename $full_file_path] + safe_exec_bg $cmd } set is_quitting 0 @@ -2761,13 +2771,13 @@ if {[is_Windows]} { regsub "/mingw../libexec/git-core/git-gui$" \ $normalized "/git-bash.exe" cmdLine if {$cmdLine != $normalized && [file exists $cmdLine]} { - set cmdLine [list "Git Bash" $cmdLine &] + set cmdLine [list "Git Bash" $cmdLine] } else { - set cmdLine [list "Git Bash" bash --login -l &] + set cmdLine [list "Git Bash" bash --login -l] } .mbar.repository add command \ -label [mc "Git Bash"] \ - -command {eval exec [auto_execok start] $cmdLine} + -command {safe_exec_bg [concat [list [auto_execok start]] $cmdLine]} } if {[is_Windows] || ![is_bare]} { From 23ba43256b421c322af9b99150fb324575175bb0 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Sat, 3 May 2025 11:52:35 +0200 Subject: [PATCH 07/15] git-gui: remove option --stderr from git_read Some callers of git_read want to redirect stderr of the invoked command to stdout. The function offers option --stderr for this purpose. However, the option only appends 2>@1 to the commands. The callers can do that themselves. In lib/console.tcl we even have a caller that already knew implictly what --stderr does behind the scenes. This is a preparation for a later change where we want to make git_read non-variadic. Then it cannot have optional leading arguments. Signed-off-by: Johannes Sixt Signed-off-by: Taylor Blau --- git-gui.sh | 4 ---- lib/checkout_op.tcl | 3 ++- lib/choose_repository.tcl | 3 ++- lib/console.tcl | 4 ++-- lib/merge.tcl | 2 +- 5 files changed, 7 insertions(+), 9 deletions(-) diff --git a/git-gui.sh b/git-gui.sh index 6ecddfdd0a..890a172fc4 100755 --- a/git-gui.sh +++ b/git-gui.sh @@ -651,10 +651,6 @@ proc git_read {args} { _lappend_nice opt } - --stderr { - lappend args 2>@1 - } - default { break } diff --git a/lib/checkout_op.tcl b/lib/checkout_op.tcl index 5f7011078a..31992f461b 100644 --- a/lib/checkout_op.tcl +++ b/lib/checkout_op.tcl @@ -345,13 +345,14 @@ method _readtree {} { [mc "Updating working directory to '%s'..." [_name $this]] \ [mc "files checked out"]] - set fd [git_read --stderr read-tree \ + set fd [git_read read-tree \ -m \ -u \ -v \ --exclude-per-directory=.gitignore \ $HEAD \ $new_hash \ + 2>@1 \ ] fconfigure $fd -blocking 0 -translation binary fileevent $fd readable [cb _readtree_wait $fd $status_bar_operation] diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl index ef7ad7c175..7bd738e51e 100644 --- a/lib/choose_repository.tcl +++ b/lib/choose_repository.tcl @@ -953,12 +953,13 @@ method _do_clone_checkout {HEAD} { [mc "files"]] set readtree_err {} - set fd [git_read --stderr read-tree \ + set fd [git_read read-tree \ -m \ -u \ -v \ HEAD \ HEAD \ + 2>@1 \ ] fconfigure $fd -blocking 0 -translation binary fileevent $fd readable [cb _readtree_wait $fd] diff --git a/lib/console.tcl b/lib/console.tcl index bb6b9c889e..c7f20b87cb 100644 --- a/lib/console.tcl +++ b/lib/console.tcl @@ -91,10 +91,10 @@ method _init {} { } method exec {cmd {after {}}} { + lappend cmd 2>@1 if {[lindex $cmd 0] eq {git}} { - set fd_f [eval git_read --stderr [lrange $cmd 1 end]] + set fd_f [eval git_read [lrange $cmd 1 end]] } else { - lappend cmd 2>@1 set fd_f [_open_stdout_stderr $cmd] } fconfigure $fd_f -blocking 0 -translation binary diff --git a/lib/merge.tcl b/lib/merge.tcl index 897dc2a286..e97c91eab6 100644 --- a/lib/merge.tcl +++ b/lib/merge.tcl @@ -239,7 +239,7 @@ Continue with resetting the current changes?"] } if {[ask_popup $op_question] eq {yes}} { - set fd [git_read --stderr read-tree --reset -u -v HEAD] + set fd [git_read read-tree --reset -u -v HEAD 2>@1] fconfigure $fd -blocking 0 -translation binary set status_bar_operation [$::main_status \ start \ From aa42e87ef4ee9d84bd2fdb5e56de2ac2b61575d9 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Sat, 3 May 2025 13:11:21 +0200 Subject: [PATCH 08/15] git-gui: break out a separate function git_read_nice There are two callers of git_read that request special treatment using option --nice. Rewrite them to call a new function git_read_nice that does the special treatment. Now we can remove all option treatment from git_read. git_write has the same capability, but there are no callers that request --nice. Remove the feature without substitution. This is a preparation for a later change where we want to make git_read and friends non-variadic. Then it cannot have optional arguments. Signed-off-by: Johannes Sixt Signed-off-by: Taylor Blau --- git-gui.sh | 43 ++++++++++--------------------------------- lib/blame.tcl | 2 +- lib/diff.tcl | 2 +- 3 files changed, 12 insertions(+), 35 deletions(-) diff --git a/git-gui.sh b/git-gui.sh index 890a172fc4..28113220af 100755 --- a/git-gui.sh +++ b/git-gui.sh @@ -643,22 +643,16 @@ proc _open_stdout_stderr {cmd} { } proc git_read {args} { + set cmdp [_git_cmd [lindex $args 0]] + set args [lrange $args 1 end] + + return [_open_stdout_stderr [concat $cmdp $args]] +} + +proc git_read_nice {args} { set opt [list] - while {1} { - switch -- [lindex $args 0] { - --nice { - _lappend_nice opt - } - - default { - break - } - - } - - set args [lrange $args 1 end] - } + _lappend_nice opt set cmdp [_git_cmd [lindex $args 0]] set args [lrange $args 1 end] @@ -667,28 +661,11 @@ proc git_read {args} { } proc git_write {args} { - set opt [list] - - while {1} { - switch -- [lindex $args 0] { - --nice { - _lappend_nice opt - } - - default { - break - } - - } - - set args [lrange $args 1 end] - } - set cmdp [_git_cmd [lindex $args 0]] set args [lrange $args 1 end] - _trace_exec [concat $opt $cmdp $args] - return [open [concat [list | ] $opt $cmdp $args] w] + _trace_exec [concat $cmdp $args] + return [open [concat [list | ] $cmdp $args] w] } proc githook_read {hook_name args} { diff --git a/lib/blame.tcl b/lib/blame.tcl index e70a079fa7..ceb2330266 100644 --- a/lib/blame.tcl +++ b/lib/blame.tcl @@ -617,7 +617,7 @@ method _exec_blame {cur_w cur_d options cur_s} { } lappend options -- $path - set fd [eval git_read --nice blame $options] + set fd [eval git_read_nice blame $options] fconfigure $fd -blocking 0 -translation lf -encoding utf-8 fileevent $fd readable [cb _read_blame $fd $cur_w $cur_d] set current_fd $fd diff --git a/lib/diff.tcl b/lib/diff.tcl index cfa8a414d3..ce1bad6900 100644 --- a/lib/diff.tcl +++ b/lib/diff.tcl @@ -338,7 +338,7 @@ proc start_show_diff {cont_info {add_opts {}}} { } } - if {[catch {set fd [eval git_read --nice $cmd]} err]} { + if {[catch {set fd [eval git_read_nice $cmd]} err]} { set diff_active 0 unlock_index ui_status [mc "Unable to display %s" [escape_path $path]] From 074c2b9d7c4b1201f261263f011074c733a85d38 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Sat, 3 May 2025 19:21:53 +0200 Subject: [PATCH 09/15] git-gui: use git_read in githook_read 0730a5a3a5e6 ("git-gui - use git-hook, honor core.hooksPath", 2023-09-17) rewrote githook_read to use `git hook` to run a hook script. The code that was replaced discovered the hook script file manually and invoked it using function _open_stdout_stderr. After the rewrite, this function is still invoked, but it calls into `git` instead of the hook scripts. Notice though, that we have function git_read that invokes git and prepares a pipe for the caller to read from. Replace the implementation of githook_read to be just a wrapper around git_read. This unifies the way in which the git executable is invoked. git_read ultimately also calls into _open_stdout_stderr, but it modifies the path to the git executable before doing so. Signed-off-by: Johannes Sixt Signed-off-by: Taylor Blau --- git-gui.sh | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/git-gui.sh b/git-gui.sh index 28113220af..e10bf2a039 100755 --- a/git-gui.sh +++ b/git-gui.sh @@ -669,8 +669,7 @@ proc git_write {args} { } proc githook_read {hook_name args} { - set cmd [concat git hook run --ignore-missing $hook_name -- $args 2>@1] - return [_open_stdout_stderr $cmd] + git_read hook run --ignore-missing $hook_name -- $args 2>@1 } proc kill_file_process {fd} { From dc9ecb1aab1a3438fceeb44db67ddf8e8d938324 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Sat, 3 May 2025 13:24:48 +0200 Subject: [PATCH 10/15] git-gui: convert git_read*, git_write to be non-variadic We are going to treat command arguments and redirections differently to avoid passing arguments that look like redirections to the command accidentally. To do so, it will be necessary to know which arguments are intentional redirections. As a preparation, convert git_read, git_read_nice, and git_write to take just a single argument that is the command in a list. Adjust all call sites accordingly. In the future, this argument will be the regular command arguments and a second argument will be the redirection operations. Signed-off-by: Johannes Sixt Signed-off-by: Taylor Blau --- git-gui.sh | 48 ++++++++++++++++++------------------ lib/blame.tcl | 10 ++++---- lib/branch.tcl | 6 ++--- lib/browser.tcl | 2 +- lib/checkout_op.tcl | 10 ++++---- lib/choose_repository.tcl | 8 +++--- lib/choose_rev.tcl | 6 ++--- lib/commit.tcl | 6 ++--- lib/console.tcl | 2 +- lib/database.tcl | 2 +- lib/diff.tcl | 8 +++--- lib/index.tcl | 8 +++--- lib/merge.tcl | 2 +- lib/mergetool.tcl | 2 +- lib/remote.tcl | 2 +- lib/remote_branch_delete.tcl | 2 +- 16 files changed, 62 insertions(+), 62 deletions(-) diff --git a/git-gui.sh b/git-gui.sh index e10bf2a039..301c7647ec 100755 --- a/git-gui.sh +++ b/git-gui.sh @@ -621,7 +621,7 @@ proc _lappend_nice {cmd_var} { } proc git {args} { - set fd [eval [list git_read] $args] + set fd [git_read $args] fconfigure $fd -translation binary -encoding utf-8 set result [string trimright [read $fd] "\n"] close $fd @@ -642,34 +642,34 @@ proc _open_stdout_stderr {cmd} { return $fd } -proc git_read {args} { - set cmdp [_git_cmd [lindex $args 0]] - set args [lrange $args 1 end] +proc git_read {cmd} { + set cmdp [_git_cmd [lindex $cmd 0]] + set cmd [lrange $cmd 1 end] - return [_open_stdout_stderr [concat $cmdp $args]] + return [_open_stdout_stderr [concat $cmdp $cmd]] } -proc git_read_nice {args} { +proc git_read_nice {cmd} { set opt [list] _lappend_nice opt - set cmdp [_git_cmd [lindex $args 0]] - set args [lrange $args 1 end] + set cmdp [_git_cmd [lindex $cmd 0]] + set cmd [lrange $cmd 1 end] - return [_open_stdout_stderr [concat $opt $cmdp $args]] + return [_open_stdout_stderr [concat $opt $cmdp $cmd]] } -proc git_write {args} { - set cmdp [_git_cmd [lindex $args 0]] - set args [lrange $args 1 end] +proc git_write {cmd} { + set cmdp [_git_cmd [lindex $cmd 0]] + set cmd [lrange $cmd 1 end] - _trace_exec [concat $cmdp $args] - return [open [concat [list | ] $cmdp $args] w] + _trace_exec [concat $cmdp $cmd] + return [open [concat [list | ] $cmdp $cmd] w] } proc githook_read {hook_name args} { - git_read hook run --ignore-missing $hook_name -- $args 2>@1 + git_read [concat [list hook run --ignore-missing $hook_name --] $args 2>@1] } proc kill_file_process {fd} { @@ -1110,10 +1110,10 @@ proc _parse_config {arr_name args} { array unset arr set buf {} catch { - set fd_rc [eval \ - [list git_read config] \ + set fd_rc [git_read \ + [concat config \ $args \ - [list --null --list]] + --null --list]] fconfigure $fd_rc -translation binary -encoding utf-8 set buf [read $fd_rc] close $fd_rc @@ -1482,12 +1482,12 @@ proc rescan {after {honor_trustmtime 1}} { } else { set rescan_active 1 ui_status [mc "Refreshing file status..."] - set fd_rf [git_read update-index \ + set fd_rf [git_read [list update-index \ -q \ --unmerged \ --ignore-missing \ --refresh \ - ] + ]] fconfigure $fd_rf -blocking 0 -translation binary fileevent $fd_rf readable \ [list rescan_stage2 $fd_rf $after] @@ -1527,11 +1527,11 @@ proc rescan_stage2 {fd after} { set rescan_active 2 ui_status [mc "Scanning for modified files ..."] if {[git-version >= "1.7.2"]} { - set fd_di [git_read diff-index --cached --ignore-submodules=dirty -z [PARENT]] + set fd_di [git_read [list diff-index --cached --ignore-submodules=dirty -z [PARENT]]] } else { - set fd_di [git_read diff-index --cached -z [PARENT]] + set fd_di [git_read [list diff-index --cached -z [PARENT]]] } - set fd_df [git_read diff-files -z] + set fd_df [git_read [list diff-files -z]] fconfigure $fd_di -blocking 0 -translation binary -encoding binary fconfigure $fd_df -blocking 0 -translation binary -encoding binary @@ -1540,7 +1540,7 @@ proc rescan_stage2 {fd after} { fileevent $fd_df readable [list read_diff_files $fd_df $after] if {[is_config_true gui.displayuntracked]} { - set fd_lo [eval git_read ls-files --others -z $ls_others] + set fd_lo [git_read [concat ls-files --others -z $ls_others]] fconfigure $fd_lo -blocking 0 -translation binary -encoding binary fileevent $fd_lo readable [list read_ls_others $fd_lo $after] incr rescan_active diff --git a/lib/blame.tcl b/lib/blame.tcl index ceb2330266..d6fd8bea91 100644 --- a/lib/blame.tcl +++ b/lib/blame.tcl @@ -486,9 +486,9 @@ method _load {jump} { fconfigure $fd -eofchar {} } else { if {$do_textconv ne 0} { - set fd [git_read cat-file --textconv "$commit:$path"] + set fd [git_read [list cat-file --textconv "$commit:$path"]] } else { - set fd [git_read cat-file blob "$commit:$path"] + set fd [git_read [list cat-file blob "$commit:$path"]] } } fconfigure $fd \ @@ -617,7 +617,7 @@ method _exec_blame {cur_w cur_d options cur_s} { } lappend options -- $path - set fd [eval git_read_nice blame $options] + set fd [git_read_nice [concat blame $options]] fconfigure $fd -blocking 0 -translation lf -encoding utf-8 fileevent $fd readable [cb _read_blame $fd $cur_w $cur_d] set current_fd $fd @@ -986,7 +986,7 @@ method _showcommit {cur_w lno} { if {[catch {set msg $header($cmit,message)}]} { set msg {} catch { - set fd [git_read cat-file commit $cmit] + set fd [git_read [list cat-file commit $cmit]] fconfigure $fd -encoding binary -translation lf # By default commits are assumed to be in utf-8 set enc utf-8 @@ -1134,7 +1134,7 @@ method _blameparent {} { } else { set diffcmd [list diff-tree --unified=0 $cparent $cmit -- $new_path] } - if {[catch {set fd [eval git_read $diffcmd]} err]} { + if {[catch {set fd [git_read $diffcmd]} err]} { $status_operation stop [mc "Unable to display parent"] error_popup [strcat [mc "Error loading diff:"] "\n\n$err"] return diff --git a/lib/branch.tcl b/lib/branch.tcl index 8b0c485889..39e0f2dc98 100644 --- a/lib/branch.tcl +++ b/lib/branch.tcl @@ -7,7 +7,7 @@ proc load_all_heads {} { set rh refs/heads set rh_len [expr {[string length $rh] + 1}] set all_heads [list] - set fd [git_read for-each-ref --format=%(refname) $rh] + set fd [git_read [list for-each-ref --format=%(refname) $rh]] fconfigure $fd -translation binary -encoding utf-8 while {[gets $fd line] > 0} { if {!$some_heads_tracking || ![is_tracking_branch $line]} { @@ -21,10 +21,10 @@ proc load_all_heads {} { proc load_all_tags {} { set all_tags [list] - set fd [git_read for-each-ref \ + set fd [git_read [list for-each-ref \ --sort=-taggerdate \ --format=%(refname) \ - refs/tags] + refs/tags]] fconfigure $fd -translation binary -encoding utf-8 while {[gets $fd line] > 0} { if {![regsub ^refs/tags/ $line {} name]} continue diff --git a/lib/browser.tcl b/lib/browser.tcl index a982983667..6fc8d4d637 100644 --- a/lib/browser.tcl +++ b/lib/browser.tcl @@ -196,7 +196,7 @@ method _ls {tree_id {name {}}} { lappend browser_stack [list $tree_id $name] $w conf -state disabled - set fd [git_read ls-tree -z $tree_id] + set fd [git_read [list ls-tree -z $tree_id]] fconfigure $fd -blocking 0 -translation binary -encoding utf-8 fileevent $fd readable [cb _read $fd] } diff --git a/lib/checkout_op.tcl b/lib/checkout_op.tcl index 31992f461b..48fd1a3cac 100644 --- a/lib/checkout_op.tcl +++ b/lib/checkout_op.tcl @@ -304,12 +304,12 @@ The rescan will be automatically started now. _readtree $this } else { ui_status [mc "Refreshing file status..."] - set fd [git_read update-index \ + set fd [git_read [list update-index \ -q \ --unmerged \ --ignore-missing \ --refresh \ - ] + ]] fconfigure $fd -blocking 0 -translation binary fileevent $fd readable [cb _refresh_wait $fd] } @@ -345,7 +345,7 @@ method _readtree {} { [mc "Updating working directory to '%s'..." [_name $this]] \ [mc "files checked out"]] - set fd [git_read read-tree \ + set fd [git_read [list read-tree \ -m \ -u \ -v \ @@ -353,7 +353,7 @@ method _readtree {} { $HEAD \ $new_hash \ 2>@1 \ - ] + ]] fconfigure $fd -blocking 0 -translation binary fileevent $fd readable [cb _readtree_wait $fd $status_bar_operation] } @@ -573,7 +573,7 @@ method _confirm_reset {cur} { pack $w.buttons.cancel -side right -padx 5 pack $w.buttons -side bottom -fill x -pady 10 -padx 10 - set fd [git_read rev-list --pretty=oneline $cur ^$new_hash] + set fd [git_read [list rev-list --pretty=oneline $cur ^$new_hash]] while {[gets $fd line] > 0} { set abbr [string range $line 0 7] set subj [string range $line 41 end] diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl index 7bd738e51e..7b64a11239 100644 --- a/lib/choose_repository.tcl +++ b/lib/choose_repository.tcl @@ -818,9 +818,9 @@ method _clone_refs {} { error_popup [mc "Not a Git repository: %s" [file tail $origin_url]] return 0 } - set fd_in [git_read for-each-ref \ + set fd_in [git_read [list for-each-ref \ --tcl \ - {--format=list %(refname) %(objectname) %(*objectname)}] + {--format=list %(refname) %(objectname) %(*objectname)}]] cd $pwd set fd [safe_open_file [gitdir packed-refs] w] @@ -953,14 +953,14 @@ method _do_clone_checkout {HEAD} { [mc "files"]] set readtree_err {} - set fd [git_read read-tree \ + set fd [git_read [list read-tree \ -m \ -u \ -v \ HEAD \ HEAD \ 2>@1 \ - ] + ]] fconfigure $fd -blocking 0 -translation binary fileevent $fd readable [cb _readtree_wait $fd] } diff --git a/lib/choose_rev.tcl b/lib/choose_rev.tcl index 7cf9e160fa..8ae7e8a5c4 100644 --- a/lib/choose_rev.tcl +++ b/lib/choose_rev.tcl @@ -146,14 +146,14 @@ constructor _new {path unmerged_only title} { append fmt { %(*subject)} append fmt {]} set all_refn [list] - set fr_fd [git_read for-each-ref \ + set fr_fd [git_read [list for-each-ref \ --tcl \ --sort=-taggerdate \ --format=$fmt \ refs/heads \ refs/remotes \ refs/tags \ - ] + ]] fconfigure $fr_fd -translation lf -encoding utf-8 while {[gets $fr_fd line] > 0} { set line [eval $line] @@ -176,7 +176,7 @@ constructor _new {path unmerged_only title} { close $fr_fd if {$unmerged_only} { - set fr_fd [git_read rev-list --all ^$::HEAD] + set fr_fd [git_read [list rev-list --all ^$::HEAD]] while {[gets $fr_fd sha1] > 0} { if {[catch {set rlst $cmt_refn($sha1)}]} continue foreach refn $rlst { diff --git a/lib/commit.tcl b/lib/commit.tcl index 8d135845a5..b27e37d385 100644 --- a/lib/commit.tcl +++ b/lib/commit.tcl @@ -27,7 +27,7 @@ You are currently in the middle of a merge that has not been fully completed. Y if {[catch { set name "" set email "" - set fd [git_read cat-file commit $curHEAD] + set fd [git_read [list cat-file commit $curHEAD]] fconfigure $fd -encoding binary -translation lf # By default commits are assumed to be in utf-8 set enc utf-8 @@ -325,7 +325,7 @@ proc commit_commitmsg_wait {fd_ph curHEAD msg_p} { proc commit_writetree {curHEAD msg_p} { ui_status [mc "Committing changes..."] - set fd_wt [git_read write-tree] + set fd_wt [git_read [list write-tree]] fileevent $fd_wt readable \ [list commit_committree $fd_wt $curHEAD $msg_p] } @@ -350,7 +350,7 @@ proc commit_committree {fd_wt curHEAD msg_p} { # -- Verify this wasn't an empty change. # if {$commit_type eq {normal}} { - set fd_ot [git_read cat-file commit $PARENT] + set fd_ot [git_read [list cat-file commit $PARENT]] fconfigure $fd_ot -encoding binary -translation lf set old_tree [gets $fd_ot] close $fd_ot diff --git a/lib/console.tcl b/lib/console.tcl index c7f20b87cb..44dcdf29be 100644 --- a/lib/console.tcl +++ b/lib/console.tcl @@ -93,7 +93,7 @@ method _init {} { method exec {cmd {after {}}} { lappend cmd 2>@1 if {[lindex $cmd 0] eq {git}} { - set fd_f [eval git_read [lrange $cmd 1 end]] + set fd_f [git_read [lrange $cmd 1 end]] } else { set fd_f [_open_stdout_stderr $cmd] } diff --git a/lib/database.tcl b/lib/database.tcl index 85783081e0..1fc0ea00b3 100644 --- a/lib/database.tcl +++ b/lib/database.tcl @@ -3,7 +3,7 @@ proc do_stats {} { global use_ttk NS - set fd [git_read count-objects -v] + set fd [git_read [list count-objects -v]] while {[gets $fd line] > 0} { if {[regexp {^([^:]+): (\d+)$} $line _ name value]} { set stats($name) $value diff --git a/lib/diff.tcl b/lib/diff.tcl index ce1bad6900..8ec740eb50 100644 --- a/lib/diff.tcl +++ b/lib/diff.tcl @@ -338,7 +338,7 @@ proc start_show_diff {cont_info {add_opts {}}} { } } - if {[catch {set fd [eval git_read_nice $cmd]} err]} { + if {[catch {set fd [git_read_nice $cmd]} err]} { set diff_active 0 unlock_index ui_status [mc "Unable to display %s" [escape_path $path]] @@ -617,7 +617,7 @@ proc apply_or_revert_hunk {x y revert} { if {[catch { set enc [get_path_encoding $current_diff_path] - set p [eval git_write $apply_cmd] + set p [git_write $apply_cmd] fconfigure $p -translation binary -encoding $enc puts -nonewline $p $wholepatch close $p} err]} { @@ -853,7 +853,7 @@ proc apply_or_revert_range_or_line {x y revert} { if {[catch { set enc [get_path_encoding $current_diff_path] - set p [eval git_write $apply_cmd] + set p [git_write $apply_cmd] fconfigure $p -translation binary -encoding $enc puts -nonewline $p $current_diff_header puts -nonewline $p $wholepatch @@ -890,7 +890,7 @@ proc undo_last_revert {} { if {[catch { set enc $last_revert_enc - set p [eval git_write $apply_cmd] + set p [git_write $apply_cmd] fconfigure $p -translation binary -encoding $enc puts -nonewline $p $last_revert close $p} err]} { diff --git a/lib/index.tcl b/lib/index.tcl index d2ec24bd80..857864ff2b 100644 --- a/lib/index.tcl +++ b/lib/index.tcl @@ -75,7 +75,7 @@ proc update_indexinfo {msg path_list after} { if {$batch > 25} {set batch 25} set status_bar_operation [$::main_status start $msg [mc "files"]] - set fd [git_write update-index -z --index-info] + set fd [git_write [list update-index -z --index-info]] fconfigure $fd \ -blocking 0 \ -buffering full \ @@ -144,7 +144,7 @@ proc update_index {msg path_list after} { if {$batch > 25} {set batch 25} set status_bar_operation [$::main_status start $msg [mc "files"]] - set fd [git_write update-index --add --remove -z --stdin] + set fd [git_write [list update-index --add --remove -z --stdin]] fconfigure $fd \ -blocking 0 \ -buffering full \ @@ -218,13 +218,13 @@ proc checkout_index {msg path_list after capture_error} { if {$batch > 25} {set batch 25} set status_bar_operation [$::main_status start $msg [mc "files"]] - set fd [git_write checkout-index \ + set fd [git_write [list checkout-index \ --index \ --quiet \ --force \ -z \ --stdin \ - ] + ]] fconfigure $fd \ -blocking 0 \ -buffering full \ diff --git a/lib/merge.tcl b/lib/merge.tcl index e97c91eab6..6317c32127 100644 --- a/lib/merge.tcl +++ b/lib/merge.tcl @@ -239,7 +239,7 @@ Continue with resetting the current changes?"] } if {[ask_popup $op_question] eq {yes}} { - set fd [git_read read-tree --reset -u -v HEAD 2>@1] + set fd [git_read [list read-tree --reset -u -v HEAD 2>@1]] fconfigure $fd -blocking 0 -translation binary set status_bar_operation [$::main_status \ start \ diff --git a/lib/mergetool.tcl b/lib/mergetool.tcl index f2fa439305..777d7b323b 100644 --- a/lib/mergetool.tcl +++ b/lib/mergetool.tcl @@ -88,7 +88,7 @@ proc merge_load_stages {path cont} { set merge_stages(3) {} set merge_stages_buf {} - set merge_stages_fd [eval git_read ls-files -u -z -- {$path}] + set merge_stages_fd [git_read [list ls-files -u -z -- $path]] fconfigure $merge_stages_fd -blocking 0 -translation binary -encoding binary fileevent $merge_stages_fd readable [list read_merge_stages $merge_stages_fd $cont] diff --git a/lib/remote.tcl b/lib/remote.tcl index a733a0f36e..cf796d1601 100644 --- a/lib/remote.tcl +++ b/lib/remote.tcl @@ -32,7 +32,7 @@ proc all_tracking_branches {} { } if {$pat ne {}} { - set fd [eval git_read for-each-ref --format=%(refname) $cmd] + set fd [git_read [concat for-each-ref --format=%(refname) $cmd]] while {[gets $fd n] > 0} { foreach spec $pat { set dst [string range [lindex $spec 0] 0 end-2] diff --git a/lib/remote_branch_delete.tcl b/lib/remote_branch_delete.tcl index 5ba9fcadd1..c8c99b17a8 100644 --- a/lib/remote_branch_delete.tcl +++ b/lib/remote_branch_delete.tcl @@ -308,7 +308,7 @@ method _load {cache uri} { set full_list [list] set head_cache($cache) [list] set full_cache($cache) [list] - set active_ls [git_read ls-remote $uri] + set active_ls [git_read [list ls-remote $uri]] fconfigure $active_ls \ -blocking 0 \ -translation lf \ From 1e0a93c3d35c84547b21ba704a9c4383d4360140 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Sun, 4 May 2025 15:06:11 +0200 Subject: [PATCH 11/15] git-gui: pass redirections as separate argument to _open_stdout_stderr We are going to treat command arguments and redirections differently to avoid passing arguments that look like redirections to the command accidentally. To do so, it will be necessary to know which arguments are intentional redirections. Rewrite direct callers of _open_stdout_stderr to pass intentional redirections as a second (optional) argument. Passing arbitrary arguments is not safe right now, but we rename it to safe_open_command anyway to avoid having to touch the call sites again later when we make it actually safe. We cannot make the function safe right away because one caller is git_read, which does not yet know which of its arguments are redirections. This is the topic of the next commit. Signed-off-by: Johannes Sixt Signed-off-by: Taylor Blau --- git-gui.sh | 10 +++++----- lib/console.tcl | 4 ++-- lib/mergetool.tcl | 4 ++-- lib/sshkey.tcl | 2 +- lib/tools.tcl | 3 +-- 5 files changed, 11 insertions(+), 12 deletions(-) diff --git a/git-gui.sh b/git-gui.sh index 301c7647ec..408149b530 100755 --- a/git-gui.sh +++ b/git-gui.sh @@ -631,10 +631,10 @@ proc git {args} { return $result } -proc _open_stdout_stderr {cmd} { - _trace_exec $cmd +proc safe_open_command {cmd {redir {}}} { + _trace_exec [concat $cmd $redir] if {[catch { - set fd [open [concat [list | ] $cmd] r] + set fd [open [concat [list | ] $cmd $redir] r] } err]} { error $err } @@ -646,7 +646,7 @@ proc git_read {cmd} { set cmdp [_git_cmd [lindex $cmd 0]] set cmd [lrange $cmd 1 end] - return [_open_stdout_stderr [concat $cmdp $cmd]] + return [safe_open_command [concat $cmdp $cmd]] } proc git_read_nice {cmd} { @@ -657,7 +657,7 @@ proc git_read_nice {cmd} { set cmdp [_git_cmd [lindex $cmd 0]] set cmd [lrange $cmd 1 end] - return [_open_stdout_stderr [concat $opt $cmdp $cmd]] + return [safe_open_command [concat $opt $cmdp $cmd]] } proc git_write {cmd} { diff --git a/lib/console.tcl b/lib/console.tcl index 44dcdf29be..cc416d4811 100644 --- a/lib/console.tcl +++ b/lib/console.tcl @@ -91,11 +91,11 @@ method _init {} { } method exec {cmd {after {}}} { - lappend cmd 2>@1 if {[lindex $cmd 0] eq {git}} { + lappend cmd 2>@1 set fd_f [git_read [lrange $cmd 1 end]] } else { - set fd_f [_open_stdout_stderr $cmd] + set fd_f [safe_open_command $cmd [list 2>@1]] } fconfigure $fd_f -blocking 0 -translation binary fileevent $fd_f readable [cb _read $fd_f $after] diff --git a/lib/mergetool.tcl b/lib/mergetool.tcl index 777d7b323b..6b26726418 100644 --- a/lib/mergetool.tcl +++ b/lib/mergetool.tcl @@ -343,9 +343,9 @@ proc merge_tool_start {cmdline target backup stages} { # Force redirection to avoid interpreting output on stderr # as an error, and launch the tool - lappend cmdline {2>@1} + set redir [list {2>@1}] - if {[catch { set mtool_fd [_open_stdout_stderr $cmdline] } err]} { + if {[catch { set mtool_fd [safe_open_command $cmdline $redir] } err]} { delete_temp_files $mtool_tmpfiles error_popup [mc "Could not start the merge tool:\n\n%s" $err] return diff --git a/lib/sshkey.tcl b/lib/sshkey.tcl index 2e006cb8ca..b32bdd06e9 100644 --- a/lib/sshkey.tcl +++ b/lib/sshkey.tcl @@ -85,7 +85,7 @@ proc make_ssh_key {w} { set cmdline [list sh -c {echo | ssh-keygen -q -t rsa -f ~/.ssh/id_rsa 2>&1}] - if {[catch { set sshkey_fd [_open_stdout_stderr $cmdline] } err]} { + if {[catch { set sshkey_fd [safe_open_command $cmdline] } err]} { error_popup [mc "Could not start ssh-keygen:\n\n%s" $err] return } diff --git a/lib/tools.tcl b/lib/tools.tcl index 413f1a1700..142ffceedd 100644 --- a/lib/tools.tcl +++ b/lib/tools.tcl @@ -130,8 +130,7 @@ proc tools_exec {fullname} { } proc tools_run_silent {cmd after} { - lappend cmd 2>@1 - set fd [_open_stdout_stderr $cmd] + set fd [safe_open_command $cmd [list 2>@1]] fconfigure $fd -blocking 0 -translation binary fileevent $fd readable [list tools_consume_input $fd $after] From 60b0ba0a04c413b716dc33f83285ede26e820668 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Sun, 4 May 2025 15:39:03 +0200 Subject: [PATCH 12/15] git-gui: pass redirections as separate argument to git_read We are going to treat command arguments and redirections differently to avoid passing arguments that look like redirections to the command accidentally. To do so, it will be necessary to know which arguments are intentional redirections. Rewrite direct call sites of git_read to pass intentional redirections as a second (optional) argument. git_read defers to safe_open_command, but we cannot make it safe, yet, because one of the callers of git_read is proc git, which does not yet know which of its arguments are redirections. This is the topic of the next commit. Signed-off-by: Johannes Sixt Signed-off-by: Taylor Blau --- git-gui.sh | 6 +++--- lib/checkout_op.tcl | 4 ++-- lib/choose_repository.tcl | 4 ++-- lib/console.tcl | 3 +-- lib/merge.tcl | 2 +- 5 files changed, 9 insertions(+), 10 deletions(-) diff --git a/git-gui.sh b/git-gui.sh index 408149b530..bbdbd35d26 100755 --- a/git-gui.sh +++ b/git-gui.sh @@ -642,11 +642,11 @@ proc safe_open_command {cmd {redir {}}} { return $fd } -proc git_read {cmd} { +proc git_read {cmd {redir {}}} { set cmdp [_git_cmd [lindex $cmd 0]] set cmd [lrange $cmd 1 end] - return [safe_open_command [concat $cmdp $cmd]] + return [safe_open_command [concat $cmdp $cmd] $redir] } proc git_read_nice {cmd} { @@ -669,7 +669,7 @@ proc git_write {cmd} { } proc githook_read {hook_name args} { - git_read [concat [list hook run --ignore-missing $hook_name --] $args 2>@1] + git_read [concat [list hook run --ignore-missing $hook_name --] $args] [list 2>@1] } proc kill_file_process {fd} { diff --git a/lib/checkout_op.tcl b/lib/checkout_op.tcl index 48fd1a3cac..87ed0b4858 100644 --- a/lib/checkout_op.tcl +++ b/lib/checkout_op.tcl @@ -352,8 +352,8 @@ method _readtree {} { --exclude-per-directory=.gitignore \ $HEAD \ $new_hash \ - 2>@1 \ - ]] + ] \ + [list 2>@1]] fconfigure $fd -blocking 0 -translation binary fileevent $fd readable [cb _readtree_wait $fd $status_bar_operation] } diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl index 7b64a11239..5b361cc424 100644 --- a/lib/choose_repository.tcl +++ b/lib/choose_repository.tcl @@ -959,8 +959,8 @@ method _do_clone_checkout {HEAD} { -v \ HEAD \ HEAD \ - 2>@1 \ - ]] + ] \ + [list 2>@1]] fconfigure $fd -blocking 0 -translation binary fileevent $fd readable [cb _readtree_wait $fd] } diff --git a/lib/console.tcl b/lib/console.tcl index cc416d4811..4715ce91e6 100644 --- a/lib/console.tcl +++ b/lib/console.tcl @@ -92,8 +92,7 @@ method _init {} { method exec {cmd {after {}}} { if {[lindex $cmd 0] eq {git}} { - lappend cmd 2>@1 - set fd_f [git_read [lrange $cmd 1 end]] + set fd_f [git_read [lrange $cmd 1 end] [list 2>@1]] } else { set fd_f [safe_open_command $cmd [list 2>@1]] } diff --git a/lib/merge.tcl b/lib/merge.tcl index 6317c32127..55b4fc5ed3 100644 --- a/lib/merge.tcl +++ b/lib/merge.tcl @@ -239,7 +239,7 @@ Continue with resetting the current changes?"] } if {[ask_popup $op_question] eq {yes}} { - set fd [git_read [list read-tree --reset -u -v HEAD 2>@1]] + set fd [git_read [list read-tree --reset -u -v HEAD] [list 2>@1]] fconfigure $fd -blocking 0 -translation binary set status_bar_operation [$::main_status \ start \ From 99f7bc1af65fabab907bf35e645241f714e7386e Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Sun, 4 May 2025 20:26:11 +0200 Subject: [PATCH 13/15] git-gui: introduce function git_redir for git calls with redirections Proc git invokes git and collects all output, which is it returns. We are going to treat command arguments and redirections differently to avoid passing arguments that look like redirections to the command accidentally. A few invocations also pass redirection operators as command arguments deliberately. Rewrite these cases to use a new function git_redir that takes two lists, one for the regular command arguments and one for the redirection operations. Signed-off-by: Johannes Sixt Signed-off-by: Taylor Blau --- git-gui.sh | 8 ++++++-- lib/commit.tcl | 4 ++-- lib/merge.tcl | 2 +- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/git-gui.sh b/git-gui.sh index bbdbd35d26..75d7b9b9fc 100755 --- a/git-gui.sh +++ b/git-gui.sh @@ -621,7 +621,11 @@ proc _lappend_nice {cmd_var} { } proc git {args} { - set fd [git_read $args] + git_redir $args {} +} + +proc git_redir {cmd redir} { + set fd [git_read $cmd $redir] fconfigure $fd -translation binary -encoding utf-8 set result [string trimright [read $fd] "\n"] close $fd @@ -1423,7 +1427,7 @@ proc PARENT {} { return $p } if {$empty_tree eq {}} { - set empty_tree [git mktree << {}] + set empty_tree [git_redir [list mktree] [list << {}]] } return $empty_tree } diff --git a/lib/commit.tcl b/lib/commit.tcl index b27e37d385..bb6056d0ad 100644 --- a/lib/commit.tcl +++ b/lib/commit.tcl @@ -388,8 +388,8 @@ A rescan will be automatically started now. foreach p [concat $PARENT $MERGE_HEAD] { lappend cmd -p $p } - lappend cmd <$msg_p - if {[catch {set cmt_id [eval git $cmd]} err]} { + set msgtxt [list <$msg_p] + if {[catch {set cmt_id [git_redir $cmd $msgtxt]} err]} { catch {file delete $msg_p} error_popup [strcat [mc "commit-tree failed:"] "\n\n$err"] ui_status [mc "Commit failed."] diff --git a/lib/merge.tcl b/lib/merge.tcl index 55b4fc5ed3..44c3f93584 100644 --- a/lib/merge.tcl +++ b/lib/merge.tcl @@ -118,7 +118,7 @@ method _start {} { set cmd [list git] lappend cmd merge lappend cmd --strategy=recursive - lappend cmd [git fmt-merge-msg <[gitdir FETCH_HEAD]] + lappend cmd [git_redir [list fmt-merge-msg] [list <[gitdir FETCH_HEAD]]] lappend cmd HEAD lappend cmd $name } From 44e3935d53e3c0b00ff35bea4fcf8e1731ee4f9b Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Sun, 4 May 2025 21:59:19 +0200 Subject: [PATCH 14/15] git-gui: do not mistake command arguments as redirection operators Tcl 'open' assigns special meaning to its argument when they begin with redirection, pipe or background operator. There are many calls of the 'open' variant that runs a process which construct arguments that are taken from the Git repository or are user input. However, when file names or ref names are taken from the repository, it is possible to find names that have these special forms. They must not be interpreted by 'open' lest it redirects input or output, or attempts to build a pipeline using a command name controlled by the repository. Use the helper function make_arglist_safe, which identifies such arguments and prepends "./" to force such a name to be regarded as a relative file name. After this change the following 'open' calls that start a process do not apply the argument processing: git-gui.sh:4095: || [catch {set spell_fd [open $spell_cmd r+]} spell_err]} { lib/spellcheck.tcl:47: set pipe_fd [open [list | $s_prog -v] r] lib/spellcheck.tcl:133: _connect $this [open $spell_cmd r+] lib/spellcheck.tcl:405: set fd [open [list | aspell dump dicts] r] In all cases, the command arguments are constant strings (or begin with a constant string) that are of a form that would not be affected by the processing anyway. Signed-off-by: Johannes Sixt Signed-off-by: Taylor Blau --- git-gui.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/git-gui.sh b/git-gui.sh index 75d7b9b9fc..9ad172ac66 100755 --- a/git-gui.sh +++ b/git-gui.sh @@ -600,6 +600,7 @@ proc open_cmd_pipe {cmd path} { } else { set run [list [shellpath] -c "$cmd \"\$0\"" $path] } + set run [make_arglist_safe $run] return [open |$run r] } @@ -636,6 +637,7 @@ proc git_redir {cmd redir} { } proc safe_open_command {cmd {redir {}}} { + set cmd [make_arglist_safe $cmd] _trace_exec [concat $cmd $redir] if {[catch { set fd [open [concat [list | ] $cmd $redir] r] @@ -665,6 +667,7 @@ proc git_read_nice {cmd} { } proc git_write {cmd} { + set cmd [make_arglist_safe $cmd] set cmdp [_git_cmd [lindex $cmd 0]] set cmd [lrange $cmd 1 end] From a437f5bc93330a70b42a230e52f3bd036ca1b1da Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Wed, 14 May 2025 21:06:37 +0200 Subject: [PATCH 15/15] git-gui: sanitize 'exec' arguments: convert new 'cygpath' calls The side branch merged in the previous commit introduces new 'exec' calls. Convert these in the same way we did earlier for existing 'exec' calls. Signed-off-by: Johannes Sixt Signed-off-by: Taylor Blau --- git-gui.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/git-gui.sh b/git-gui.sh index 0355c0c836..2c446c2784 100755 --- a/git-gui.sh +++ b/git-gui.sh @@ -393,7 +393,7 @@ if {[string match @@* $_shellpath]} { } if {[is_Windows]} { - set _shellpath [exec cygpath -m $_shellpath] + set _shellpath [safe_exec [list cygpath -m $_shellpath]] } if {![file executable $_shellpath] || \ @@ -2778,7 +2778,7 @@ if {![is_bare]} { if {[is_Windows]} { # Use /git-bash.exe if available - set _git_bash [exec cygpath -m /git-bash.exe] + set _git_bash [safe_exec [list cygpath -m /git-bash.exe]] if {[file executable $_git_bash]} { set _bash_cmdline [list "Git Bash" $_git_bash] } else {