HEAD request returning 200 OK with no body will hang indefinitely

[krzysztofsmigiel]

Hi!
While working with Fiber and it's Proxy middleware https://github.com/gofiber/fiber/tree/main/middleware/proxy I've encountered weird bug.
When I am heading to HEAD http://myproxy, server returns:

HTTP/1.1 200 OK
X-elastic-product: Elasticsearch
content-type: application/json
Transfer-Encoding: chunked

When I am trying to do the same with client.Do() it hangs forever here

/go/pkg/mod/github.com/valyala/[email protected]/header.go

func (h *ResponseHeader) tryRead(r *bufio.Reader, n int) error {
	h.resetSkipNormalize()
	b, err := r.Peek(n) <<< Hangs here forever.
	if len(b) == 0 {
		// Return ErrTimeout on any timeout.
		if x, ok := err.(interface{ Timeout() bool }); ok && x.Timeout() {
[...]

I am not that experienced with fasthttp, but it might be related with Transfer-Encoding: chunked returned by the server for this HEAD request.

[newacorn]

Head requet trigger Skip Body pattern. If your heading server send chunked body(not follow the HTTP protocol, not empty body) as head response. fasthttp client must hang forever.
If you want solve this problem，simple change

fasthttp/http.go

Line 1439 in 9fe0bc2

if !resp.mustSkipBody() {

to

fasthttp/http.go

Line 1457 in 9fe0bc2

}

become below:

	if !resp.mustSkipBody() {
		err = resp.ReadBody(r, maxBodySize)
		if err != nil {
			if isConnectionReset(err) {
				return nil
			}
			return err
		}
		//}

		if resp.Header.ContentLength() == -1 && !resp.StreamBody {
			err = resp.Header.ReadTrailer(r)
			if err != nil && err != io.EOF {
				if isConnectionReset(err) {
					return nil
				}
				return err
			}
		}
	}

But now, if your sever send head or other without body chunked response you never read trialer header. but I think it ok.

A better approach is to have the ReadTrailer method perform the check, and if the chunked ending is not correct, it should immediately return an error rather than waiting indefinitely for the server to send a valid chunked ending byte stream.

Currently, fasthttp mixes the handling of user-defined skipBody and skipBody required by the HTTP protocol, which can lead to other issues. A comprehensive fix is needed. However, the code above can temporarily solve your problem effectively.

[newacorn]

Test Replay:

func TestClientSkipRespBodyDontReadTrailer(t *testing.T) {
	server := Server{Handler: func(ctx *RequestCtx) {
		_, err := ctx.WriteString(randomstring.HumanFriendlyString(1024))
		ctx.Response.SetBodyStream(strings.NewReader(randomstring.HumanFriendlyString(1024)), -1)
		assert.NoErr(t, err)
	}}
	req := AcquireRequest()
	defer func() {
		ReleaseRequest(req)
	}()
	req.SetRequestURI("http://127.0.0.1:7070")
	resp := AcquireResponse()
	resp.SkipBody = true
	//
	pcs := fasthttputil.NewPipeConns()
	cliCon, serCon := pcs.Conn1(), pcs.Conn2()
	err := cliCon.SetReadDeadline(time.Now().Add(time.Second*5))
	assert.NoErr(t, err)
	go func() {
		_, err := req.WriteTo(cliCon)
		assert.NoErr(t, err)
		err = resp.Read(bufio.NewReader(cliCon))
		assert.NoErr(t, err)
		err = cliCon.Close()
		assert.NoErr(t, err)
	}()
	//
	err = server.serveConn(serCon)
	if err != nil && !strings.Contains(err.Error(), "closed") {
		t.Fatal(err)
	}
}

Result

=== RUN   TestClientSkipRespBodyDontReadTrailer
    http_test.go:3201: 
          Test Name:  TestClientSkipRespBodyDontReadTrailer
          Error Pos:  /Users/acorn/workspace/programming/golang/wx/fasthttp/http_test.go:3201
          Error Msg:  Received unexpected error:
                      error when reading response headers: timeout. Buffer size=1036, contents: "400\r\nidobatobiratusakupibanudabumosenabidesoluponepuvogiveooparumalotimobegiruvadiouvetumagoyadivukavoaeooeuioouaoeiouaaieeouiiaeeioeeuaeeoaioauoieoiaaeoaeouiaaoiauuoieooeeooiaoiaeiieoaeeaaiieuaoiuuoa"..."oiuoiiaeuaooiauoaeiaeeouiieuaaeoiaoeiaoeuooaiuueaieaiuaooiaeiaeeaoueiauiiueoieeoaeiioaeoiaoeuuoiauooaeuuiiaaeeaieeouieeuoiaeeaueaoeuiaoeiiouieeiouiieaooaiiaeooeaoeiaaoiiueiaeiueeiuaaiioaueeioee\r\n0\r\n\r\n"
--- FAIL: TestClientSkipRespBodyDontReadTrailer (1.00s)

After FIx

=== RUN   TestClientSkipRespBodyDontReadTrailer
--- PASS: TestClientSkipRespBodyDontReadTrailer (0.00s)
PASS

But this is a tmp fix.

[erikdubbelboer]

What is happening is that the client doesn't expect a response body for the HEAD request as that's not possible according to the HTTP spec. So it either fails to read the trailers (which is a bug in fasthttp, since you can't have trailers without a body). Or if there is a timeout set, it will timeout and retry the request, it then tries to read a proper response from the connection while the body of the HEAD response is still on it. So it fails to parse the body of the HEAD response as response trailers and hangs on that. If you set a ReadTimeout it will just fail with a timeout.

#1825 here are some fixes for issues I found while investigating this. But it doesn't fix your main issue of a HEAD having a response. This is just invalid HTTP and we won't support this. net/http also prints Unsolicited response received on idle HTTP channel starting with "24\r\nThis is the data in the first chunk \r\n1B\r\nand this is the second one \r\n0\r\n\r\n" when you try this.

[krzysztofsmigiel]

Thanks you guys for thoughtful research! Now I understand problem more. There is one caveat: my code hangs even before reaching if !resp.mustSkipBody() { on line 1437. It hangs before:

func (resp *Response) ReadLimitBody(r *bufio.Reader, maxBodySize int) error {
	resp.resetSkipHeader()
	err := resp.Header.Read(r) <<<<<<<<<<
	if err != nil {
		return err
	}

So even before considering SkipBody and reading trailers.
This malfunctioned response is sent by donwstream server which I have no control, since I am using Fiber as reverse proxy. As I understand, sending HEAD with Transfer-Encoding: chunked with no valid byte termination 0\r\n\r\n results in error in fasthttp?

[newacorn]

Server

In many aspects, fasthttp aligns with the Go official net/http package. In the current implementation of net/http, if a response is for a HEAD method, it will never include the Transfer-Encoding: chunked header. Consequently, it also won't include any Trailer headers.
net/http/server.go#L1521

if w.req.Method == "HEAD" || !bodyAllowedForStatus(code) || code == StatusNoContent {
		// Response has no body.
		delHeader("Transfer-Encoding")
	} else if hasCL {

FasthttpServer：

fasthttp/http.go

Lines 2064 to 2075 in a1db411

	resp.Header.SetContentLength(-1)
	if err = resp.Header.Write(w); err == nil {
	if resp.ImmediateHeaderFlush {
	err = w.Flush()
	}
	if err == nil && sendBody {
	err = writeBodyChunked(w, resp.bodyStream)
	}
	if err == nil {
	err = resp.Header.writeTrailer(w)
	}
	}

The fasthttp server's handling of responses that shouldn't have a body (including certain status codes or HEAD method requests) is not consistent with net/http. It simply omits the response body, while still sending everything else.This issue should be resolved soon.

Client

net/http/client:
For responses that shouldn't have a body, it will only read the response headers without reading the body (and of course, without reading any trailing headers), then parse them into a Response object and return it. If the response includes a body, it will consider this another type of error, but it won't affect the previously read response (which won't notice this error). From the server's perspective, it has sent a complete response, but from the net/http client's perspective, it has received a complete response without a body followed by some garbage data. When garbage data appears in the session, the client will close the connection and output an error to the terminal. This does not affect the parsing of the first response, which it considers correct.

fasthttp/client:
before this pull #1825, fasthttp would always attempt to read the trailing headers at the end of the response if the Transfer-Encoding was chunked.

After this pull request, the behavior of the fasthttp client is largely consistent with net/http/client. It only reads the parts it should, according to the specifications, leaving any remaining parts (if any) in the connection or partially pre-read in the bufio buffer. However, handling this leftover data will be a concern for the next response parsing.

By the way, if this leftover data is crafted appropriately, it could cause the parsing to hang indefinitely when treated as response headers, rather than reporting a parsing error. So, if you find that some code hangs while parsing response/request headers, it might not be an issue with the headers of the current response/request but rather leftover data from the previous one, especially with long connections. Therefore, you should debug to see exactly what data is causing the header parsing to hang.

sending HEAD with Transfer-Encoding: chunked with no valid byte termination 0\r\n\r\n results in error in fasthttp?
before this pull #1825, Parsing this specific response will not produce any errors, but it may leave some data lingering in the connection or in bufio, or both. After this pull request, there will definitely be some data left in bufio, the connection, or both.
For the uncertainty before the pull request, you can refer to the implementation code below.
It depends on the segment of the byte stream extracted from the connection by buf that includes 0\r\n\r\n.

fasthttp/header.go

Lines 2738 to 2746 in a1db411

	func (h *RequestHeader) parseTrailer(buf []byte) (int, error) {
	// Skip any 0 length chunk.
	if buf[0] == '0' {
	skip := len(strCRLF) + 1
	if len(buf) < skip {
	return 0, io.EOF
	}
	buf = buf[skip:]
	}

[krzysztofsmigiel]

To summarise:

1. When HEAD request is sent with Transfer-Encoding: chunked without proper byte termination it will hang forever.
2. There is no workaround yet, A response without a body can't have trailers #1825 will fix some problems but main case from this issue is not considered.

Debugging shows that it hangs while reading Headers (before reading Body) from connection. This is all I was able to debug with my knowledge.

[erikdubbelboer]

My advise would be to, either set a read timeout so bad requests like this won't hang, or to make sure the client speaks proper HTTP.