net/http: After HTTP write StatusSwitchingProtocols, calling Hijack doesn't clean up req.Body data

[lujinda]

What version of Go are you using (go version)?

$ go version
go version go1.19.5 linux/amd64

Does this issue reproduce with the latest release?

The issue with the latest code of master still exists

What operating system and processor architecture are you using (go env)?

go env Output

$ go env

What did you do?

When Containerd processes exec requests, it will upgrade the HTTP protocol to SPDY through HTTP Upgrade.

// UpgradeResponse upgrades an HTTP response to one that supports multiplexed
// streams. newStreamHandler will be called synchronously whenever the
// other end of the upgraded connection creates a new stream.
func (u responseUpgrader) UpgradeResponse(w http.ResponseWriter, req *http.Request, newStreamHandler httpstream.NewStreamHandler) httpstream.Connection {
	connectionHeader := strings.ToLower(req.Header.Get(httpstream.HeaderConnection))
	upgradeHeader := strings.ToLower(req.Header.Get(httpstream.HeaderUpgrade))
	if !strings.Contains(connectionHeader, strings.ToLower(httpstream.HeaderUpgrade)) || !strings.Contains(upgradeHeader, strings.ToLower(HeaderSpdy31)) {
		errorMsg := fmt.Sprintf("unable to upgrade: missing upgrade headers in request: %#v", req.Header)
		http.Error(w, errorMsg, http.StatusBadRequest)
		return nil
	}

	hijacker, ok := w.(http.Hijacker)
	if !ok {
		errorMsg := fmt.Sprintf("unable to upgrade: unable to hijack response")
		http.Error(w, errorMsg, http.StatusInternalServerError)
		return nil
	}

	w.Header().Add(httpstream.HeaderConnection, httpstream.HeaderUpgrade)
	w.Header().Add(httpstream.HeaderUpgrade, HeaderSpdy31)
	w.WriteHeader(http.StatusSwitchingProtocols)

	conn, bufrw, err := hijacker.Hijack()
	if err != nil {
		runtime.HandleError(fmt.Errorf("unable to upgrade: error hijacking response: %v", err))
		return nil
	}

In the Hijack function, if writeHeader==true , w.cw.flush() will be called

go/src/net/http/server.go

Lines 2075 to 2077 in 24f83ed

	if w.wroteHeader {
	w.cw.flush()
	}

and the writeHeader in the flush function will eventually clear the content of req.Body:

go/src/net/http/server.go

Lines 397 to 402 in 2c95fa4

	func (cw *chunkWriter) flush() error {
	if !cw.wroteHeader {
	cw.writeHeader(nil)
	}
	return cw.res.conn.bufw.Flush()
	}

go/src/net/http/server.go

Lines 1401 to 1422 in 2c95fa4

	if discard {
	_, err := io.CopyN(io.Discard, w.reqBody, maxPostHandlerReadBytes+1)
	switch err {
	case nil:
	// There must be even more data left over.
	tooBig = true
	case ErrBodyReadAfterClose:
	// Body was already consumed and closed.
	case io.EOF:
	// The remaining body was just consumed, close it.
	err = w.reqBody.Close()
	if err != nil {
	w.closeAfterReply = true
	}
	default:
	// Some other kind of error occurred, like a read timeout, or
	// corrupt chunked encoding. In any case, whatever remains
	// on the wire must not be parsed as another HTTP request.
	w.closeAfterReply = true
	}
	}

So if you want to clear req.Body, writeHeader must be true. writeHeader is set in the (w *response) WriteHeader function. But if the code is 101, it will not be set. So in our scene, writeHeader is always false.

go/src/net/http/server.go

Lines 1143 to 1181 in 24f83ed

	func (w *response) WriteHeader(code int) {
	if w.conn.hijacked() {
	caller := relevantCaller()
	w.conn.server.logf("http: response.WriteHeader on hijacked connection from %s (%s:%d)", caller.Function, path.Base(caller.File), caller.Line)
	return
	}
	if w.wroteHeader {
	caller := relevantCaller()
	w.conn.server.logf("http: superfluous response.WriteHeader call from %s (%s:%d)", caller.Function, path.Base(caller.File), caller.Line)
	return
	}
	checkWriteHeaderCode(code)
	// Handle informational headers.
	//
	// We shouldn't send any further headers after 101 Switching Protocols,
	// so it takes the non-informational path.
	if code >= 100 && code <= 199 && code != StatusSwitchingProtocols {
	// Prevent a potential race with an automatically-sent 100 Continue triggered by Request.Body.Read()
	if code == 100 && w.canWriteContinue.Load() {
	w.writeContinueMu.Lock()
	w.canWriteContinue.Store(false)
	w.writeContinueMu.Unlock()
	}
	writeStatusLine(w.conn.bufw, w.req.ProtoAtLeast(1, 1), code, w.statusBuf[:])
	// Per RFC 8297 we must not clear the current header map
	w.handlerHeader.WriteSubset(w.conn.bufw, excludedHeadersNoBody)
	w.conn.bufw.Write(crlf)
	w.conn.bufw.Flush()
	return
	}
	w.wroteHeader = true
	w.status = code
	if w.calledHeader && w.cw.header == nil {

If there is data in the HTTP Body (such as Transfer-Encoding: chunked time, Body is 0\r\n\r\n), the raw conn obtained by Hijack will always have dirty data, resulting in the upgraded protocol (such as SPDY, WebSocket) have problems.

Can we modify Hijack like this?

// Hijack implements the Hijacker.Hijack method. Our response is both a ResponseWriter
// and a Hijacker.
func (w *response) Hijack() (rwc net.Conn, buf *bufio.ReadWriter, err error) {
	if w.handlerDone.isSet() {
		panic("net/http: Hijack called after ServeHTTP finished")
	}
	// if w.wroteHeader {
	w.cw.flush() // Always call flush
	// }

What did you expect to see?

What did you see instead?

[bcmills]

The documentation for net/http.Hijacker explicitly says:

 // The returned bufio.Reader may contain unprocessed buffered
 // data from the client.

Does that not cover exactly this situation?

(CC @neild)

[lujinda]

The documentation for net/http.Hijacker explicitly says:

 // The returned bufio.Reader may contain unprocessed buffered
 // data from the client.

Does that not cover exactly this situation?

(CC @neild)

Thank you for your reply.
Also as described in the documentation:

// After a call to Hijack, the original Request.Body must not
// be used.

So I think that after called Hijack, the original content of req.Body is no longer meaningful?

thx

[neild]

If you want to remove the original request body from the connection before hijacking it, you can drain it yourself by reading Request.Body to completion before calling Hijack. (For example, io.Copy(io.Discard, req.Body).)

Hijacking a connection takes over ownership of the socket from net/http. Reading from Request.Body can read from the socket, so you must not read from the body after hijacking.

[seankhliao]

looks like it's working as intended