Set Setpgid on child git processes

[zeripath]

When Gitea is running as PID 1 git will occassionally orphan child processes leading
to (defunct) processes. This PR simply sets Setpgid to true on these child processes
meaning that these defunct processes will also be correctly reaped.

Fix #19077

Signed-off-by: Andrew Thornton [email protected]

[zeripath]

PLEASE CHECK THIS COMPILES ON WINDOWS AND RUNS OK ON WINDOWS.

[delvh]

Unfortunately don't have a windows machine to test.

[zeripath]

Hmm... it looks like it doesn't compile on Windows. -- have adjusted this to make it work on doze

[wxiaoguang]

Looks good to me while I think there could be a comment in code to remember the reason why it is needed.

And I am not sure whether there could be a case that: Gitea -> SubProcessA -> SubProcessB , then although SubProcessA is killed (and there is no defunct due to Setpgid), SubProcessB is still running for a long time (still somewhat resource leaking?).

[singuliere]

Looks good to me while I think there could be a comment in code to remember the reason why it is needed.

And I am not sure whether there could be a case that: Gitea -> SubProcessA -> SubProcessB , then although SubProcessA is killed (and there is no defunct due to Setpgid), SubProcessB is still running for a long time (still somewhat resource leaking?).

You misunderstand the purpose of Setpgid, let me explain:

although SubProcessA is killed (and there is no defunct due to Setpgid)...

SubProcessA is not defunct/zombie because Gitea waits for it, not because of Setpgid

SubProcessB is still running for a long time...

the purpose of Setpgid is to make sure SubProcessB is also killed when SubProcessA is killed. For a more detailed explanation and a reproducer, please see https://hostea.org/blog/zombies/.

[wxiaoguang]

func main() {
	c := exec.Command("/bin/bash", "-c", `echo "sleep 100000" | bash`)
	c.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
	fmt.Printf("run err = %v", c.Run())
}

go run test-setpgid.go
pstree
 |      \-+- 86853 user test-setpgid
 |       \-+= 86855 user /bin/bash -c echo "sleep 100000" | bash
 |         \-+- 86857 user bash
 |           \--- 86858 user sleep 100000

kill 86855
run err = signal: terminated

pstree
 |-+- 86857 user bash
 | \--- 86858 user sleep 100000

[singuliere]

kill -9 -- -86855

Would have worked as intended.

[wxiaoguang]

func main() {
	ctx, _ := context.WithTimeout(context.Background(), time.Second*2)
	c := exec.CommandContext(ctx, "/bin/bash", "-c", `echo "while true; do echo running; sleep 1; done" | bash`)
	c.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
	c.Stdout = os.Stdout
	fmt.Printf("run err = %v", c.Run())
}

$ go run test-setpgid.go
running
running
run err = signal: killed
running
running
running
...

[singuliere]

package main

import (
	"fmt"
	"os"
	"os/exec"
	"syscall"
	"time"
)

func main() {
	c := exec.Command("/bin/bash", "-c", `echo "while true; do echo running; sleep 1; done" | bash`)
	c.Stdout = os.Stdout
	c.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
	c.Start()
	time.Sleep(5 * time.Second)
	syscall.Kill(-c.Process.Pid, syscall.SIGKILL)
	fmt.Println("run err = ", c.Wait())
}

$ go run /tmp/pgidworks.go 
running
running
running
running
running
run err =  signal: killed
$

[wxiaoguang]

Then, does Gitea use your syscall.Kill in modules/git/command.go or modules/process/manager_exec.go? Or Gitea just simply uses contexts with deadlines?

[zeripath]

This PR is not designed to kill children - but to simply allow for them to be reaped.

We need to check that this does stop the (defunct) processes from occuring.

[zeripath]

I've just checked and it does.

[zeripath]

Please stop both of you - I've deleted the last few comments as I don't think there is anything to be gained from them.