From: Catalin Patulea <catalinp@google.com>
Date: Mon, 17 Mar 2014 22:47:16 +0000 (-0700)
Subject: net/http/fcgi: fix handling of request ID reuse
X-Git-Tag: go1.3beta1~342
X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=a387f915538abbb6f5661cb39b8fccb606c5ad25;p=gostls13.git

net/http/fcgi: fix handling of request ID reuse

Request ID reuse is allowed by the FastCGI spec [1]. In particular nginx uses
the same request ID, 1, for all requests on a given connection. Because
serveRequest does not remove the request from conn.requests, this causes it to
treat the second request as a duplicate and drops the connection immediately
after beginRequest. This manifests with nginx option 'fastcgi_keep_conn on' as
the following message in nginx error log:

2014/03/17 01:39:13 [error] 730#0: *109 recv() failed (104: Connection reset by peer) while reading response header from upstream, client: x.x.x.x, server: example.org, request: "GET / HTTP/1.1", upstream: "fastcgi://127.0.0.1:9001", host: "example.org"

Because handleRecord and serveRequest run in different goroutines, access to
conn.requests must now be synchronized.

[1] http://www.fastcgi.com/drupal/node/6?q=node/22#S3.3

LGTM=bradfitz
R=bradfitz
CC=golang-codereviews
https://golang.org/cl/76800043
---

diff --git a/src/pkg/net/http/fcgi/child.go b/src/pkg/net/http/fcgi/child.go
index 60b794e077..a3beaa33a8 100644
--- a/src/pkg/net/http/fcgi/child.go
+++ b/src/pkg/net/http/fcgi/child.go
@@ -16,6 +16,7 @@ import (
 	"net/http/cgi"
 	"os"
 	"strings"
+	"sync"
 	"time"
 )
 
@@ -126,8 +127,10 @@ func (r *response) Close() error {
 }
 
 type child struct {
-	conn     *conn
-	handler  http.Handler
+	conn    *conn
+	handler http.Handler
+
+	mu       sync.Mutex          // protects requests:
 	requests map[uint16]*request // keyed by request ID
 }
 
@@ -157,7 +160,9 @@ var errCloseConn = errors.New("fcgi: connection should be closed")
 var emptyBody = ioutil.NopCloser(strings.NewReader(""))
 
 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
@@ -179,7 +184,10 @@ func (c *child) handleRecord(rec *record) error {
 			c.conn.writeEndRequest(rec.h.Id, 0, statusUnknownRole)
 			return nil
 		}
-		c.requests[rec.h.Id] = newRequest(rec.h.Id, br.flags)
+		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
@@ -220,7 +228,9 @@ func (c *child) handleRecord(rec *record) error {
 		return nil
 	case typeAbortRequest:
 		println("abort")
+		c.mu.Lock()
 		delete(c.requests, rec.h.Id)
+		c.mu.Unlock()
 		c.conn.writeEndRequest(rec.h.Id, 0, statusRequestComplete)
 		if !req.keepConn {
 			// connection will close upon return
@@ -247,6 +257,9 @@ func (c *child) serveRequest(req *request, body io.ReadCloser) {
 		c.handler.ServeHTTP(r, httpReq)
 	}
 	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