Status Line does not have CRLF appended properly to it

[ShivanshVij]

I think I've found a bug where a response status line is not properly formatted then the ResponseHeader.AppendBytes function is called.

A screenshot of the error is attached:

You can see the error in the bottom part of the above screenshot

What seems to be happening is pretty clear, the response is parsed correctly (I have confirmed this), but when the AppendBytes function is being called, a newline (CRLF) is not being appended in the following lines:

[ShivanshVij]

It might have something to do with this commit which modifies the statusline logic: 556aa81

[ShivanshVij]

Okay I've spent some time investigating and reverse engineering the bugs, and it looks like the problem is in this line: 556aa81#diff-445b7d49a79bb217c576f137077e18262a0a2d58e424239fcc01a437397188f7R1659

Before this change, the function call would be dst = append(dst, statusLine(statusCode)...) which was correct because the statusLine function would make use of the statusLines map which was initialized like so:

func init() {
	// Fill all valid status lines
	for i := 0; i < len(statusLines); i++ {
		statusLines[i] = []byte(fmt.Sprintf("HTTP/1.1 %d %s\r\n", i, StatusMessage(i)))
	}
}

(that code snippet is available here: https://github.com/valyala/fasthttp/blob/master/status.go#L171)

Now, since we're simply returning the editable statusLine variable, which is filled in by this code snipped:

	// parse status code
	h.statusCode, n, err = parseUintBuf(b)
	if err != nil {
		if h.secureErrorLogMessage {
			return 0, fmt.Errorf("cannot parse response status code: %s", err)
		}
		return 0, fmt.Errorf("cannot parse response status code: %s. Response %q", err, buf)
	}
	if len(b) > n && b[n] != ' ' {
		if h.secureErrorLogMessage {
			return 0, fmt.Errorf("unexpected char at the end of status code")
		}
		return 0, fmt.Errorf("unexpected char at the end of status code. Response %q", buf)
	}
	if len(b) > n+1 && !bytes.Equal(b[n+1:], statusLine(h.statusCode)) {
		h.SetStatusLine(b[n+1:])
	}

(from https://github.com/valyala/fasthttp/blob/master/header.go#L1871)

We can see from the above snippet that we're not adding the status code or HTTP/1.1 to the first line of the response anymore.

So the bug is more than just the CRLF endings. It is due to the way the h.statusLine field is being initialized/used.

I'll write up a fix for this.

[ShivanshVij]

The way the statusLine map is generated is extremely perforamant, as all the string and byte conversions are done in the init function in status.go.

What I'll need to do to fix this is create a function line this:

func formatStatusLine(statusCode int, statusMessage []byte) (ret []byte) {
	ret = append(httpHeader, ' ')
	ret = append(ret, []byte(strconv.Itoa(statusCode))...)
	ret = append(ret, ' ')
	ret = append(ret, statusMessage...)
	ret = append(ret, strCRLF...)
	return
}

But when this function gets called in the AppendBytes function there will be a performance hit.

The best way around this in my opinion, is to remove the following: https://github.com/valyala/fasthttp/blob/master/header.go#L1883

Another option is to call the formatStatusLine function in the snipped above inside SetStatusLine, which would also be a performance hit since it will get called in the parseFirstLine function (https://github.com/valyala/fasthttp/blob/master/header.go#L1884)

[erikdubbelboer]

Oh wow I totally missed that. I'm proposing either #1134 or #1133 but I'm not sure what is best. SetStatusText() is a simpler API that should do everything that is needed. SetStatusLine() also empowers users to do things like HTTP5/6 if they want for some reason.

I'm learning towards SetStatusText().

[ShivanshVij]

I think your solution #1134 is pretty good, but it misses the bug in ResponseHeader.parseFirstLine (https://github.com/valyala/fasthttp/pull/1134/files#diff-445b7d49a79bb217c576f137077e18262a0a2d58e424239fcc01a437397188f7R1898) which always calls SetStatusText.

I'm a complete idiot and didn't realize the strconv.AppendInt functions were a thing so I've removed my hideous i2b code

[ShivanshVij]

Alright, while I was in there I added the ability to modify the protocol as well

[ShivanshVij]

Added some test cases too, and brought in your fixes for SetStatusTest (which I'm proposing should be called SetStatusMessage since that's how status.go refers to it)

[ShivanshVij]

The main differences are https://github.com/valyala/fasthttp/pull/1135/files#diff-445b7d49a79bb217c576f137077e18262a0a2d58e424239fcc01a437397188f7R1905 which fixes the bug where the bytes.Equal function call would always return false (leading to h.SetStatusLine always getting called)

It adds the SetProtocol and Protocol functions which allows for modifying the protocol type in the future (and test cases to go along with that), and it keeps the formatStatusLine function because I think it makes things a little cleaner

[ShivanshVij]

Btw this is all proposed in #1135

[ShivanshVij]

Your commit (#1133) is also pretty good, but it should also take into account this: https://github.com/valyala/fasthttp/pull/1135/files#diff-445b7d49a79bb217c576f137077e18262a0a2d58e424239fcc01a437397188f7R1905

[erikdubbelboer]

I think your solution #1134 is pretty good, but it misses the bug in ResponseHeader.parseFirstLine (https://github.com/valyala/fasthttp/pull/1134/files#diff-445b7d49a79bb217c576f137077e18262a0a2d58e424239fcc01a437397188f7R1898) which always calls SetStatusText.

What bug? It turns it into the text only so there is no bug there anymore.

We already have a huge API so adding even more methods like Protocol(), SetProtocol() and AppendStatusLine() mighht be a bit much. Fasthttp also only speaks http1/0 and http1/1 so maybe having those SetProtocol functions doesn't really make sense.

I'm still in favor of merging #1134 as is. What would you have against that?

[ShivanshVij]

Maybe I'm missing something, but in this line !bytes.Equal(b[n+1:], statusLine(h.statusCode)) in the parseFirstLine function, the bytes in b[n+1] do not include "HTTP/1.1" or the status code, but the bytes returned by the statusLine function include the full line (including "HTTP/1.1" - so the equality will always be false

[ShivanshVij]

Other than that though, I have nothing against #1134

I think since the request header can set protocols it might make sense to have the response headers able to do that as well (and maybe make the AppendStatusLine function unexported to keep the API clean), but I leave that up to your judgement