]> Cypherpunks repositories - gostls13.git/commitdiff
fcgi: fix server capability discovery
authorBrad Fitzpatrick <bradfitz@golang.org>
Wed, 16 Nov 2011 18:11:39 +0000 (10:11 -0800)
committerBrad Fitzpatrick <bradfitz@golang.org>
Wed, 16 Nov 2011 18:11:39 +0000 (10:11 -0800)
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
src/pkg/net/http/fcgi/fcgi.go
src/pkg/net/http/fcgi/fcgi_test.go

index 7b563951ccf202a4287b3ff376e40934e61c75d9..529440cbe92a0d7a644e93cdefb9215fb8aba7aa 100644 (file)
@@ -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) {
index 70cf781e228623aa23e904f344e5354030624b6e..d35aa84d229aab920addb06265aa9768f43b1c1d 100644 (file)
@@ -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
 }
 
index e42f8efd65847a9555b35acdc8d8a70bd0a0d9d8..6c7e1a9ce831ff428aa19605e42f74d554a12361 100644 (file)
@@ -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)
+       }
+}