From: Bryan C. Mills Date: Fri, 9 Dec 2022 17:30:24 +0000 (-0500) Subject: syscall: clean up variable declarations in forkAndExecInChild X-Git-Tag: go1.21rc1~1804 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=e216ee7e7416df48dc9550a6f18552a4ada5d419;p=gostls13.git syscall: clean up variable declarations in forkAndExecInChild The various forkAndExecInChild implementations have comments explaining that they pre-declare variables to force allocations to occur before forking, but then later use ":=" declarations for additional variables. To make it clearer that those ":=" declarations do not allocate, we move their declarations up to the predeclared blocks. For #57208. Change-Id: Ie8cb577fa7180b51b64d6dc398169053fdf8ea97 Reviewed-on: https://go-review.googlesource.com/c/go/+/456516 Auto-Submit: Bryan Mills Reviewed-by: Ian Lance Taylor Run-TryBot: Bryan Mills TryBot-Result: Gopher Robot --- diff --git a/src/syscall/exec_bsd.go b/src/syscall/exec_bsd.go index 32c3ebdd9b..379875561b 100644 --- a/src/syscall/exec_bsd.go +++ b/src/syscall/exec_bsd.go @@ -55,10 +55,13 @@ func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr // Declare all variables at top in case any // declarations require heap allocation (e.g., err1). var ( - r1 uintptr - err1 Errno - nextfd int - i int + r1 uintptr + err1 Errno + nextfd int + i int + pgrp _C_int + cred *Credential + ngroups, groups uintptr ) // guard against side effects of shuffling fds below. @@ -119,7 +122,7 @@ func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr if sys.Foreground { // This should really be pid_t, however _C_int (aka int32) is // generally equivalent. - pgrp := _C_int(sys.Pgid) + pgrp = _C_int(sys.Pgid) if pgrp == 0 { r1, _, err1 = RawSyscall(SYS_GETPID, 0, 0, 0) if err1 != 0 { @@ -149,9 +152,9 @@ func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr } // User and groups - if cred := sys.Credential; cred != nil { - ngroups := uintptr(len(cred.Groups)) - groups := uintptr(0) + if cred = sys.Credential; cred != nil { + ngroups = uintptr(len(cred.Groups)) + groups = uintptr(0) if ngroups > 0 { groups = uintptr(unsafe.Pointer(&cred.Groups[0])) } diff --git a/src/syscall/exec_freebsd.go b/src/syscall/exec_freebsd.go index af5a4158f0..9e1cc46c15 100644 --- a/src/syscall/exec_freebsd.go +++ b/src/syscall/exec_freebsd.go @@ -60,10 +60,14 @@ func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr // Declare all variables at top in case any // declarations require heap allocation (e.g., err1). var ( - r1 uintptr - err1 Errno - nextfd int - i int + r1 uintptr + err1 Errno + nextfd int + i int + pgrp _C_int + cred *Credential + ngroups, groups uintptr + upid uintptr ) // Record parent PID so child can test if it has died. @@ -127,7 +131,7 @@ func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr if sys.Foreground { // This should really be pid_t, however _C_int (aka int32) is // generally equivalent. - pgrp := _C_int(sys.Pgid) + pgrp = _C_int(sys.Pgid) if pgrp == 0 { r1, _, err1 = RawSyscall(SYS_GETPID, 0, 0, 0) if err1 != 0 { @@ -157,9 +161,9 @@ func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr } // User and groups - if cred := sys.Credential; cred != nil { - ngroups := uintptr(len(cred.Groups)) - groups := uintptr(0) + if cred = sys.Credential; cred != nil { + ngroups = uintptr(len(cred.Groups)) + groups = uintptr(0) if ngroups > 0 { groups = uintptr(unsafe.Pointer(&cred.Groups[0])) } @@ -204,8 +208,8 @@ func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr // using SIGKILL. r1, _, _ = RawSyscall(SYS_GETPPID, 0, 0, 0) if r1 != ppid { - pid, _, _ := RawSyscall(SYS_GETPID, 0, 0, 0) - _, _, err1 = RawSyscall(SYS_KILL, pid, uintptr(sys.Pdeathsig), 0) + upid, _, _ = RawSyscall(SYS_GETPID, 0, 0, 0) + _, _, err1 = RawSyscall(SYS_KILL, upid, uintptr(sys.Pdeathsig), 0) if err1 != 0 { goto childerror } diff --git a/src/syscall/exec_libc.go b/src/syscall/exec_libc.go index ef0c87e03c..f8769b9aba 100644 --- a/src/syscall/exec_libc.go +++ b/src/syscall/exec_libc.go @@ -81,10 +81,13 @@ func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr // Declare all variables at top in case any // declarations require heap allocation (e.g., err1). var ( - r1 uintptr - err1 Errno - nextfd int - i int + r1 uintptr + err1 Errno + nextfd int + i int + pgrp _Pid_t + cred *Credential + ngroups, groups uintptr ) // guard against side effects of shuffling fds below. @@ -135,7 +138,7 @@ func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr } if sys.Foreground { - pgrp := _Pid_t(sys.Pgid) + pgrp = _Pid_t(sys.Pgid) if pgrp == 0 { r1, err1 = getpid() if err1 != 0 { @@ -165,9 +168,9 @@ func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr } // User and groups - if cred := sys.Credential; cred != nil { - ngroups := uintptr(len(cred.Groups)) - groups := uintptr(0) + if cred = sys.Credential; cred != nil { + ngroups = uintptr(len(cred.Groups)) + groups = uintptr(0) if ngroups > 0 { groups = uintptr(unsafe.Pointer(&cred.Groups[0])) } diff --git a/src/syscall/exec_libc2.go b/src/syscall/exec_libc2.go index 41bc79a721..9b04f96c81 100644 --- a/src/syscall/exec_libc2.go +++ b/src/syscall/exec_libc2.go @@ -52,14 +52,17 @@ func runtime_AfterForkInChild() // functions that do not grow the stack. // //go:norace -func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr *ProcAttr, sys *SysProcAttr, pipe int) (pid int, err Errno) { +func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr *ProcAttr, sys *SysProcAttr, pipe int) (pid int, err1 Errno) { // Declare all variables at top in case any // declarations require heap allocation (e.g., err1). var ( - r1 uintptr - err1 Errno - nextfd int - i int + r1 uintptr + nextfd int + i int + err error + pgrp _C_int + cred *Credential + ngroups, groups uintptr ) // guard against side effects of shuffling fds below. @@ -94,7 +97,7 @@ func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr // Enable tracing if requested. if sys.Ptrace { - if err := ptrace(PTRACE_TRACEME, 0, 0, 0); err != nil { + if err = ptrace(PTRACE_TRACEME, 0, 0, 0); err != nil { err1 = err.(Errno) goto childerror } @@ -120,7 +123,7 @@ func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr if sys.Foreground { // This should really be pid_t, however _C_int (aka int32) is // generally equivalent. - pgrp := _C_int(sys.Pgid) + pgrp = _C_int(sys.Pgid) if pgrp == 0 { r1, _, err1 = rawSyscall(abi.FuncPCABI0(libc_getpid_trampoline), 0, 0, 0) if err1 != 0 { @@ -149,9 +152,9 @@ func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr } // User and groups - if cred := sys.Credential; cred != nil { - ngroups := uintptr(len(cred.Groups)) - groups := uintptr(0) + if cred = sys.Credential; cred != nil { + ngroups = uintptr(len(cred.Groups)) + groups = uintptr(0) if ngroups > 0 { groups = uintptr(unsafe.Pointer(&cred.Groups[0])) } diff --git a/src/syscall/exec_linux.go b/src/syscall/exec_linux.go index 7e0c3d250b..a8eb4bf927 100644 --- a/src/syscall/exec_linux.go +++ b/src/syscall/exec_linux.go @@ -127,19 +127,19 @@ func runtime_AfterForkInChild() func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr *ProcAttr, sys *SysProcAttr, pipe int) (pid int, err Errno) { // Set up and fork. This returns immediately in the parent or // if there's an error. - r1, err1, p, locked := forkAndExecInChild1(argv0, argv, envv, chroot, dir, attr, sys, pipe) + upid, err, mapPipe, locked := forkAndExecInChild1(argv0, argv, envv, chroot, dir, attr, sys, pipe) if locked { runtime_AfterFork() } - if err1 != 0 { - return 0, err1 + if err != 0 { + return 0, err } // parent; return PID - pid = int(r1) + pid = int(upid) if sys.UidMappings != nil || sys.GidMappings != nil { - Close(p[0]) + Close(mapPipe[0]) var err2 Errno // uid/gid mappings will be written after fork and unshare(2) for user // namespaces. @@ -148,8 +148,8 @@ func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr err2 = err.(Errno) } } - RawSyscall(SYS_WRITE, uintptr(p[1]), uintptr(unsafe.Pointer(&err2)), unsafe.Sizeof(err2)) - Close(p[1]) + RawSyscall(SYS_WRITE, uintptr(mapPipe[1]), uintptr(unsafe.Pointer(&err2)), unsafe.Sizeof(err2)) + Close(mapPipe[1]) } return pid, 0 @@ -203,7 +203,7 @@ type cloneArgs struct { // //go:noinline //go:norace -func forkAndExecInChild1(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr *ProcAttr, sys *SysProcAttr, pipe int) (r1 uintptr, err1 Errno, p [2]int, locked bool) { +func forkAndExecInChild1(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr *ProcAttr, sys *SysProcAttr, pipe int) (pid uintptr, err1 Errno, mapPipe [2]int, locked bool) { // Defined in linux/prctl.h starting with Linux 4.3. const ( PR_CAP_AMBIENT = 0x2f @@ -215,8 +215,15 @@ func forkAndExecInChild1(argv0 *byte, argv, envv []*byte, chroot, dir *byte, att // processing in this stack frame and never returns, while the // parent returns immediately from this frame and does all // post-fork processing in the outer frame. + // // Declare all variables at top in case any - // declarations require heap allocation (e.g., err1). + // declarations require heap allocation (e.g., err2). + // ":=" should not be used to declare any variable after + // the call to runtime_BeforeFork. + // + // NOTE(bcmills): The allocation behavior described in the above comment + // seems to lack a corresponding test, and it may be rendered invalid + // by an otherwise-correct change in the compiler. var ( err2 Errno nextfd int @@ -226,6 +233,11 @@ func forkAndExecInChild1(argv0 *byte, argv, envv []*byte, chroot, dir *byte, att puid, psetgroups, pgid []byte uidmap, setgroups, gidmap []byte clone3 *cloneArgs + pgrp int32 + dirfd int + cred *Credential + ngroups, groups uintptr + c uintptr ) if sys.UidMappings != nil { @@ -264,7 +276,7 @@ func forkAndExecInChild1(argv0 *byte, argv, envv []*byte, chroot, dir *byte, att // Allocate another pipe for parent to child communication for // synchronizing writing of User ID/Group ID mappings. if sys.UidMappings != nil || sys.GidMappings != nil { - if err := forkExecPipe(p[:]); err != nil { + if err := forkExecPipe(mapPipe[:]); err != nil { err1 = err.(Errno) return } @@ -288,17 +300,17 @@ func forkAndExecInChild1(argv0 *byte, argv, envv []*byte, chroot, dir *byte, att runtime_BeforeFork() locked = true if clone3 != nil { - r1, err1 = rawVforkSyscall(_SYS_clone3, uintptr(unsafe.Pointer(clone3)), unsafe.Sizeof(*clone3)) + pid, err1 = rawVforkSyscall(_SYS_clone3, uintptr(unsafe.Pointer(clone3)), unsafe.Sizeof(*clone3)) } else { flags |= uintptr(SIGCHLD) if runtime.GOARCH == "s390x" { // On Linux/s390, the first two arguments of clone(2) are swapped. - r1, err1 = rawVforkSyscall(SYS_CLONE, 0, flags) + pid, err1 = rawVforkSyscall(SYS_CLONE, 0, flags) } else { - r1, err1 = rawVforkSyscall(SYS_CLONE, flags, 0) + pid, err1 = rawVforkSyscall(SYS_CLONE, flags, 0) } } - if err1 != 0 || r1 != 0 { + if err1 != 0 || pid != 0 { // If we're in the parent, we must return immediately // so we're not in the same stack frame as the child. // This can at most use the return PC, which the child @@ -320,14 +332,14 @@ func forkAndExecInChild1(argv0 *byte, argv, envv []*byte, chroot, dir *byte, att // Wait for User ID/Group ID mappings to be written. if sys.UidMappings != nil || sys.GidMappings != nil { - if _, _, err1 = RawSyscall(SYS_CLOSE, uintptr(p[1]), 0, 0); err1 != 0 { + if _, _, err1 = RawSyscall(SYS_CLOSE, uintptr(mapPipe[1]), 0, 0); err1 != 0 { goto childerror } - r1, _, err1 = RawSyscall(SYS_READ, uintptr(p[0]), uintptr(unsafe.Pointer(&err2)), unsafe.Sizeof(err2)) + pid, _, err1 = RawSyscall(SYS_READ, uintptr(mapPipe[0]), uintptr(unsafe.Pointer(&err2)), unsafe.Sizeof(err2)) if err1 != 0 { goto childerror } - if r1 != unsafe.Sizeof(err2) { + if pid != unsafe.Sizeof(err2) { err1 = EINVAL goto childerror } @@ -355,11 +367,11 @@ func forkAndExecInChild1(argv0 *byte, argv, envv []*byte, chroot, dir *byte, att } if sys.Foreground { - pgrp := int32(sys.Pgid) + pgrp = int32(sys.Pgid) if pgrp == 0 { - r1, _ = rawSyscallNoError(SYS_GETPID, 0, 0, 0) + pid, _ = rawSyscallNoError(SYS_GETPID, 0, 0, 0) - pgrp = int32(r1) + pgrp = int32(pid) } // Place process group in foreground. @@ -381,11 +393,11 @@ func forkAndExecInChild1(argv0 *byte, argv, envv []*byte, chroot, dir *byte, att } if sys.Unshareflags&CLONE_NEWUSER != 0 && sys.GidMappings != nil { - dirfd := int(_AT_FDCWD) + dirfd = int(_AT_FDCWD) if fd1, _, err1 = RawSyscall6(SYS_OPENAT, uintptr(dirfd), uintptr(unsafe.Pointer(&psetgroups[0])), uintptr(O_WRONLY), 0, 0, 0); err1 != 0 { goto childerror } - r1, _, err1 = RawSyscall(SYS_WRITE, uintptr(fd1), uintptr(unsafe.Pointer(&setgroups[0])), uintptr(len(setgroups))) + pid, _, err1 = RawSyscall(SYS_WRITE, uintptr(fd1), uintptr(unsafe.Pointer(&setgroups[0])), uintptr(len(setgroups))) if err1 != 0 { goto childerror } @@ -396,7 +408,7 @@ func forkAndExecInChild1(argv0 *byte, argv, envv []*byte, chroot, dir *byte, att if fd1, _, err1 = RawSyscall6(SYS_OPENAT, uintptr(dirfd), uintptr(unsafe.Pointer(&pgid[0])), uintptr(O_WRONLY), 0, 0, 0); err1 != 0 { goto childerror } - r1, _, err1 = RawSyscall(SYS_WRITE, uintptr(fd1), uintptr(unsafe.Pointer(&gidmap[0])), uintptr(len(gidmap))) + pid, _, err1 = RawSyscall(SYS_WRITE, uintptr(fd1), uintptr(unsafe.Pointer(&gidmap[0])), uintptr(len(gidmap))) if err1 != 0 { goto childerror } @@ -406,11 +418,11 @@ func forkAndExecInChild1(argv0 *byte, argv, envv []*byte, chroot, dir *byte, att } if sys.Unshareflags&CLONE_NEWUSER != 0 && sys.UidMappings != nil { - dirfd := int(_AT_FDCWD) + dirfd = int(_AT_FDCWD) if fd1, _, err1 = RawSyscall6(SYS_OPENAT, uintptr(dirfd), uintptr(unsafe.Pointer(&puid[0])), uintptr(O_WRONLY), 0, 0, 0); err1 != 0 { goto childerror } - r1, _, err1 = RawSyscall(SYS_WRITE, uintptr(fd1), uintptr(unsafe.Pointer(&uidmap[0])), uintptr(len(uidmap))) + pid, _, err1 = RawSyscall(SYS_WRITE, uintptr(fd1), uintptr(unsafe.Pointer(&uidmap[0])), uintptr(len(uidmap))) if err1 != 0 { goto childerror } @@ -443,9 +455,9 @@ func forkAndExecInChild1(argv0 *byte, argv, envv []*byte, chroot, dir *byte, att } // User and groups - if cred := sys.Credential; cred != nil { - ngroups := uintptr(len(cred.Groups)) - groups := uintptr(0) + if cred = sys.Credential; cred != nil { + ngroups = uintptr(len(cred.Groups)) + groups = uintptr(0) if ngroups > 0 { groups = uintptr(unsafe.Pointer(&cred.Groups[0])) } @@ -474,7 +486,7 @@ func forkAndExecInChild1(argv0 *byte, argv, envv []*byte, chroot, dir *byte, att goto childerror } - for _, c := range sys.AmbientCaps { + for _, c = range sys.AmbientCaps { // Add the c capability to the permitted and inheritable capability mask, // otherwise we will not be able to add it to the ambient capability mask. caps.data[capToIndex(c)].permitted |= capToMask(c) @@ -485,7 +497,7 @@ func forkAndExecInChild1(argv0 *byte, argv, envv []*byte, chroot, dir *byte, att goto childerror } - for _, c := range sys.AmbientCaps { + for _, c = range sys.AmbientCaps { _, _, err1 = RawSyscall6(SYS_PRCTL, PR_CAP_AMBIENT, uintptr(PR_CAP_AMBIENT_RAISE), c, 0, 0, 0) if err1 != 0 { goto childerror @@ -511,9 +523,9 @@ func forkAndExecInChild1(argv0 *byte, argv, envv []*byte, chroot, dir *byte, att // Signal self if parent is already dead. This might cause a // duplicate signal in rare cases, but it won't matter when // using SIGKILL. - r1, _ = rawSyscallNoError(SYS_GETPPID, 0, 0, 0) - if r1 != ppid { - pid, _ := rawSyscallNoError(SYS_GETPID, 0, 0, 0) + pid, _ = rawSyscallNoError(SYS_GETPPID, 0, 0, 0) + if pid != ppid { + pid, _ = rawSyscallNoError(SYS_GETPID, 0, 0, 0) _, _, err1 = RawSyscall(SYS_KILL, pid, uintptr(sys.Pdeathsig), 0) if err1 != 0 { goto childerror diff --git a/src/syscall/exec_plan9.go b/src/syscall/exec_plan9.go index 8f28b5aa22..8762237825 100644 --- a/src/syscall/exec_plan9.go +++ b/src/syscall/exec_plan9.go @@ -135,6 +135,8 @@ func forkAndExecInChild(argv0 *byte, argv []*byte, envv []envItem, dir *byte, at errbuf [ERRMAX]byte statbuf [STATMAX]byte dupdevfd int + n int + b []byte ) // Guard against side effects of shuffling fds below. @@ -177,14 +179,14 @@ func forkAndExecInChild(argv0 *byte, argv []*byte, envv []envItem, dir *byte, at dirloop: for { r1, _, _ = RawSyscall6(SYS_PREAD, uintptr(dupdevfd), uintptr(unsafe.Pointer(&statbuf[0])), uintptr(len(statbuf)), ^uintptr(0), ^uintptr(0), 0) - n := int(r1) + n = int(r1) switch n { case -1: goto childerror case 0: break dirloop } - for b := statbuf[:n]; len(b) > 0; { + for b = statbuf[:n]; len(b) > 0; { var s []byte s, b = gdirname(b) if s == nil {