Skip to content

Commit 90d10f0

Browse files
nicholashusinnicholashusin
committed
internal/http3: delete invalid Content-Length if declared in server handler
Following our HTTP/1 and HTTP/2 server implementations, when a Content-Length header is set to an invalid value in a server handler, it should be stripped. Also change responseWriter.bodyLenLeft to be int64, to avoid possible overflow shenanigan on 32-bit systems. For golang/go#70914 Change-Id: I76174d81e7b5993b62a32235ba5008f76a6a6964 Reviewed-on: https://go-review.googlesource.com/c/net/+/813560 Reviewed-by: Nicholas Husin <husin@google.com> LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Damien Neil <dneil@google.com>
1 parent 08abf4d commit 90d10f0

2 files changed

Lines changed: 84 additions & 43 deletions

File tree

‎internal/http3/server.go‎

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -507,7 +507,7 @@ func (sc *serverConn) handleRequestStream(st *stream) error {
507507
}
508508

509509
contentLength := int64(-1)
510-
if n, err := strconv.Atoi(header.Get("Content-Length")); err == nil {
510+
if n, err := strconv.ParseUint(header.Get("Content-Length"), 10, 63); err == nil {
511511
contentLength = int64(n)
512512
}
513513

@@ -603,11 +603,11 @@ type responseWriter struct {
603603
snapHeaders http.Header // Snapshot of headers at WriteHeader time
604604
trailer http.Header
605605
bb bodyBuffer
606-
wroteHeader bool // Non-1xx header has been (logically) written.
607-
statusCode int // Non-1xx status of the response that will be sent in HEADERS frame. Zero means none has been set.
608-
sent100 bool // Status 100 has been sent by the server.
609-
cannotHaveBody bool // Response should not have a body (e.g. response to a HEAD request).
610-
bodyLenLeft int // How much of the content body is left to be sent, set via "Content-Length" header. -1 if unknown.
606+
wroteHeader bool // Non-1xx header has been (logically) written.
607+
statusCode int // Non-1xx status of the response that will be sent in HEADERS frame. Zero means none has been set.
608+
sent100 bool // Status 100 has been sent by the server.
609+
cannotHaveBody bool // Response should not have a body (e.g. response to a HEAD request).
610+
bodyLenLeft int64 // How much of the content body is left to be sent, set via "Content-Length" header. -1 if unknown.
611611
}
612612

613613
func (rw *responseWriter) Header() http.Header {
@@ -755,13 +755,14 @@ func (rw *responseWriter) WriteHeader(statusCode int) {
755755

756756
// Non-informational headers should only be set once, and should be
757757
// buffered.
758-
rw.statusCode = statusCode
759-
rw.snapHeaders = rw.headers.Clone()
760-
if n, err := strconv.Atoi(rw.Header().Get("Content-Length")); err == nil {
761-
rw.bodyLenLeft = n
758+
if n, err := strconv.ParseUint(rw.headers.Get("Content-Length"), 10, 63); err == nil {
759+
rw.bodyLenLeft = int64(n)
762760
} else {
761+
rw.headers.Del("Content-Length")
763762
rw.bodyLenLeft = -1 // Unknown.
764763
}
764+
rw.statusCode = statusCode
765+
rw.snapHeaders = rw.headers.Clone()
765766
}
766767

767768
// trimWriteLocked trims a byte slice, b, such that the length of b will not
@@ -772,9 +773,9 @@ func (rw *responseWriter) trimWriteLocked(b []byte) ([]byte, bool) {
772773
if rw.bodyLenLeft < 0 {
773774
return b, false
774775
}
775-
n := min(len(b), rw.bodyLenLeft)
776+
n := min(int64(len(b)), rw.bodyLenLeft)
776777
rw.bodyLenLeft -= n
777-
return b[:n], n != len(b)
778+
return b[:n], n != int64(len(b))
778779
}
779780

780781
func (rw *responseWriter) Write(b []byte) (n int, err error) {

‎internal/http3/server_test.go‎

Lines changed: 71 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -570,48 +570,80 @@ func TestServerHandlerStreaming(t *testing.T) {
570570
})
571571
}
572572

573-
func TestServerHandlerTrimsContentBody(t *testing.T) {
573+
func TestServerHandlerDeclaresContentLength(t *testing.T) {
574574
tests := []struct {
575-
name string
576-
declaredContentLen int
577-
declaredInvalidContentLen bool
578-
actualContentLen int
579-
wantTrimmed bool
575+
name string
576+
contentLen string
577+
actualContentLen int
578+
wantWrittenLen int
579+
wantTrimmed bool
580+
wantCLHeader bool
580581
}{
581582
{
582-
name: "declared accurate content length",
583-
declaredContentLen: 100,
584-
actualContentLen: 100,
583+
name: "accurate content length",
584+
contentLen: "100",
585+
actualContentLen: 100,
586+
wantWrittenLen: 100,
587+
wantCLHeader: true,
585588
},
586589
{
587-
name: "declared larger content length",
588-
declaredContentLen: 100,
589-
actualContentLen: 10,
590+
name: "larger content length",
591+
contentLen: "100",
592+
actualContentLen: 10,
593+
wantWrittenLen: 10,
594+
wantCLHeader: true,
590595
},
591596
{
592-
name: "declared smaller content length",
593-
declaredContentLen: 10,
594-
actualContentLen: 100,
595-
wantTrimmed: true,
597+
name: "smaller content length",
598+
contentLen: "10",
599+
actualContentLen: 100,
600+
wantWrittenLen: 10,
601+
wantTrimmed: true,
602+
wantCLHeader: true,
596603
},
597604
{
598-
name: "declared invalid content length",
599-
declaredInvalidContentLen: true,
600-
actualContentLen: 100,
605+
name: "non-numeric string",
606+
contentLen: "intentional gibberish",
607+
actualContentLen: 100,
608+
wantWrittenLen: 100,
609+
},
610+
{
611+
name: "negative number",
612+
contentLen: "-10",
613+
actualContentLen: 100,
614+
wantWrittenLen: 100,
615+
},
616+
{
617+
name: "plus sign",
618+
contentLen: "+10",
619+
actualContentLen: 100,
620+
wantWrittenLen: 100,
621+
},
622+
{
623+
name: "empty",
624+
contentLen: "",
625+
actualContentLen: 100,
626+
wantWrittenLen: 100,
627+
},
628+
{
629+
name: "large valid content length",
630+
contentLen: "3000000000",
631+
actualContentLen: 100,
632+
wantWrittenLen: 100,
633+
wantCLHeader: true,
634+
},
635+
{
636+
name: "content length overflowing int64",
637+
contentLen: "9223372036854775808",
638+
actualContentLen: 100,
639+
wantWrittenLen: 100,
601640
},
602641
}
603642

604643
for _, tt := range tests {
605-
wantWrittenLen := min(tt.actualContentLen, tt.declaredContentLen)
606-
if tt.declaredInvalidContentLen {
607-
wantWrittenLen = tt.actualContentLen
608-
}
609644
synctestSubtest(t, tt.name, func(t *testing.T) {
610645
ts := newTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
611-
w.Header().Set("Content-Length", strconv.Itoa(tt.declaredContentLen))
612-
if tt.declaredInvalidContentLen {
613-
w.Header().Set("Content-Length", "not a number, should be ignored")
614-
}
646+
w.Header().Set("Content-Length", tt.contentLen)
615647
var written int
616648
var lastErr error
617649
for range tt.actualContentLen {
@@ -622,17 +654,25 @@ func TestServerHandlerTrimsContentBody(t *testing.T) {
622654
if tt.wantTrimmed != (lastErr != nil) {
623655
t.Errorf("got %v error when writing response body, even though wantTrimmed is %v", lastErr, tt.wantTrimmed)
624656
}
625-
if written != wantWrittenLen {
626-
t.Errorf("got %v bytes written by the server, want %v bytes", written, wantWrittenLen)
657+
if written != tt.wantWrittenLen {
658+
t.Errorf("got %v bytes written by the server, want %v bytes", written, tt.wantWrittenLen)
627659
}
628660
}))
629661
tc := ts.connect()
630662
tc.greet()
631663

632664
reqStream := tc.newStream(streamTypeRequest)
633665
reqStream.writeHeaders(requestHeader(nil))
634-
reqStream.wantHeaders(nil)
635-
reqStream.wantData(slices.Repeat([]byte("a"), wantWrittenLen))
666+
expectedHeaders := http.Header{
667+
":status": {"200"},
668+
"Content-Type": {"text/plain; charset=utf-8"},
669+
"Date": {"Sat, 01 Jan 2000 00:00:00 GMT"}, // Synctest starting time.
670+
}
671+
if tt.wantCLHeader {
672+
expectedHeaders.Set("Content-Length", tt.contentLen)
673+
}
674+
reqStream.wantHeaders(expectedHeaders)
675+
reqStream.wantData(slices.Repeat([]byte("a"), tt.wantWrittenLen))
636676
reqStream.wantClosed("request is complete")
637677
})
638678
}

0 commit comments

Comments
 (0)