os: don't store reference count in Process.state

We only need a reference count in processHandle.

For #70907
Fixes #71564

Change-Id: I209ded869203dea10f12b070190774fb5f1d3d71
Reviewed-on: https://go-review.googlesource.com/c/go/+/638577
Commit-Queue: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>

diff --git a/src/os/exec.go b/src/os/exec.go
index a531cdab0801144610671dd5e803d35c25bf4002..f3cede299673aa04e1a0042847153b1fabdd0230 100644 (file)
--- a/src/os/exec.go
+++ b/src/os/exec.go
@@ -38,31 +38,10 @@ const (
 type Process struct {
        Pid int
 
-       // State contains the atomic process state.
+       // state contains the atomic process state.
        //
-       // If handle is nil, this consists only of the processStatus fields,
+       // This consists of the processStatus fields,
        // which indicate if the process is done/released.
-       //
-       // In handle is not nil, the lower bits also contain a reference
-       // count for the handle field.
-       //
-       // The Process itself initially holds 1 persistent reference. Any
-       // operation that uses the handle with a system call temporarily holds
-       // an additional transient reference. This prevents the handle from
-       // being closed prematurely, which could result in the OS allocating a
-       // different handle with the same value, leading to Process' methods
-       // operating on the wrong process.
-       //
-       // Release and Wait both drop the Process' persistent reference, but
-       // other concurrent references may delay actually closing the handle
-       // because they hold a transient reference.
-       //
-       // Regardless, we want new method calls to immediately treat the handle
-       // as unavailable after Release or Wait to avoid extending this delay.
-       // This is achieved by setting either processStatus flag when the
-       // Process' persistent reference is dropped. The only difference in the
-       // flags is the reason the handle is unavailable, which affects the
-       // errors returned by concurrent calls.
        state atomic.Uint64
 
        // Used only when handle is nil
@@ -151,7 +130,6 @@ func newHandleProcess(pid int, handle uintptr) *Process {
                Pid:    pid,
                handle: ph,
        }
-       p.state.Store(1) // 1 persistent reference
        runtime.SetFinalizer(p, (*Process).Release)
        return p
 }
@@ -170,53 +148,31 @@ func (p *Process) handleTransientAcquire() (uintptr, processStatus) {
                panic("handleTransientAcquire called in invalid mode")
        }
 
-       for {
-               refs := p.state.Load()
-               if refs&processStatusMask != 0 {
-                       return 0, processStatus(refs & processStatusMask)
-               }
-               new := refs + 1
-               if !p.state.CompareAndSwap(refs, new) {
-                       continue
-               }
-               h, ok := p.handle.acquire()
-               if !ok {
-                       panic("inconsistent reference counts")
-               }
+       state := p.state.Load()
+       if state&processStatusMask != 0 {
+               return 0, processStatus(state & processStatusMask)
+       }
+       h, ok := p.handle.acquire()
+       if ok {
                return h, statusOK
        }
+
+       // This case means that the handle has been closed.
+       // We always set the status to non-zero before closing the handle.
+       // If we get here the status must have been set non-zero after
+       // we just checked it above.
+       state = p.state.Load()
+       if state&processStatusMask == 0 {
+               panic("inconsistent process status")
+       }
+       return 0, processStatus(state & processStatusMask)
 }
 
 func (p *Process) handleTransientRelease() {
        if p.handle == nil {
                panic("handleTransientRelease called in invalid mode")
        }
-
-       for {
-               state := p.state.Load()
-               refs := state &^ processStatusMask
-               status := processStatus(state & processStatusMask)
-               if refs == 0 {
-                       // This should never happen because
-                       // handleTransientRelease is always paired with
-                       // handleTransientAcquire.
-                       panic("release of handle with refcount 0")
-               }
-               if refs == 1 && status == statusOK {
-                       // Process holds a persistent reference and always sets
-                       // a status when releasing that reference
-                       // (handlePersistentRelease). Thus something has gone
-                       // wrong if this is the last release but a status has
-                       // not always been set.
-                       panic("final release of handle without processStatus")
-               }
-               new := state - 1
-               if !p.state.CompareAndSwap(state, new) {
-                       continue
-               }
-               p.handle.release()
-               return
-       }
+       p.handle.release()
 }
 
 // Drop the Process' persistent reference on the handle, deactivating future
@@ -230,8 +186,8 @@ func (p *Process) handlePersistentRelease(reason processStatus) processStatus {
        }
 
        for {
-               refs := p.state.Load()
-               status := processStatus(refs & processStatusMask)
+               state := p.state.Load()
+               status := processStatus(state & processStatusMask)
                if status != statusOK {
                        // Both Release and successful Wait will drop the
                        // Process' persistent reference on the handle. We
@@ -240,13 +196,7 @@ func (p *Process) handlePersistentRelease(reason processStatus) processStatus {
                        // reference is dropped exactly once.
                        return status
                }
-               if refs == 0 {
-                       // This should never happen because dropping the
-                       // persistent reference always sets a status.
-                       panic("release of handle with refcount 0")
-               }
-               new := (refs - 1) | uint64(reason)
-               if !p.state.CompareAndSwap(refs, new) {
+               if !p.state.CompareAndSwap(state, uint64(reason)) {
                        continue
                }
                p.handle.release()