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.

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

os/exec: if exec.Cmd.Ptrace is set should the code set runtime.LockOSThread()? #28315

Closed
rminnich opened this issue Oct 22, 2018 · 5 comments
Closed
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@rminnich
Copy link
Contributor

What version of Go are you using (go version)?

This is a problem under all most all Go versions, but this was 1.11

Does this issue reproduce with the latest release?

yes, but it's racy, see below.

What operating system and processor architecture are you using (go env)?

almost any linux post about 1.2 :-) and probably any Unix.

What did you do?

As part of writing strace in go for the u-root project I hit an interesting snag and have a question.

If you want to trace a child on Unix, the common path is to have the child of a fork call ptrace with PTRACE_TRACEME. The child will be stopped on the next system call and the parent is then able to take control via ptrace. This is why, as you may have noticed, the strace commands you run show an exec as the first system call.

What's this have to do with Go? The TRACEME rules are strict: only the parent can trace. So if, by some means, the goroutine running the tracing sometimes ends up being run by some process not the parent, you lose trace records with ENOENT, since a process not the parent is trying to trace the child.

I hit this in practice and the fix is easy: the process starting the Command with Ptrace set has to do a runtime.LockOSThread() (and of course unlock after) before starting the command, which ensures that the parent tracing the child is always the parent.

My question: should exec.Command, before calling fork or clone, call runtime.LockOSThread() when Ptrace is set?

On the one hand, it means we're adding a side effect of setting Ptrace: a LockOSThread(). OTOH, NOT setting leads to all kinds of unexpected problems: ptrace failures that seem to have no origin.

Here's a program from liz rice, modified to show the problem. If you use the go func the program rarely works correctly; comment out the go func and it rarely fails.

package main

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

func main() {
var regs syscall.PtraceRegs

cmd := exec.Command(os.Args[1], os.Args[2:]...)
cmd.Stderr = os.Stderr
cmd.Stdout = os.Stdout
cmd.SysProcAttr = &syscall.SysProcAttr{
	Ptrace: true,
}

cmd.Start()
err := cmd.Wait()
if err != nil {
	fmt.Printf("Wait returned: %v\n", err)
}

// comment out go func() to see correct behavior.
/** / go func() /**/{
	pid := cmd.Process.Pid
	var i int
	var sysno uint64
	for {
		i++

		err = syscall.PtraceGetRegs(pid, &regs)
		if err != nil {
			panic(err)
		}
		if (i & 1) == 1 {
			sysno = regs.Orig_rax
			fmt.Printf("%#016x %#016x", sysno, []uint64{
				regs.Rdi, regs.Rsi, regs.Rdx,
				regs.R10, regs.R8, regs.R9})
		} else {
			fmt.Printf("= %d\n", regs.Rax)
		}
		err = syscall.PtraceSyscall(pid, 0)
		if err != nil {
			panic(err)
		}

		_, err = syscall.Wait4(pid, nil, 0, nil)
		if err != nil {
			panic(err)
		}

	}
}/** /() /**/

}

@ianlancetaylor ianlancetaylor changed the title if exec.Cmd.Ptrace is set should the code set runtime.LockOSThread()? os/exec: if exec.Cmd.Ptrace is set should the code set runtime.LockOSThread()? Oct 22, 2018
@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 22, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.12 milestone Oct 22, 2018
@ianlancetaylor
Copy link
Member

It clearly is the parent that is calling syscall.PtraceGetRegs; there is no other process involved. Are you saying that on GNU/Linux it has to be not just the parent process, but the parent thread?

@rminnich
Copy link
Contributor Author

that's what it seems to require.

@rminnich
Copy link
Contributor Author

but if you get a chance try that simple program and see what you think; it's as easy as
go run whatever.go /bin/date
and you will see the issue.

@ianlancetaylor
Copy link
Member

I don't think that exec.Command should call runtime.LockOSThread, because it won't work if it calls runtime.UnlockOSThread. It would be overly complex for the os/exec package to leave the thread locked in such a system-specific case.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/155138 mentions this issue: syscall: document LockOSThread with GNU/Linux SysProcAttr.Ptrace

@golang golang locked and limited conversation to collaborators Dec 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

3 participants