Commit a387f915 authored by Catalin Patulea's avatar Catalin Patulea Committed by Brad Fitzpatrick

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
parent be60bd09
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment