diff options
| -rw-r--r-- | gopher/request.go | 50 | ||||
| -rw-r--r-- | gopher/request_test.go | 26 | ||||
| -rw-r--r-- | internal/server.go | 16 | ||||
| -rw-r--r-- | internal/server_test.go | 66 |
4 files changed, 143 insertions, 15 deletions
diff --git a/gopher/request.go b/gopher/request.go index fdd7480..3aeaf74 100644 --- a/gopher/request.go +++ b/gopher/request.go @@ -11,6 +11,11 @@ import ( "tildegit.org/tjp/sliderule/internal/types" ) +const ( + maxSelectorLen = 255 + maxRequestLen = 4096 +) + // ParseRequest parses a gopher protocol request into a sliderule.Request object. func ParseRequest(rdr io.Reader) (*types.Request, error) { selector, search, err := readFullRequest(rdr) @@ -34,9 +39,7 @@ func ParseRequest(rdr io.Reader) (*types.Request, error) { } func readFullRequest(rdr io.Reader) (string, string, error) { - // The vast majority of requests will fit in this size: - // the specified 255 byte max for selector, then CRLF. - buf := make([]byte, 257) + buf := make([]byte, maxSelectorLen+len("\r\n")) n, err := rdr.Read(buf) if err != nil && !errors.Is(err, io.EOF) { @@ -44,29 +47,46 @@ func readFullRequest(rdr io.Reader) (string, string, error) { } buf = buf[:n] - // Full-text search transactions are the exception, they - // may be longer because there is an additional search string - if n == 257 && buf[256] != '\n' { - intake := buf[n:cap(buf)] - total := n + if lineEnd := bytes.IndexByte(buf, '\n'); lineEnd >= 0 { + buf = buf[:lineEnd+1] + } else if tab := bytes.IndexByte(buf, '\t'); tab >= 0 { + // Full-text search transactions append TAB and a search string. + // RFC 1436 still caps the selector at 255 bytes, but gives no + // search-string limit, so keep a small implementation cap. for { - intake = append(intake, 0) - intake = intake[:cap(intake)] + if len(buf) >= maxRequestLen { + return "", "", errors.New("gopher request too long") + } + intake := make([]byte, maxRequestLen-len(buf)) n, err = rdr.Read(intake) if err != nil && err != io.EOF { return "", "", err } - total += n - if n < cap(intake) || intake[cap(intake)-1] == '\n' { + buf = append(buf, intake[:n]...) + if lineEnd := bytes.IndexByte(intake[:n], '\n'); lineEnd >= 0 { + buf = buf[:len(buf)-n+lineEnd+1] + break + } + if err == io.EOF { break } - intake = intake[n:] } - buf = buf[:total] + } else if len(buf) > maxSelectorLen { + return "", "", errors.New("gopher selector too long") } selector, search, _ := bytes.Cut(buf, []byte{'\t'}) - return strings.TrimRight(string(selector), "\r\n"), strings.TrimRight(string(search), "\r\n"), nil + selector = bytes.TrimRight(selector, "\r\n") + search = bytes.TrimRight(search, "\r\n") + + if len(selector) > maxSelectorLen { + return "", "", errors.New("gopher selector too long") + } + if bytes.ContainsAny(selector, "\x00\r\n") || bytes.ContainsAny(search, "\x00\t\r\n") { + return "", "", errors.New("malformed gopher request") + } + + return string(selector), string(search), nil } diff --git a/gopher/request_test.go b/gopher/request_test.go index 74030c6..3488f39 100644 --- a/gopher/request_test.go +++ b/gopher/request_test.go @@ -2,6 +2,7 @@ package gopher_test import ( "bytes" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -41,3 +42,28 @@ func TestParseRequest(t *testing.T) { }) } } + +func TestParseRequestLimits(t *testing.T) { + t.Run("max selector", func(t *testing.T) { + request, err := gopher.ParseRequest(bytes.NewBufferString(strings.Repeat("a", 255) + "\r\n")) + require.Nil(t, err) + assert.Equal(t, "/"+strings.Repeat("a", 255), request.Path) + }) + + t.Run("selector too long", func(t *testing.T) { + _, err := gopher.ParseRequest(bytes.NewBufferString(strings.Repeat("a", 256) + "\r\n")) + require.NotNil(t, err) + }) + + t.Run("search may extend request line", func(t *testing.T) { + request, err := gopher.ParseRequest(bytes.NewBufferString(strings.Repeat("a", 255) + "\t" + strings.Repeat("b", 255) + "\r\n")) + require.Nil(t, err) + assert.Equal(t, "/"+strings.Repeat("a", 255), request.Path) + assert.Equal(t, strings.Repeat("b", 255), request.RawQuery) + }) + + t.Run("long malformed selector does not panic", func(t *testing.T) { + _, err := gopher.ParseRequest(bytes.NewBufferString(strings.Repeat("a", 1487) + "\r\n")) + require.NotNil(t, err) + }) +} diff --git a/internal/server.go b/internal/server.go index 46b95ef..3ae9be2 100644 --- a/internal/server.go +++ b/internal/server.go @@ -2,7 +2,9 @@ package internal import ( "context" + "fmt" "net" + "runtime/debug" "strings" "sync" @@ -84,6 +86,7 @@ func (s *Server) Serve() error { go func() { defer s.Wg.Done() defer func() { _ = conn.Close() }() + defer s.recoverPanic("connection", "remote", conn.RemoteAddr().String()) s.HandleConn(conn) }() } @@ -129,12 +132,25 @@ func (s *Server) propagateClose() { s.Wg.Add(1) go func() { defer s.Wg.Done() + defer s.recoverPanic("close propagation") <-s.Ctx.Done() _ = s.Listener.Close() }() } +func (s *Server) recoverPanic(goroutine string, keyvals ...any) { + if r := recover(); r != nil { + keyvals = append([]any{ + "msg", "panic in server goroutine", + "goroutine", goroutine, + "panic", fmt.Sprintf("%v", r), + "stack", string(debug.Stack()), + }, keyvals...) + _ = s.ErrorLog.Log(keyvals...) + } +} + // JoinDefaultPort appends ":<port>" iff the address does not already contain a port. func JoinDefaultPort(address string, port string) string { if len(address) > 0 && address[0] == '[' { diff --git a/internal/server_test.go b/internal/server_test.go new file mode 100644 index 0000000..62d8681 --- /dev/null +++ b/internal/server_test.go @@ -0,0 +1,66 @@ +package internal_test + +import ( + "context" + "fmt" + "net" + "strings" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "tildegit.org/tjp/sliderule/internal" +) + +type captureLogger struct { + mu sync.Mutex + logs []string +} + +func (cl *captureLogger) Log(keyvals ...any) error { + cl.mu.Lock() + defer cl.mu.Unlock() + + for _, val := range keyvals { + cl.logs = append(cl.logs, fmt.Sprint(val)) + } + return nil +} + +func (cl *captureLogger) String() string { + cl.mu.Lock() + defer cl.mu.Unlock() + + return strings.Join(cl.logs, " ") +} + +func TestServerRecoversConnectionPanic(t *testing.T) { + var logs captureLogger + server, err := internal.NewServer( + context.Background(), + "localhost", + "tcp", + "127.0.0.1:0", + &logs, + func(net.Conn) { panic("boom") }, + ) + require.Nil(t, err) + + done := make(chan error, 1) + go func() { done <- server.Serve() }() + + conn, err := net.Dial(server.Network(), server.Address()) + require.Nil(t, err) + _ = conn.Close() + + require.Eventually(t, func() bool { + return strings.Contains(logs.String(), "panic in server goroutine") + }, time.Second, 10*time.Millisecond) + + server.Close() + assert.Nil(t, <-done) + assert.Contains(t, logs.String(), "boom") +} |