Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ensure child creation time is after parent creation time #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Ensure child creation time is after parent creation time #1

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a1db4b6
Ensure child creation time is after parent creation time to avoid lin…
nebel Oct 1, 2023
9dc1e4d
Ensure handle is closed in GetCreationTime
nebel Oct 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
module github.com/jesseduffield/kill

go 1.18

require golang.org/x/sys v0.12.0
43 changes: 40 additions & 3 deletions kill_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,49 @@
package kill

import (
"golang.org/x/sys/windows"
"os"
"os/exec"
"syscall"
"unsafe"
)

const PROCESS_ALL_ACCESS = windows.STANDARD_RIGHTS_REQUIRED | windows.SYNCHRONIZE | 0xffff

func GetWindowsHandle(pid int) (handle windows.Handle, err error) {
handle, err = windows.OpenProcess(PROCESS_ALL_ACCESS, false, uint32(pid))
return
}

func GetCreationTime(pid int) (time int64, err error) {
handle, err := GetWindowsHandle(pid)
if err != nil {
return
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a defer CloseHandle(handle) here? I think this might leak otherwise.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this! I added a call to closeHandle(HANDLE(handle)) to reuse the existing function. I also noticed that windows.CloseHandle is available with the new import, though it can also return an error. Not sure if that way would be better or not.

Copy link

@Jim-with-a-J Jim-with-a-J Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not even a beginner with Go, so I'm not qualified to comment on best practice :) The only thing that stands out to me is that you never return anything explicitly, while the rest of the code does. I assume that's purely a stylistic choice and Go does something sensible with the returns (editing previous dumb comment, does it create local variables with the names in the trailing part of the function definition and return those?)

Copy link
Author

@nebel nebel Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that's purely a stylistic choice and Go does something sensible with the returns (editing previous dumb comment, does it create local variables with the names in the trailing part of the function definition and return those?)

Yes, I was also surprised at how this works but the names before the types in the function return type are local variables which will be used as return values when doing a blank return. Seems to be commonly used to avoid having to supply the full (ok, err) tuple at each return, and is used by GetProcs in this code for example. But I'm certainly not qualified to comment on best practices either.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the record I'm not a fan of naked returns (when you specify the variable name as part of the function signature) because they confuse everybody (myself included) who comes across them. I am aware this is my repo so it's strange that the GetProcs function uses naked returns but in truth I hadn't formed this opinion before the PR was originally made (it wasn't made by me)

var u syscall.Rusage
err = syscall.GetProcessTimes(syscall.Handle(handle), &u.CreationTime, &u.ExitTime, &u.KernelTime, &u.UserTime)
if err != nil {
return
}

time = u.CreationTime.Nanoseconds()
return
}

// Kill kills a process, along with any child processes it may have spawned.
func Kill(cmd *exec.Cmd) error {
if cmd.Process == nil {
// You can't kill a person with no body
return nil
}

pids := Getppids(uint32(cmd.Process.Pid))
ptime, err := GetCreationTime(cmd.Process.Pid)
if err != nil {
return err
}

pids := Getppids(uint32(cmd.Process.Pid), ptime)
for _, pid := range pids {
pro, err := os.FindProcess(int(pid))
if err != nil {
Expand Down Expand Up @@ -70,7 +99,7 @@ var (
procCloseHandle = modkernel32.NewProc("CloseHandle")
)

func Getppids(pid uint32) []uint32 {
func Getppids(pid uint32, ptime int64) []uint32 {
infos, err := GetProcs()
if err != nil {
return []uint32{pid}
Expand All @@ -83,7 +112,15 @@ func Getppids(pid uint32) []uint32 {
for index < length {
for _, info := range infos {
if info.PPid == pids[index] {
pids = append(pids, info.Pid)
ctime, err := GetCreationTime(int(info.Pid))
if err != nil {
continue
}

if ctime >= ptime {
// Only appending if child is newer than parent, otherwise PPid was reused
pids = append(pids, info.Pid)
}
}
}
index += 1
Expand Down