Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
lujinda opened this issue Aug 8, 2023 · 4 comments
Closed

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

lujinda opened this issue Aug 8, 2023 · 4 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone
Unplanned

Comments

@lujinda
Copy link

lujinda commented Aug 8, 2023

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 bcmills changed the title After HTTP write StatusSwitchingProtocols, calling Hijack doesn't clean up req.Body data net/http: After HTTP write StatusSwitchingProtocols, calling Hijack doesn't clean up req.Body data Aug 8, 2023
@bcmills
Copy link
Contributor

bcmills commented Aug 8, 2023

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)

@bcmills bcmills added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 8, 2023
@bcmills bcmills added this to the Unplanned milestone Aug 8, 2023
@lujinda
Copy link
Author

lujinda commented Aug 8, 2023

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
Copy link
Contributor

neild commented Aug 8, 2023

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
Copy link
Member

looks like it's working as intended

@seankhliao seankhliao closed this as not planned Won't fix, can't repro, duplicate, stale Sep 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
4 participants
@neild @bcmills @lujinda @seankhliao