fcgi: fix server capability discovery

The wrong length was being sent, and two parameters
were also transposed. Made the record type be a type
and made the constants typed, to prevent that sort
of bug in the future.

Fixes #2469

R=golang-dev, edsrzf
CC=golang-dev
https://golang.org/cl/5394046

src/pkg/net/http/fcgi/child.go

@@ -7,6 +7,7 @@ package fcgi
 // This file implements FastCGI from the perspective of a child process.
 
 import (
+	"errors"
 	"fmt"
 	"io"
 	"net"
@@ -123,89 +124,101 @@ func (r *response) Close() error {
 }
 
 type child struct {
-	conn    *conn
-	handler http.Handler
+	conn     *conn
+	handler  http.Handler
+	requests map[uint16]*request // keyed by request ID
 }
 
-func newChild(rwc net.Conn, handler http.Handler) *child {
-	return &child{newConn(rwc), handler}
+func newChild(rwc io.ReadWriteCloser, handler http.Handler) *child {
+	return &child{
+		conn:     newConn(rwc),
+		handler:  handler,
+		requests: make(map[uint16]*request),
+	}
 }
 
 func (c *child) serve() {
-	requests := map[uint16]*request{}
 	defer c.conn.Close()
 	var rec record
-	var br beginRequest
 	for {
 		if err := rec.read(c.conn.rwc); err != nil {
 			return
 		}
-
-		req, ok := requests[rec.h.Id]
-		if !ok && rec.h.Type != typeBeginRequest && rec.h.Type != typeGetValues {
-			// The spec says to ignore unknown request IDs.
-			continue
-		}
-		if ok && rec.h.Type == typeBeginRequest {
-			// The server is trying to begin a request with the same ID
-			// as an in-progress request. This is an error.
+		if err := c.handleRecord(&rec); err != nil {
 			return
 		}
+	}
+}
 
-		switch rec.h.Type {
-		case typeBeginRequest:
-			if err := br.read(rec.content()); err != nil {
-				return
-			}
-			if br.role != roleResponder {
-				c.conn.writeEndRequest(rec.h.Id, 0, statusUnknownRole)
-				break
-			}
-			requests[rec.h.Id] = newRequest(rec.h.Id, br.flags)
-		case typeParams:
-			// NOTE(eds): Technically a key-value pair can straddle the boundary
-			// between two packets. We buffer until we've received all parameters.
-			if len(rec.content()) > 0 {
-				req.rawParams = append(req.rawParams, rec.content()...)
-				break
-			}
-			req.parseParams()
-		case typeStdin:
-			content := rec.content()
-			if req.pw == nil {
-				var body io.ReadCloser
-				if len(content) > 0 {
-					// body could be an io.LimitReader, but it shouldn't matter
-					// as long as both sides are behaving.
-					body, req.pw = io.Pipe()
-				}
-				go c.serveRequest(req, body)
-			}
+var errCloseConn = errors.New("fcgi: connection should be closed")
+
+func (c *child) handleRecord(rec *record) error {
+	req, ok := c.requests[rec.h.Id]
+	if !ok && rec.h.Type != typeBeginRequest && rec.h.Type != typeGetValues {
+		// The spec says to ignore unknown request IDs.
+		return nil
+	}
+	if ok && rec.h.Type == typeBeginRequest {
+		// The server is trying to begin a request with the same ID
+		// as an in-progress request. This is an error.
+		return errors.New("fcgi: received ID that is already in-flight")
+	}
+
+	switch rec.h.Type {
+	case typeBeginRequest:
+		var br beginRequest
+		if err := br.read(rec.content()); err != nil {
+			return err
+		}
+		if br.role != roleResponder {
+			c.conn.writeEndRequest(rec.h.Id, 0, statusUnknownRole)
+			return nil
+		}
+		c.requests[rec.h.Id] = newRequest(rec.h.Id, br.flags)
+	case typeParams:
+		// NOTE(eds): Technically a key-value pair can straddle the boundary
+		// between two packets. We buffer until we've received all parameters.
+		if len(rec.content()) > 0 {
+			req.rawParams = append(req.rawParams, rec.content()...)
+			return nil
+		}
+		req.parseParams()
+	case typeStdin:
+		content := rec.content()
+		if req.pw == nil {
+			var body io.ReadCloser
 			if len(content) > 0 {
-				// 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()
-			}
-		case typeGetValues:
-			values := map[string]string{"FCGI_MPXS_CONNS": "1"}
-			c.conn.writePairs(0, typeGetValuesResult, values)
-		case typeData:
-			// If the filter role is implemented, read the data stream here.
-		case typeAbortRequest:
-			delete(requests, rec.h.Id)
-			c.conn.writeEndRequest(rec.h.Id, 0, statusRequestComplete)
-			if !req.keepConn {
-				// connection will close upon return
-				return
+				// body could be an io.LimitReader, but it shouldn't matter
+				// as long as both sides are behaving.
+				body, req.pw = io.Pipe()
 			}
-		default:
-			b := make([]byte, 8)
-			b[0] = rec.h.Type
-			c.conn.writeRecord(typeUnknownType, 0, b)
+			go c.serveRequest(req, body)
+		}
+		if len(content) > 0 {
+			// 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()
+		}
+	case typeGetValues:
+		values := map[string]string{"FCGI_MPXS_CONNS": "1"}
+		c.conn.writePairs(typeGetValuesResult, 0, values)
+	case typeData:
+		// If the filter role is implemented, read the data stream here.
+	case typeAbortRequest:
+		delete(c.requests, rec.h.Id)
+		c.conn.writeEndRequest(rec.h.Id, 0, statusRequestComplete)
+		if !req.keepConn {
+			// connection will close upon return
+			return errCloseConn
 		}
+	default:
+		b := make([]byte, 8)
+		b[0] = byte(rec.h.Type)
+		c.conn.writeRecord(typeUnknownType, 0, b)
 	}
+	return nil
 }
 
 func (c *child) serveRequest(req *request, body io.ReadCloser) {

src/pkg/net/http/fcgi/fcgi.go

@@ -19,19 +19,22 @@ import (
 	"sync"
 )
 
+// recType is a record type, as defined by
+// http://www.fastcgi.com/devkit/doc/fcgi-spec.html#S8
+type recType uint8
+
 const (
-	// Packet Types
-	typeBeginRequest = iota + 1
-	typeAbortRequest
-	typeEndRequest
-	typeParams
-	typeStdin
-	typeStdout
-	typeStderr
-	typeData
-	typeGetValues
-	typeGetValuesResult
-	typeUnknownType
+	typeBeginRequest    recType = 1
+	typeAbortRequest    recType = 2
+	typeEndRequest      recType = 3
+	typeParams          recType = 4
+	typeStdin           recType = 5
+	typeStdout          recType = 6
+	typeStderr          recType = 7
+	typeData            recType = 8
+	typeGetValues       recType = 9
+	typeGetValuesResult recType = 10
+	typeUnknownType     recType = 11
 )
 
 // keep the connection between web-server and responder open after request
@@ -59,7 +62,7 @@ const headerLen = 8
 
 type header struct {
 	Version       uint8
-	Type          uint8
+	Type          recType
 	Id            uint16
 	ContentLength uint16
 	PaddingLength uint8
@@ -85,7 +88,7 @@ func (br *beginRequest) read(content []byte) error {
 // not synchronized because we don't care what the contents are
 var pad [maxPad]byte
 
-func (h *header) init(recType uint8, reqId uint16, contentLength int) {
+func (h *header) init(recType recType, reqId uint16, contentLength int) {
 	h.Version = 1
 	h.Type = recType
 	h.Id = reqId
@@ -137,7 +140,7 @@ func (r *record) content() []byte {
 }
 
 // writeRecord writes and sends a single record.
-func (c *conn) writeRecord(recType uint8, reqId uint16, b []byte) error {
+func (c *conn) writeRecord(recType recType, reqId uint16, b []byte) error {
 	c.mutex.Lock()
 	defer c.mutex.Unlock()
 	c.buf.Reset()
@@ -167,12 +170,12 @@ func (c *conn) writeEndRequest(reqId uint16, appStatus int, protocolStatus uint8
 	return c.writeRecord(typeEndRequest, reqId, b)
 }
 
-func (c *conn) writePairs(recType uint8, reqId uint16, pairs map[string]string) error {
+func (c *conn) writePairs(recType recType, reqId uint16, pairs map[string]string) error {
 	w := newWriter(c, recType, reqId)
 	b := make([]byte, 8)
 	for k, v := range pairs {
 		n := encodeSize(b, uint32(len(k)))
-		n += encodeSize(b[n:], uint32(len(k)))
+		n += encodeSize(b[n:], uint32(len(v)))
 		if _, err := w.Write(b[:n]); err != nil {
 			return err
 		}
@@ -235,7 +238,7 @@ func (w *bufWriter) Close() error {
 	return w.closer.Close()
 }
 
-func newWriter(c *conn, recType uint8, reqId uint16) *bufWriter {
+func newWriter(c *conn, recType recType, reqId uint16) *bufWriter {
 	s := &streamWriter{c: c, recType: recType, reqId: reqId}
 	w, _ := bufio.NewWriterSize(s, maxWrite)
 	return &bufWriter{s, w}
@@ -245,7 +248,7 @@ func newWriter(c *conn, recType uint8, reqId uint16) *bufWriter {
 // It only writes maxWrite bytes at a time.
 type streamWriter struct {
 	c       *conn
-	recType uint8
+	recType recType
 	reqId   uint16
 }
 

src/pkg/net/http/fcgi/fcgi_test.go

@@ -6,6 +6,7 @@ package fcgi
 
 import (
 	"bytes"
+	"errors"
 	"io"
 	"testing"
 )
@@ -40,25 +41,25 @@ func TestSize(t *testing.T) {
 
 var streamTests = []struct {
 	desc    string
-	recType uint8
+	recType recType
 	reqId   uint16
 	content []byte
 	raw     []byte
 }{
 	{"single record", typeStdout, 1, nil,
-		[]byte{1, typeStdout, 0, 1, 0, 0, 0, 0},
+		[]byte{1, byte(typeStdout), 0, 1, 0, 0, 0, 0},
 	},
 	// this data will have to be split into two records
 	{"two records", typeStdin, 300, make([]byte, 66000),
 		bytes.Join([][]byte{
 			// header for the first record
-			{1, typeStdin, 0x01, 0x2C, 0xFF, 0xFF, 1, 0},
+			{1, byte(typeStdin), 0x01, 0x2C, 0xFF, 0xFF, 1, 0},
 			make([]byte, 65536),
 			// header for the second
-			{1, typeStdin, 0x01, 0x2C, 0x01, 0xD1, 7, 0},
+			{1, byte(typeStdin), 0x01, 0x2C, 0x01, 0xD1, 7, 0},
 			make([]byte, 472),
 			// header for the empty record
-			{1, typeStdin, 0x01, 0x2C, 0, 0, 0, 0},
+			{1, byte(typeStdin), 0x01, 0x2C, 0, 0, 0, 0},
 		},
 			nil),
 	},
@@ -111,3 +112,39 @@ outer:
 		}
 	}
 }
+
+type writeOnlyConn struct {
+	buf []byte
+}
+
+func (c *writeOnlyConn) Write(p []byte) (int, error) {
+	c.buf = append(c.buf, p...)
+	return len(p), nil
+}
+
+func (c *writeOnlyConn) Read(p []byte) (int, error) {
+	return 0, errors.New("conn is write-only")
+}
+
+func (c *writeOnlyConn) Close() error {
+	return nil
+}
+
+func TestGetValues(t *testing.T) {
+	var rec record
+	rec.h.Type = typeGetValues
+
+	wc := new(writeOnlyConn)
+	c := newChild(wc, nil)
+	err := c.handleRecord(&rec)
+	if err != nil {
+		t.Fatalf("handleRecord: %v", err)
+	}
+
+	const want = "\x01\n\x00\x00\x00\x12\x06\x00" +
+		"\x0f\x01FCGI_MPXS_CONNS1" +
+		"\x00\x00\x00\x00\x00\x00\x01\n\x00\x00\x00\x00\x00\x00"
+	if got := string(wc.buf); got != want {
+		t.Errorf(" got: %q\nwant: %q\n", got, want)
+	}
+}