net/http/fcgi: eliminate race, keep request id until end of stdin

There was a race condition that could lead to child.serveRequest
removing the request ID before child.handleRequest had read the empty
FCGI_STDIN message that indicates end-of-stream which in turn could
lead to child.serveRequest blocking while trying to consume the
request body.

Now, we remove the request ID from within child.handleRequest after
the end of stdin has been detected, eliminating the race condition.

Since there are no more concurrent modifications/accesses
to child.requests, we remove the accompanying sync.Mutex.

Change-Id: I80c68e65904a988dfa9e3cceec1829496628ff34
GitHub-Last-Rev: b3976111ae
GitHub-Pull-Request: golang/go#42840
Reviewed-on: https://go-review.googlesource.com/c/go/+/273366
Trust: Damien Neil <dneil@google.com>
Trust: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>

src/net/http/fcgi/child.go

@@ -16,7 +16,6 @@ import (
 	"net/http/cgi"
 	"os"
 	"strings"
-	"sync"
 	"time"
 )
 
@@ -154,7 +153,6 @@ type child struct {
 	conn    *conn
 	handler http.Handler
 
-	mu       sync.Mutex          // protects requests:
 	requests map[uint16]*request // keyed by request ID
 }
 
@@ -193,9 +191,7 @@ var ErrRequestAborted = errors.New("fcgi: request aborted by web server")
 var ErrConnClosed = errors.New("fcgi: connection to web server closed")
 
 func (c *child) handleRecord(rec *record) error {
-	c.mu.Lock()
 	req, ok := c.requests[rec.h.Id]
-	c.mu.Unlock()
 	if !ok && rec.h.Type != typeBeginRequest && rec.h.Type != typeGetValues {
 		// The spec says to ignore unknown request IDs.
 		return nil
@@ -218,9 +214,7 @@ func (c *child) handleRecord(rec *record) error {
 			return nil
 		}
 		req = newRequest(rec.h.Id, br.flags)
-		c.mu.Lock()
 		c.requests[rec.h.Id] = req
-		c.mu.Unlock()
 		return nil
 	case typeParams:
 		// NOTE(eds): Technically a key-value pair can straddle the boundary
@@ -248,8 +242,11 @@ func (c *child) handleRecord(rec *record) error {
 			// TODO(eds): This blocks until the handler reads from the pipe.
 			// If the handler takes a long time, it might be a problem.
 			req.pw.Write(content)
-		} else if req.pw != nil {
-			req.pw.Close()
+		} else {
+			delete(c.requests, req.reqId)
+			if req.pw != nil {
+				req.pw.Close()
+			}
 		}
 		return nil
 	case typeGetValues:
@@ -260,9 +257,7 @@ func (c *child) handleRecord(rec *record) error {
 		// If the filter role is implemented, read the data stream here.
 		return nil
 	case typeAbortRequest:
-		c.mu.Lock()
 		delete(c.requests, rec.h.Id)
-		c.mu.Unlock()
 		c.conn.writeEndRequest(rec.h.Id, 0, statusRequestComplete)
 		if req.pw != nil {
 			req.pw.CloseWithError(ErrRequestAborted)
@@ -309,9 +304,6 @@ func (c *child) serveRequest(req *request, body io.ReadCloser) {
 	// Make sure we serve something even if nothing was written to r
 	r.Write(nil)
 	r.Close()
-	c.mu.Lock()
-	delete(c.requests, req.reqId)
-	c.mu.Unlock()
 	c.conn.writeEndRequest(req.reqId, 0, statusRequestComplete)
 
 	// Consume the entire body, so the host isn't still writing to
@@ -330,8 +322,6 @@ func (c *child) serveRequest(req *request, body io.ReadCloser) {
 }
 
 func (c *child) cleanUp() {
-	c.mu.Lock()
-	defer c.mu.Unlock()
 	for _, req := range c.requests {
 		if req.pw != nil {
 			// race with call to Close in c.serveRequest doesn't matter because

src/net/http/fcgi/fcgi_test.go

@@ -11,6 +11,7 @@ import (
 	"net/http"
 	"strings"
 	"testing"
+	"time"
 )
 
 var sizeTests = []struct {
@@ -399,3 +400,55 @@ func TestResponseWriterSniffsContentType(t *testing.T) {
 		})
 	}
 }
+
+type signallingNopCloser struct {
+	io.Reader
+	closed chan bool
+}
+
+func (*signallingNopCloser) Write(buf []byte) (int, error) {
+	return len(buf), nil
+}
+
+func (rc *signallingNopCloser) Close() error {
+	close(rc.closed)
+	return nil
+}
+
+// Test whether server properly closes connection when processing slow
+// requests
+func TestSlowRequest(t *testing.T) {
+	pr, pw := io.Pipe()
+	go func(w io.Writer) {
+		for _, buf := range [][]byte{
+			streamBeginTypeStdin,
+			makeRecord(typeStdin, 1, nil),
+		} {
+			pw.Write(buf)
+			time.Sleep(100 * time.Millisecond)
+		}
+	}(pw)
+
+	rc := &signallingNopCloser{pr, make(chan bool)}
+	handlerDone := make(chan bool)
+
+	c := newChild(rc, http.HandlerFunc(func(
+		w http.ResponseWriter,
+		r *http.Request,
+	) {
+		w.WriteHeader(200)
+		close(handlerDone)
+	}))
+	go c.serve()
+	defer c.cleanUp()
+
+	timeout := time.After(2 * time.Second)
+
+	<-handlerDone
+	select {
+	case <-rc.closed:
+		t.Log("FastCGI child closed connection")
+	case <-timeout:
+		t.Error("FastCGI child did not close socket after handling request")
+	}
+}