Skip to content

Commit c9be6ae

Browse files
neildjoedian
authored andcommitted
[release-branch.go1.21] net/http: send body or close connection on expect-100-continue requests
When sending a request with an "Expect: 100-continue" header, we must send the request body before sending any further requests on the connection. When receiving a non-1xx response to an "Expect: 100-continue" request, send the request body if the connection isn't being closed after processing the response. In other words, if either the request or response contains a "Connection: close" header, then skip sending the request body (because the connection will not be used for further requests), but otherwise send it. Correct a comment on the server-side Expect: 100-continue handling that implied sending the request body is optional. It isn't. For #67555 Fixes #68199 Change-Id: Ia2f12091bee697771087f32ac347509ec5922d54 Reviewed-on: https://go-review.googlesource.com/c/go/+/591255 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Jonathan Amsterdam <[email protected]> (cherry picked from commit cf501e0) Reviewed-on: https://go-review.googlesource.com/c/go/+/595096 Reviewed-by: Joedian Reid <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]>
1 parent ac14d4d commit c9be6ae

File tree

3 files changed

+164
-97
lines changed

3 files changed

+164
-97
lines changed

src/net/http/server.go

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1352,16 +1352,21 @@ func (cw *chunkWriter) writeHeader(p []byte) {
13521352

13531353
// If the client wanted a 100-continue but we never sent it to
13541354
// them (or, more strictly: we never finished reading their
1355-
// request body), don't reuse this connection because it's now
1356-
// in an unknown state: we might be sending this response at
1357-
// the same time the client is now sending its request body
1358-
// after a timeout. (Some HTTP clients send Expect:
1359-
// 100-continue but knowing that some servers don't support
1360-
// it, the clients set a timer and send the body later anyway)
1361-
// If we haven't seen EOF, we can't skip over the unread body
1362-
// because we don't know if the next bytes on the wire will be
1363-
// the body-following-the-timer or the subsequent request.
1364-
// See Issue 11549.
1355+
// request body), don't reuse this connection.
1356+
//
1357+
// This behavior was first added on the theory that we don't know
1358+
// if the next bytes on the wire are going to be the remainder of
1359+
// the request body or the subsequent request (see issue 11549),
1360+
// but that's not correct: If we keep using the connection,
1361+
// the client is required to send the request body whether we
1362+
// asked for it or not.
1363+
//
1364+
// We probably do want to skip reusing the connection in most cases,
1365+
// however. If the client is offering a large request body that we
1366+
// don't intend to use, then it's better to close the connection
1367+
// than to read the body. For now, assume that if we're sending
1368+
// headers, the handler is done reading the body and we should
1369+
// drop the connection if we haven't seen EOF.
13651370
if ecr, ok := w.req.Body.(*expectContinueReader); ok && !ecr.sawEOF.Load() {
13661371
w.closeAfterReply = true
13671372
}

src/net/http/transport.go

Lines changed: 24 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2313,17 +2313,12 @@ func (pc *persistConn) readResponse(rc requestAndChan, trace *httptrace.ClientTr
23132313
return
23142314
}
23152315
resCode := resp.StatusCode
2316-
if continueCh != nil {
2317-
if resCode == 100 {
2318-
if trace != nil && trace.Got100Continue != nil {
2319-
trace.Got100Continue()
2320-
}
2321-
continueCh <- struct{}{}
2322-
continueCh = nil
2323-
} else if resCode >= 200 {
2324-
close(continueCh)
2325-
continueCh = nil
2316+
if continueCh != nil && resCode == StatusContinue {
2317+
if trace != nil && trace.Got100Continue != nil {
2318+
trace.Got100Continue()
23262319
}
2320+
continueCh <- struct{}{}
2321+
continueCh = nil
23272322
}
23282323
is1xx := 100 <= resCode && resCode <= 199
23292324
// treat 101 as a terminal status, see issue 26161
@@ -2346,6 +2341,25 @@ func (pc *persistConn) readResponse(rc requestAndChan, trace *httptrace.ClientTr
23462341
if resp.isProtocolSwitch() {
23472342
resp.Body = newReadWriteCloserBody(pc.br, pc.conn)
23482343
}
2344+
if continueCh != nil {
2345+
// We send an "Expect: 100-continue" header, but the server
2346+
// responded with a terminal status and no 100 Continue.
2347+
//
2348+
// If we're going to keep using the connection, we need to send the request body.
2349+
// Tell writeLoop to skip sending the body if we're going to close the connection,
2350+
// or to send it otherwise.
2351+
//
2352+
// The case where we receive a 101 Switching Protocols response is a bit
2353+
// ambiguous, since we don't know what protocol we're switching to.
2354+
// Conceivably, it's one that doesn't need us to send the body.
2355+
// Given that we'll send the body if ExpectContinueTimeout expires,
2356+
// be consistent and always send it if we aren't closing the connection.
2357+
if resp.Close || rc.req.Close {
2358+
close(continueCh) // don't send the body; the connection will close
2359+
} else {
2360+
continueCh <- struct{}{} // send the body
2361+
}
2362+
}
23492363

23502364
resp.TLS = pc.tlsState
23512365
return

src/net/http/transport_test.go

Lines changed: 125 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -1135,94 +1135,142 @@ func testTransportGzip(t *testing.T, mode testMode) {
11351135
}
11361136
}
11371137

1138-
// If a request has Expect:100-continue header, the request blocks sending body until the first response.
1139-
// Premature consumption of the request body should not be occurred.
1140-
func TestTransportExpect100Continue(t *testing.T) {
1141-
run(t, testTransportExpect100Continue, []testMode{http1Mode})
1138+
// A transport100Continue test exercises Transport behaviors when sending a
1139+
// request with an Expect: 100-continue header.
1140+
type transport100ContinueTest struct {
1141+
t *testing.T
1142+
1143+
reqdone chan struct{}
1144+
resp *Response
1145+
respErr error
1146+
1147+
conn net.Conn
1148+
reader *bufio.Reader
11421149
}
1143-
func testTransportExpect100Continue(t *testing.T, mode testMode) {
1144-
ts := newClientServerTest(t, mode, HandlerFunc(func(rw ResponseWriter, req *Request) {
1145-
switch req.URL.Path {
1146-
case "/100":
1147-
// This endpoint implicitly responds 100 Continue and reads body.
1148-
if _, err := io.Copy(io.Discard, req.Body); err != nil {
1149-
t.Error("Failed to read Body", err)
1150-
}
1151-
rw.WriteHeader(StatusOK)
1152-
case "/200":
1153-
// Go 1.5 adds Connection: close header if the client expect
1154-
// continue but not entire request body is consumed.
1155-
rw.WriteHeader(StatusOK)
1156-
case "/500":
1157-
rw.WriteHeader(StatusInternalServerError)
1158-
case "/keepalive":
1159-
// This hijacked endpoint responds error without Connection:close.
1160-
_, bufrw, err := rw.(Hijacker).Hijack()
1161-
if err != nil {
1162-
log.Fatal(err)
1163-
}
1164-
bufrw.WriteString("HTTP/1.1 500 Internal Server Error\r\n")
1165-
bufrw.WriteString("Content-Length: 0\r\n\r\n")
1166-
bufrw.Flush()
1167-
case "/timeout":
1168-
// This endpoint tries to read body without 100 (Continue) response.
1169-
// After ExpectContinueTimeout, the reading will be started.
1170-
conn, bufrw, err := rw.(Hijacker).Hijack()
1171-
if err != nil {
1172-
log.Fatal(err)
1173-
}
1174-
if _, err := io.CopyN(io.Discard, bufrw, req.ContentLength); err != nil {
1175-
t.Error("Failed to read Body", err)
1176-
}
1177-
bufrw.WriteString("HTTP/1.1 200 OK\r\n\r\n")
1178-
bufrw.Flush()
1179-
conn.Close()
1180-
}
11811150

1182-
})).ts
1151+
const transport100ContinueTestBody = "request body"
11831152

1184-
tests := []struct {
1185-
path string
1186-
body []byte
1187-
sent int
1188-
status int
1189-
}{
1190-
{path: "/100", body: []byte("hello"), sent: 5, status: 200}, // Got 100 followed by 200, entire body is sent.
1191-
{path: "/200", body: []byte("hello"), sent: 0, status: 200}, // Got 200 without 100. body isn't sent.
1192-
{path: "/500", body: []byte("hello"), sent: 0, status: 500}, // Got 500 without 100. body isn't sent.
1193-
{path: "/keepalive", body: []byte("hello"), sent: 0, status: 500}, // Although without Connection:close, body isn't sent.
1194-
{path: "/timeout", body: []byte("hello"), sent: 5, status: 200}, // Timeout exceeded and entire body is sent.
1153+
// newTransport100ContinueTest creates a Transport and sends an Expect: 100-continue
1154+
// request on it.
1155+
func newTransport100ContinueTest(t *testing.T, timeout time.Duration) *transport100ContinueTest {
1156+
ln := newLocalListener(t)
1157+
defer ln.Close()
1158+
1159+
test := &transport100ContinueTest{
1160+
t: t,
1161+
reqdone: make(chan struct{}),
11951162
}
11961163

1197-
c := ts.Client()
1198-
for i, v := range tests {
1199-
tr := &Transport{
1200-
ExpectContinueTimeout: 2 * time.Second,
1201-
}
1202-
defer tr.CloseIdleConnections()
1203-
c.Transport = tr
1204-
body := bytes.NewReader(v.body)
1205-
req, err := NewRequest("PUT", ts.URL+v.path, body)
1206-
if err != nil {
1207-
t.Fatal(err)
1208-
}
1164+
tr := &Transport{
1165+
ExpectContinueTimeout: timeout,
1166+
}
1167+
go func() {
1168+
defer close(test.reqdone)
1169+
body := strings.NewReader(transport100ContinueTestBody)
1170+
req, _ := NewRequest("PUT", "http://"+ln.Addr().String(), body)
12091171
req.Header.Set("Expect", "100-continue")
1210-
req.ContentLength = int64(len(v.body))
1172+
req.ContentLength = int64(len(transport100ContinueTestBody))
1173+
test.resp, test.respErr = tr.RoundTrip(req)
1174+
test.resp.Body.Close()
1175+
}()
12111176

1212-
resp, err := c.Do(req)
1213-
if err != nil {
1214-
t.Fatal(err)
1177+
c, err := ln.Accept()
1178+
if err != nil {
1179+
t.Fatalf("Accept: %v", err)
1180+
}
1181+
t.Cleanup(func() {
1182+
c.Close()
1183+
})
1184+
br := bufio.NewReader(c)
1185+
_, err = ReadRequest(br)
1186+
if err != nil {
1187+
t.Fatalf("ReadRequest: %v", err)
1188+
}
1189+
test.conn = c
1190+
test.reader = br
1191+
t.Cleanup(func() {
1192+
<-test.reqdone
1193+
tr.CloseIdleConnections()
1194+
got, _ := io.ReadAll(test.reader)
1195+
if len(got) > 0 {
1196+
t.Fatalf("Transport sent unexpected bytes: %q", got)
12151197
}
1216-
resp.Body.Close()
1198+
})
12171199

1218-
sent := len(v.body) - body.Len()
1219-
if v.status != resp.StatusCode {
1220-
t.Errorf("test %d: status code should be %d but got %d. (%s)", i, v.status, resp.StatusCode, v.path)
1221-
}
1222-
if v.sent != sent {
1223-
t.Errorf("test %d: sent body should be %d but sent %d. (%s)", i, v.sent, sent, v.path)
1200+
return test
1201+
}
1202+
1203+
// respond sends response lines from the server to the transport.
1204+
func (test *transport100ContinueTest) respond(lines ...string) {
1205+
for _, line := range lines {
1206+
if _, err := test.conn.Write([]byte(line + "\r\n")); err != nil {
1207+
test.t.Fatalf("Write: %v", err)
12241208
}
12251209
}
1210+
if _, err := test.conn.Write([]byte("\r\n")); err != nil {
1211+
test.t.Fatalf("Write: %v", err)
1212+
}
1213+
}
1214+
1215+
// wantBodySent ensures the transport has sent the request body to the server.
1216+
func (test *transport100ContinueTest) wantBodySent() {
1217+
got, err := io.ReadAll(io.LimitReader(test.reader, int64(len(transport100ContinueTestBody))))
1218+
if err != nil {
1219+
test.t.Fatalf("unexpected error reading body: %v", err)
1220+
}
1221+
if got, want := string(got), transport100ContinueTestBody; got != want {
1222+
test.t.Fatalf("unexpected body: got %q, want %q", got, want)
1223+
}
1224+
}
1225+
1226+
// wantRequestDone ensures the Transport.RoundTrip has completed with the expected status.
1227+
func (test *transport100ContinueTest) wantRequestDone(want int) {
1228+
<-test.reqdone
1229+
if test.respErr != nil {
1230+
test.t.Fatalf("unexpected RoundTrip error: %v", test.respErr)
1231+
}
1232+
if got := test.resp.StatusCode; got != want {
1233+
test.t.Fatalf("unexpected response code: got %v, want %v", got, want)
1234+
}
1235+
}
1236+
1237+
func TestTransportExpect100ContinueSent(t *testing.T) {
1238+
test := newTransport100ContinueTest(t, 1*time.Hour)
1239+
// Server sends a 100 Continue response, and the client sends the request body.
1240+
test.respond("HTTP/1.1 100 Continue")
1241+
test.wantBodySent()
1242+
test.respond("HTTP/1.1 200", "Content-Length: 0")
1243+
test.wantRequestDone(200)
1244+
}
1245+
1246+
func TestTransportExpect100Continue200ResponseNoConnClose(t *testing.T) {
1247+
test := newTransport100ContinueTest(t, 1*time.Hour)
1248+
// No 100 Continue response, no Connection: close header.
1249+
test.respond("HTTP/1.1 200", "Content-Length: 0")
1250+
test.wantBodySent()
1251+
test.wantRequestDone(200)
1252+
}
1253+
1254+
func TestTransportExpect100Continue200ResponseWithConnClose(t *testing.T) {
1255+
test := newTransport100ContinueTest(t, 1*time.Hour)
1256+
// No 100 Continue response, Connection: close header set.
1257+
test.respond("HTTP/1.1 200", "Connection: close", "Content-Length: 0")
1258+
test.wantRequestDone(200)
1259+
}
1260+
1261+
func TestTransportExpect100Continue500ResponseNoConnClose(t *testing.T) {
1262+
test := newTransport100ContinueTest(t, 1*time.Hour)
1263+
// No 100 Continue response, no Connection: close header.
1264+
test.respond("HTTP/1.1 500", "Content-Length: 0")
1265+
test.wantBodySent()
1266+
test.wantRequestDone(500)
1267+
}
1268+
1269+
func TestTransportExpect100Continue500ResponseTimeout(t *testing.T) {
1270+
test := newTransport100ContinueTest(t, 5*time.Millisecond) // short timeout
1271+
test.wantBodySent() // after timeout
1272+
test.respond("HTTP/1.1 200", "Content-Length: 0")
1273+
test.wantRequestDone(200)
12261274
}
12271275

12281276
func TestSOCKS5Proxy(t *testing.T) {

0 commit comments

Comments
 (0)