[net] http2/h2c: handle request bodies during h2c connection upgrading
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded this change for review.
http2/h2c: handle request bodies during h2c connection upgrading
If a request that triggered an upgrade from HTTP/1.1 -> HTTP/2
contained a body, it would not be replayed by the server as a HTTP/2 data frame.
This would result in hangs as the client would get no data back,
as the request body was never actually handled.
This code corrects this, and sends HTTP/2 DATA frames with the request body.
As an example:
Client:
```
$ curl -v --http2 -d 'POST BODY' http://localhost:5555
* Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 5555 (#0)
> POST / HTTP/1.1
> Host: localhost:5555
> User-Agent: curl/7.64.1
> Accept: */*
> Connection: Upgrade, HTTP2-Settings
> Upgrade: h2c
> HTTP2-Settings: AAMAAABkAARAAAAAAAIAAAAA
> Content-Length: 9
> Content-Type: application/x-www-form-urlencoded
>
* upload completely sent off: 9 out of 9 bytes
< HTTP/1.1 101 Switching Protocols
< Connection: Upgrade
< Upgrade: h2c
* Received 101
* Using HTTP2, server supports multi-use
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* Connection state changed (MAX_CONCURRENT_STREAMS == 250)!
< HTTP/2 200
< content-length: 0
< date: Sat, 29 Jan 2022 06:51:05 GMT
<
* Connection #0 to host localhost left intact
* Closing connection 0
```
Echo server:
```
$ ./bin/h2test
Listening [0.0.0.0:5555]...
Request: {Method:POST URL:/ Proto:HTTP/2.0 ProtoMajor:2 ProtoMinor:0 Header:map[Accept:[*/*]
Content-Length:[9] Content-Type:[application/x-www-form-urlencoded] User-Agent:[curl/7.64.1]]
Body:0xc000098120 GetBody:<nil> ContentLength:9 TransferEncoding:[] Close:false Host:localhost:5555
Form:map[] PostForm:map[] MultipartForm:<nil> Trailer:map[] RemoteAddr:127.0.0.1:54540 RequestURI:/
TLS:<nil> Cancel:<nil> Response:<nil> ctx:0xc0000a0000}
Received body: POST BODY
```
Fixes #38064
Change-Id: I52b6115a3c9551f7802f0c7c18d948dbf34f81be
GitHub-Last-Rev: 189b99aa2b1cd8ee48efdeba6592902c5e28604e
GitHub-Pull-Request: golang/net#128
---
M http2/h2c/h2c.go
M http2/h2c/h2c_test.go
2 files changed, 176 insertions(+), 0 deletions(-)
diff --git a/http2/h2c/h2c.go b/http2/h2c/h2c.go
index c0970d8..3dba917 100644
--- a/http2/h2c/h2c.go
+++ b/http2/h2c/h2c.go
@@ -249,6 +249,38 @@
}
}
+ // Any request body create as DATA frames
+ if r.Body != nil && r.Body != http.NoBody {
+ body, err := io.ReadAll(r.Body)
+ if err != nil {
+ return nil, nil, fmt.Errorf("Could not read request body: %v", err)
+ }
+
+ needOneDataFrame := len(body) < maxFrameSize
+ err = framer.WriteData(1,
+ needOneDataFrame, // end stream?
+ body)
+ if err != nil {
+ return nil, nil, err
+ }
+
+ for i := maxFrameSize; i < len(body); i += maxFrameSize {
+ if len(body)-i > maxFrameSize {
+ if err := framer.WriteData(1,
+ false, // end stream?
+ body[i:maxFrameSize]); err != nil {
+ return nil, nil, err
+ }
+ } else {
+ if err := framer.WriteData(1,
+ true, // end stream?
+ body[i:]); err != nil {
+ return nil, nil, err
+ }
+ }
+ }
+ }
+
return h2Bytes, settings, nil
}
diff --git a/http2/h2c/h2c_test.go b/http2/h2c/h2c_test.go
index bd9461e..201046e 100644
--- a/http2/h2c/h2c_test.go
+++ b/http2/h2c/h2c_test.go
@@ -104,3 +104,79 @@
t.Fatal(err)
}
}
+
+func Test_convertH1ReqToH2_with_POST(t *testing.T) {
+ postBody := "Some POST Body"
+
+ r, err := http.NewRequest("POST", "http://localhost:80", bytes.NewBufferString(postBody))
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ r.Header.Set("Upgrade", "h2c")
+ r.Header.Set("Connection", "Upgrade, HTTP2-Settings")
+ r.Header.Set("HTTP2-Settings", "AAEAAEAAAAIAAAABAAMAAABkAAQBAAAAAAUAAEAA") // Some Default Settings
+ h2Bytes, _, err := convertH1ReqToH2(r)
+
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ // Read off the preface
+ preface := []byte(http2.ClientPreface)
+ if h2Bytes.Len() < len(preface) {
+ t.Fatal("Could not read HTTP/2 ClientPreface")
+ }
+ readPreface := h2Bytes.Next(len(preface))
+ if string(readPreface) != http2.ClientPreface {
+ t.Fatalf("Expected Preface %s but got: %s", http2.ClientPreface, string(readPreface))
+ }
+
+ framer := http2.NewFramer(nil, h2Bytes)
+
+ // Should get a SETTINGS, HEADERS, and then DATA
+ expectedFrameTypes := []http2.FrameType{http2.FrameSettings, http2.FrameHeaders, http2.FrameData}
+ for frameNumber := 0; h2Bytes.Len() > 0; {
+ frame, err := framer.ReadFrame()
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ if frameNumber >= len(expectedFrameTypes) {
+ t.Errorf("Got more than %d frames, wanted only %d", len(expectedFrameTypes), len(expectedFrameTypes))
+ }
+
+ if frame.Header().Type != expectedFrameTypes[frameNumber] {
+ t.Errorf("Got FrameType %v, wanted %v", frame.Header().Type, expectedFrameTypes[frameNumber])
+ }
+
+ frameNumber += 1
+
+ switch f := frame.(type) {
+ case *http2.SettingsFrame:
+ if frameNumber != 1 {
+ t.Errorf("Got SETTINGS frame as frame #%d, wanted it as frame #1", frameNumber)
+ }
+ case *http2.HeadersFrame:
+ if frameNumber != 2 {
+ t.Errorf("Got HEADERS frame as frame #%d, wanted it as frame #2", frameNumber)
+ }
+ if f.FrameHeader.StreamID != 1 {
+ t.Fatalf("Expected StreamId 1, got %v", f.FrameHeader.StreamID)
+ }
+ case *http2.DataFrame:
+ if frameNumber != 3 {
+ t.Errorf("Got DATA frame as frame #%d, wanted it as frame #3", frameNumber)
+ }
+ if f.FrameHeader.StreamID != 1 {
+ t.Errorf("Got StreamID %v, wanted 1", f.FrameHeader.StreamID)
+ }
+
+ body := string(f.Data())
+
+ if body != postBody {
+ t.Errorf("Got DATA body %s, wanted %s", body, postBody)
+ }
+ }
+ }
+}
To view, visit change 383114. To unsubscribe, or for help writing mail filters, visit settings.
Ingo Oeser (Gerrit)
Attention is currently required from: Damien Neil, Tom Bergan.
2 comments:
Patchset:
Noticed an issue when streaming large request bodies.
File http2/h2c/h2c.go:
Patch Set #1, Line 254:
body, err := io.ReadAll(r.Body)io.ReadAll is dangerous here as that might read gigabytes of data into memory leading to a crash due to out of memory. Please use a read until maxFrameSize only.
I wonder whether this all could look more like the core of io.CopyBuffer or even use that.
To view, visit change 383114. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Damien Neil, Tom Bergan.
Gerrit Bot uploaded patch set #2 to this change.
GitHub-Last-Rev: ba79222830de46c3accddb442b867c4ca37a7635
GitHub-Pull-Request: golang/net#128
---
M http2/h2c/h2c.go
M http2/h2c/h2c_test.go
2 files changed, 268 insertions(+), 0 deletions(-)
To view, visit change 383114. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Tom Bergan.
Patch set 2:Code-Review -1
1 comment:
File http2/h2c/h2c.go:
Patch Set #2, Line 255:
for {This no longer uses io.ReadAll, but it still reads the entire request body into memory; it's still as dangerous as before.
I don't think this is the right approach. We can't read the entire request. Converting an HTTP/1 request body into an HTTP/2 one seems very tricky; while I haven't dug into this code to verify it, I strongly suspect that flow control will get very confused: The HTTP/2 server will think that the initial HTTP/1 request is consuming flow control tokens.
I wonder if the correct approach is to stop trying to translate the initial HTTP/1.1 request into HTTP/2.
To view, visit change 383114. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Ingo Oeser, Tom Bergan, James Lamanna.
1 comment:
File http2/h2c/h2c.go:
Patch Set #2, Line 255:
for {
This only reads maxFrameSize at a time, it does not try and read the entire body if its > maxFrameSi […]
Unless I'm misunderstanding something, this loops for as long as n==maxFrameSize, calling framer.WriteData for each frame. WriteData buffers the data.
To view, visit change 383114. To unsubscribe, or for help writing mail filters, visit settings.
James Lamanna (Gerrit)
Attention is currently required from: Ingo Oeser, Damien Neil, Tom Bergan.
2 comments:
File http2/h2c/h2c.go:
Patch Set #1, Line 254:
body, err := io.ReadAll(r.Body)
io. […]
Updated in rev2.
File http2/h2c/h2c.go:
Patch Set #2, Line 255:
for {
This no longer uses io. […]
This only reads maxFrameSize at a time, it does not try and read the entire body if its > maxFrameSize.
To view, visit change 383114. To unsubscribe, or for help writing mail filters, visit settings.
James Lamanna (Gerrit)
Attention is currently required from: Ingo Oeser, Damien Neil, Tom Bergan.
1 comment:
File http2/h2c/h2c.go:
Patch Set #2, Line 255:
for {
Unless I'm misunderstanding something, this loops for as long as n==maxFrameSize, calling framer. […]
WriteData() calls ioWriter.Write(). If you consider that buffering, than sure, but I'm not sure how else you would ever write out large amounts of data either here or in a different DATA frame path.
To view, visit change 383114. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Ingo Oeser, Tom Bergan, James Lamanna.
1 comment:
File http2/h2c/h2c.go:
Patch Set #2, Line 255:
for {
WriteData() calls ioWriter.Write(). […]
framer.WriteData calls framer.w.Write. f.w is a bytes.Buffer created on line 209 of this file. Writing to the framer buffers the data into that bytes.Buffer.
Or am I missing something?
To view, visit change 383114. To unsubscribe, or for help writing mail filters, visit settings.