Repository: cloudwego/hertz

Issue:
hertz did not cope with malformed connections properly
**Describe the bug**

1. client connected to hertz without sending any data, then client closed connection, hertz server did not close until a few minutes later

<img width="1390" alt="Image" src="https://github.com/user-attachments/assets/9c23b78b-a0c2-4262-ae0d-1b9473a9ea3b" />

2. client send https request to a http port, client send tls ClientHello while hertz server waits, expecting  to receive the first HTTP1.x line;
both client and server wait each other, until client close the connection and server will respond with 400

<img width="1342" alt="Image" src="https://github.com/user-attachments/assets/b883ca3b-19b4-46fb-9111-6e0f61b5ab49" />

**To Reproduce**

Steps to reproduce the behavior:
1. start a demo hertz http server
2. use Postman or anything else to request to the server


**Expected behavior**

1. server responds with TCP FIN when client send FIN
2. server responds with 400 when illegal HTTP method is detected before a complete request line is received

**Screenshots**

If applicable, add screenshots to help explain your problem.

**Hertz version:**

v0.10.0

**Environment:**

The output of `go env`.

**Additional context**

Add any other context about the problem here.


Discussion:
@xiaost: Thanks for feedback.

for simplicity, would like to close conn if it's a malformed request.

Solution patch:
diff --git a/go.mod b/go.mod
index d35115e..8e58c38 100644
--- a/go.mod
+++ b/go.mod
@@ -13,7 +13,7 @@ require (
 	github.com/tidwall/gjson v1.14.4
 	golang.org/x/sync v0.8.0
 	golang.org/x/sys v0.24.0
-	google.golang.org/protobuf v1.27.1
+	google.golang.org/protobuf v1.34.1
 )
 
 require (
diff --git a/go.sum b/go.sum
index 1fba56c..225ff2d 100644
--- a/go.sum
+++ b/go.sum
@@ -120,8 +120,8 @@ golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8T
 golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IVDexMakfv52eLZ2CXBWiKr4=
 golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
 google.golang.org/protobuf v1.26.0-rc.1/go.mod h1:jlhhOSvTdKEhbULTjvd4ARK9grFBp09yW+WbY/TyQbw=
-google.golang.org/protobuf v1.27.1 h1:SnqbnDw1V7RiZcXPx5MEeqPv2s79L9i7BJUlG/+RurQ=
-google.golang.org/protobuf v1.27.1/go.mod h1:9q0QmTI4eRPtz6boOQmLYwt+qCgq0jsYwAQnmE0givc=
+google.golang.org/protobuf v1.34.1 h1:9ddQBjfCyZPOHPUiPxpYESBLc+T8P3E+Vo4IbKZgFWg=
+google.golang.org/protobuf v1.34.1/go.mod h1:c6P6GXX6sHbq/GpV6MGZEdwhWPcYBgnhAHhKbcUYpos=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127 h1:qIbj1fsPNlZgppZ+VLlY7N33q108Sa+fhmuc+sWQYwY=
 gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
diff --git a/pkg/protocol/http1/req/header.go b/pkg/protocol/http1/req/header.go
index 442e344..80745f8 100644
--- a/pkg/protocol/http1/req/header.go
+++ b/pkg/protocol/http1/req/header.go
@@ -131,39 +131,76 @@ func parse(h *protocol.RequestHeader, buf []byte) (int, error) {
 	return m + n, nil
 }
 
+const (
+	maxFirstLineLen   = 8 << 10
+	maxCheckMethodLen = 10
+
+	// reuse ValidHeaderFieldNameTable for Method, both are `token`
+	// see:
+	//	https://www.rfc-editor.org/rfc/rfc9110.html#name-methods
+	//	https://www.rfc-editor.org/rfc/rfc9110.html#name-field-names
+	validMethodCharTable = bytesconv.ValidHeaderFieldNameTable
+)
+
+var errMalformedHTTPRequest = errors.New("malformed HTTP request")
+
+// request-line = method SP request-target SP HTTP-version CRLF
 func parseFirstLine(h *protocol.RequestHeader, buf []byte) (int, error) {
-	bNext := buf
-	var b []byte
-	var err error
-	for len(b) == 0 {
-		if b, bNext, err = utils.NextLine(bNext); err != nil {
-			return 0, err
+	b, leftb, err := utils.NextLine(buf)
+	if err != nil {
+		// errs.ErrNeedMore?
+		// check malformed HTTP request before reading more data
+
+		// check len
+		if len(buf) > maxFirstLineLen {
+			return 0, errMalformedHTTPRequest
+		}
+
+		// check method bytes
+		// NOTE:
+		//  only check method bytes if errs.ErrNeedMore for closing malformed connections.
+		//  for performance concern, it won't be checked in the hot path.
+		for i, c := range buf {
+			if c == ' ' || i > maxCheckMethodLen {
+				break // skip if SP or reach maxCheckMethodLen
+			}
+			if validMethodCharTable[c] == 0 {
+				return 0, errMalformedHTTPRequest
+			}
 		}
+		return 0, err
 	}
 
 	// parse method
 	n := bytes.IndexByte(b, ' ')
 	if n <= 0 {
-		return 0, fmt.Errorf("cannot find http request method in %q", ext.BufferSnippet(buf))
+		return 0, errMalformedHTTPRequest
 	}
 	h.SetMethodBytes(b[:n])
 	b = b[n+1:]
 
-	// Set default protocol
-	h.SetProtocol(consts.HTTP11)
-	// parse requestURI
-	n = bytes.LastIndexByte(b, ' ')
-	if n < 0 {
-		h.SetProtocol(consts.HTTP10)
-		n = len(b)
-	} else if n == 0 {
-		return 0, fmt.Errorf("requestURI cannot be empty in %q", buf)
-	} else if !bytes.Equal(b[n+1:], bytestr.StrHTTP11) {
-		h.SetProtocol(consts.HTTP10)
+	// parse request-target (uri)
+	n = bytes.IndexByte(b, ' ')
+	if n <= 0 {
+		return 0, errMalformedHTTPRequest
 	}
 	h.SetRequestURIBytes(b[:n])
+	b = b[n+1:]
 
-	return len(buf) - len(bNext), nil
+	// parse http protocol
+	switch string(b) {
+	case consts.HTTP11: // likely HTTP/1.1
+		h.SetProtocol(consts.HTTP11)
+	case consts.HTTP10:
+		h.SetProtocol(consts.HTTP10)
+	default:
+		if len(b) < 5 || string(b[:5]) != "HTTP/" {
+			return 0, errMalformedHTTPRequest
+		}
+		// XXX: all other cases are considered to be HTTP/1.0 for safe
+		h.SetProtocol(consts.HTTP10)
+	}
+	return len(buf) - len(leftb), nil
 }
 
 // validHeaderFieldValue is equal to httpguts.ValidHeaderFieldValue（shares the same context）
diff --git a/pkg/protocol/http1/resp/writer.go b/pkg/protocol/http1/resp/writer.go
index 02a3b7c..1150826 100644
--- a/pkg/protocol/http1/resp/writer.go
+++ b/pkg/protocol/http1/resp/writer.go
@@ -26,10 +26,10 @@ import (
 	"github.com/cloudwego/hertz/pkg/protocol/http1/ext"
 )
 
-var chunkReaderPool sync.Pool
+var chunkWriterPool sync.Pool
 
 func init() {
-	chunkReaderPool = sync.Pool{
+	chunkWriterPool = sync.Pool{
 		New: func() interface{} {
 			return &chunkedBodyWriter{}
 		},
@@ -58,6 +58,12 @@ func (c *chunkedBodyWriter) Write(p []byte) (n int, err error) {
 	if err := c.writeHeader(); err != nil {
 		return 0, err
 	}
+	if len(p) == 0 {
+		// prevent from sending zero-len chunk which indicates stream ends.
+		// callers may write with zero-len buf unintentionally.
+		// use Finalize() instead.
+		return 0, nil
+	}
 	if err := c.writeChunk(p); err != nil {
 		return 0, err
 	}
@@ -93,11 +99,14 @@ func (c *chunkedBodyWriter) Finalize() error {
 	if c.finalized || c.err != nil {
 		return c.err
 	}
+	c.finalized = true
+	if err := c.writeHeader(); err != nil {
+		return err
+	}
 	// zero-len chunk
-	if _, err := c.Write(nil); err != nil {
+	if err := c.writeChunk(nil); err != nil {
 		return err
 	}
-	c.finalized = true
 	// trailer which ends with \r\n
 	_, c.err = c.w.WriteBinary(c.r.Header.Trailer().Header())
 	if c.err == nil {
@@ -112,12 +121,12 @@ func (c *chunkedBodyWriter) release() {
 	c.err = nil
 	c.finalized = false
 	c.wroteHeader = false
-	chunkReaderPool.Put(c)
+	chunkWriterPool.Put(c)
 }
 
 // NewChunkedBodyWriter creates a new chunked body writer.
 func NewChunkedBodyWriter(r *protocol.Response, w network.Writer) network.ExtWriter {
-	extWriter := chunkReaderPool.Get().(*chunkedBodyWriter)
+	extWriter := chunkWriterPool.Get().(*chunkedBodyWriter)
 	extWriter.r = r
 	extWriter.w = w
 	runtime.SetFinalizer(extWriter, (*chunkedBodyWriter).release)
diff --git a/pkg/route/engine.go b/pkg/route/engine.go
index 81e4f75..502c16d 100644
--- a/pkg/route/engine.go
+++ b/pkg/route/engine.go
@@ -455,17 +455,7 @@ func (engine *Engine) onData(c context.Context, conn interface{}) (err error) {
 	return
 }
 
-func errProcess(conn io.Closer, err error) {
-	if err == nil {
-		return
-	}
-
-	defer func() {
-		if err != nil {
-			conn.Close()
-		}
-	}()
-
+func logError(conn network.Conn, err error) {
 	// Quiet close the connection
 	if errors.Is(err, errs.ErrShortConnection) || errors.Is(err, errs.ErrIdleTimeout) {
 		return
@@ -473,12 +463,14 @@ func errProcess(conn io.Closer, err error) {
 
 	// Do not process the hijack connection error
 	if errors.Is(err, errs.ErrHijacked) {
-		err = nil
 		return
 	}
 
 	// Get remote address
-	rip := getRemoteAddrFromCloser(conn)
+	rip := ""
+	if addr := conn.RemoteAddr(); addr != nil {
+		rip = addr.String()
+	}
 
 	// Handle Specific error
 	if hsp, ok := conn.(network.HandleSpecificError); ok {
@@ -490,15 +482,6 @@ func errProcess(conn io.Closer, err error) {
 	hlog.SystemLogger().Errorf(hlog.EngineErrorFormat, err.Error(), rip)
 }
 
-func getRemoteAddrFromCloser(conn io.Closer) string {
-	if c, ok := conn.(network.Conn); ok {
-		if addr := c.RemoteAddr(); addr != nil {
-			return addr.String()
-		}
-	}
-	return ""
-}
-
 func (engine *Engine) Close() error {
 	if engine.htmlRender != nil {
 		engine.htmlRender.Close() //nolint:errcheck
@@ -523,7 +506,12 @@ func (engine *Engine) GetServerName() []byte {
 
 func (engine *Engine) Serve(c context.Context, conn network.Conn) (err error) {
 	defer func() {
-		errProcess(conn, err)
+		if err != nil {
+			logError(conn, err)
+		}
+		if !errors.Is(err, errs.ErrHijacked) {
+			_ = conn.Close()
+		}
 	}()
 
 	// H2C path



Test patch:
diff --git a/pkg/protocol/http1/req/header_test.go b/pkg/protocol/http1/req/header_test.go
index 29192c7..12b0e65 100644
--- a/pkg/protocol/http1/req/header_test.go
+++ b/pkg/protocol/http1/req/header_test.go
@@ -46,8 +46,6 @@ import (
 	"bytes"
 	"errors"
 	"fmt"
-	"net/http"
-	"strings"
 	"testing"
 
 	errs "github.com/cloudwego/hertz/pkg/common/errors"
@@ -82,34 +80,6 @@ func TestRequestHeader_Read(t *testing.T) {
 	assert.DeepEqual(t, []byte("100-continue"), rh.Peek("Expect"))
 }
 
-func TestRequestHeaderMultiLineValue(t *testing.T) {
-	s := "HTTP/1.1 200 OK\r\n" +
-		"EmptyValue1:\r\n" +
-		"Content-Type: foo/bar;\r\n\tnewline;\r\n another/newline\r\n" +
-		"Foo: Bar\r\n" +
-		"Multi-Line: one;\r\n two\r\n" +
-		"Values: v1;\r\n v2; v3;\r\n v4;\tv5\r\n" +
-		"\r\n"
-
-	header := new(protocol.RequestHeader)
-	if _, err := parse(header, []byte(s)); err != nil {
-		t.Fatalf("parse headers with multi-line values failed, %s", err)
-	}
-	response, err := http.ReadResponse(bufio.NewReader(strings.NewReader(s)), nil)
-	if err != nil {
-		t.Fatalf("parse response using net/http failed, %s", err)
-	}
-
-	for name, vals := range response.Header {
-		got := string(header.Peek(name))
-		want := vals[0]
-
-		if got != want {
-			t.Errorf("unexpected %s got: %q want: %q", name, got, want)
-		}
-	}
-}
-
 func TestRequestHeader_Peek(t *testing.T) {
 	s := "PUT /foo/bar HTTP/1.1\r\nExpect: 100-continue\r\nUser-Agent: foo\r\nHost: 127.0.0.1\r\nConnection: Keep-Alive\r\nContent-Length: 5\r\nTransfer-Encoding: foo\r\nContent-Type: foo/bar\r\n\r\nabcdef4343"
 	zr := mock.NewZeroCopyReader(s)
@@ -446,46 +416,68 @@ func TestTryRead(t *testing.T) {
 
 func TestParseFirstLine(t *testing.T) {
 	tests := []struct {
+		name     string
 		input    []byte
 		method   string
 		uri      string
 		protocol string
 		err      error
 	}{
-		// Test case 1: n < 0
 		{
+			name:     "case: normal",
 			input:    []byte("GET /path/to/resource HTTP/1.0\r\n"),
 			method:   "GET",
 			uri:      "/path/to/resource",
 			protocol: "HTTP/1.0",
-			err:      nil,
 		},
-		// Test case 2: n == 0
 		{
-			input:    []byte(" /path/to/resource HTTP/1.1\r\n"),
-			method:   "",
-			uri:      "",
-			protocol: "",
-			err:      fmt.Errorf("requestURI cannot be empty in"),
+			name:  "case: empty uri",
+			input: []byte("GET  HTTP/1.1\r\n"),
+			err:   errMalformedHTTPRequest,
 		},
-		// Test case 3: !bytes.Equal(b[n+1:], bytestr.StrHTTP11)
 		{
+			name:     "case: unknown protocol should use HTTP/1.0",
 			input:    []byte("POST /path/to/resource HTTP/1.2\r\n"),
 			method:   "POST",
 			uri:      "/path/to/resource",
 			protocol: "HTTP/1.0",
-			err:      nil,
+		},
+		{
+			name:  "case: invalid protocol",
+			input: []byte("POST /path/to/resource XTTP/1.1\r\n"),
+			err:   errMalformedHTTPRequest,
+		},
+		{
+			name:  "case: input too large",
+			input: make([]byte, 9<<10),
+			err:   errMalformedHTTPRequest,
+		},
+		{
+			name:  "case: method invalid",
+			input: []byte("< / HTTP/1."),
+			err:   errMalformedHTTPRequest,
+		},
+		{
+			name:  "case: need more err",
+			input: []byte("GET / HTTP/1."),
+			err:   errs.ErrNeedMore,
 		},
 	}
 
 	for _, tc := range tests {
-		header := &protocol.RequestHeader{}
-		_, err := parseFirstLine(header, tc.input)
+		h := &protocol.RequestHeader{}
+		_, err := parseFirstLine(h, tc.input)
 		if tc.err != nil {
-			assert.NotNil(t, err)
-		} else {
-			assert.Nil(t, err)
+			assert.Assert(t, errors.Is(err, tc.err), tc.name, err)
+			continue
 		}
+		assert.Assert(t, err == nil, tc.name, err)
+		if string(h.Method()) != tc.method ||
+			string(h.RequestURI()) != tc.uri ||
+			h.GetProtocol() != tc.protocol {
+			t.Fatal(tc.name, "got", h.String())
+		}
+
 	}
 }
 
diff --git a/pkg/protocol/http1/resp/header_test.go b/pkg/protocol/http1/resp/header_test.go
index 0fad023..488849f 100644
--- a/pkg/protocol/http1/resp/header_test.go
+++ b/pkg/protocol/http1/resp/header_test.go
@@ -42,7 +42,10 @@
 package resp
 
 import (
+	"bufio"
 	"bytes"
+	"net/http"
+	"strings"
 	"testing"
 
 	"github.com/cloudwego/hertz/pkg/protocol"
@@ -179,3 +182,31 @@ func equalCookie(c1, c2 *protocol.Cookie) bool {
 	}
 	return true
 }
+
+func TestResponseHeaderMultiLineValue(t *testing.T) {
+	s := "HTTP/1.1 200 OK\r\n" +
+		"EmptyValue1:\r\n" +
+		"Content-Type: foo/bar;\r\n\tnewline;\r\n another/newline\r\n" +
+		"Foo: Bar\r\n" +
+		"Multi-Line: one;\r\n two\r\n" +
+		"Values: v1;\r\n v2; v3;\r\n v4;\tv5\r\n" +
+		"\r\n"
+
+	header := new(protocol.ResponseHeader)
+	if _, err := parse(header, []byte(s)); err != nil {
+		t.Fatalf("parse headers with multi-line values failed, %s", err)
+	}
+	response, err := http.ReadResponse(bufio.NewReader(strings.NewReader(s)), nil)
+	if err != nil {
+		t.Fatalf("parse response using net/http failed, %s", err)
+	}
+
+	for name, vals := range response.Header {
+		got := string(header.Peek(name))
+		want := vals[0]
+
+		if got != want {
+			t.Errorf("unexpected %s got: %q want: %q", name, got, want)
+		}
+	}
+}
diff --git a/pkg/protocol/http1/resp/writer_test.go b/pkg/protocol/http1/resp/writer_test.go
index 80bec2f..c52b34c 100644
--- a/pkg/protocol/http1/resp/writer_test.go
+++ b/pkg/protocol/http1/resp/writer_test.go
@@ -33,6 +33,7 @@ func TestNewChunkedBodyWriter(t *testing.T) {
 	mockConn := mock.NewConn("")
 	w := NewChunkedBodyWriter(response, mockConn)
 	_, _ = w.Write([]byte("hello"))
+	_, _ = w.Write(nil) // noop
 	assert.Nil(t, w.Flush())
 	out, _ := mockConn.WriterRecorder().Peek(mockConn.WriterRecorder().WroteLen())
 	resp := string(out)