From 881f2076fb595d85fd8fa80ab2a7000b5a6ab737 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Wed, 16 Nov 2011 10:11:39 -0800 Subject: [PATCH] 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 | 151 ++++++++++++++++------------- src/pkg/net/http/fcgi/fcgi.go | 41 ++++---- src/pkg/net/http/fcgi/fcgi_test.go | 47 ++++++++- 3 files changed, 146 insertions(+), 93 deletions(-) diff --git a/src/pkg/net/http/fcgi/child.go b/src/pkg/net/http/fcgi/child.go index 7b563951cc..529440cbe9 100644 --- a/src/pkg/net/http/fcgi/child.go +++ b/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,91 +124,103 @@ 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) - } - 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 - } - default: - b := make([]byte, 8) - b[0] = rec.h.Type - c.conn.writeRecord(typeUnknownType, 0, b) - } } } +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 { + // 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) + } + 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) { r := newResponse(c, req) httpReq, err := cgi.RequestFromMap(req.params) diff --git a/src/pkg/net/http/fcgi/fcgi.go b/src/pkg/net/http/fcgi/fcgi.go index 70cf781e22..d35aa84d22 100644 --- a/src/pkg/net/http/fcgi/fcgi.go +++ b/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 } diff --git a/src/pkg/net/http/fcgi/fcgi_test.go b/src/pkg/net/http/fcgi/fcgi_test.go index e42f8efd65..6c7e1a9ce8 100644 --- a/src/pkg/net/http/fcgi/fcgi_test.go +++ b/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) + } +}