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.

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

x/net/http2: Server can write invalid HTTP/2 if handler writes before reading 100-continue request body #14030

Closed
bradfitz opened this issue Jan 20, 2016 · 6 comments
Assignees
Milestone

Comments

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jan 20, 2016

I had this in an old local dev branch, never completed:

 func (sc *serverConn) writeFrame(wm frameWriteMsg) {
        sc.serveG.check()
+
+       // http2 handlers can conccurently read and write.
+       // If they already wrote headers, ignore a 100-continue
+       // header generated by the reader part.
+       println(fmt.Sprintf("Frame write for %v of type %T", wm.stream, wm.write))
+       switch wm.write.(type) {
+       case *writeResHeaders:
+               wm.stream.wroteHeaders = true
+       case write100ContinueHeadersFrame:
+               if wm.stream.wroteHeaders {
+                       sc.scheduleFrameWrite()
+                       return
+               }
+       }
+

The http2 Server supports an http.Handler concurrently reading the request body while the response body is also being streamed.

But because the Go http servers have always automatically sent 100-continue headers in response to "Expect: 100-continue" in requests, there's a potential bug in Go's http2 server where the automatic HEADERS frame (with header field :status=100) gets sent AFTER the handler has already started writing, if separate goroutines are reading & writing.

Or not. But I don't think there's a test or precautions in the code about it. We might be getting lucky from some other layer.

Investigate. Low priority, though, since handlers don't typically do this, and people don't really use 100-continue.

@bradfitz bradfitz self-assigned this Jan 20, 2016
@bradfitz bradfitz added this to the Go1.7 milestone Jan 20, 2016
@quentinmit

This comment has been minimized.

Copy link
Contributor

@quentinmit quentinmit commented May 19, 2016

@bradfitz Punt to Go 1.8?

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

@bradfitz bradfitz commented May 19, 2016

I will either finish everything or punt everything in the next two days before I disappear for a month. This one I'm still curious about and is easier to test now that https://go-review.googlesource.com/#/c/23235/ for #13851 is ready.

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

@bradfitz bradfitz commented May 21, 2016

Confirmed that this is a bug.

I wrote this test:

// golang.org/issue/14030                                                                                                                                                    
func TestExpect100ContinueAfterHandlerWrites(t *testing.T) {
        const msg = "Hello"
        const msg2 = "World"

        doRead := make(chan bool, 1)
        defer close(doRead) // fallback cleanup                                                                                                                              

        st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) {
                io.WriteString(w, msg)
                w.(http.Flusher).Flush()

                // Do a read, which might force a 100-continue status to be sent.                                                                                            
                <-doRead 
                r.Body.Read(make([]byte, 10))

                io.WriteString(w, msg2) 

        }, optOnlyServer)
        defer st.Close()

        tr := &Transport{TLSClientConfig: tlsConfigInsecure}
        defer tr.CloseIdleConnections()

        req, _ := http.NewRequest("POST", st.ts.URL, io.LimitReader(neverEnding('A'), 2<<20))
        req.Header.Set("Expect", "100-continue")

        res, err := tr.RoundTrip(req) 
        if err != nil {
                t.Fatal(err) 
        } 
        defer res.Body.Close()

        buf := make([]byte, len(msg))
        if _, err := io.ReadFull(res.Body, buf); err != nil {
                t.Fatal(err)
        }
        if string(buf) != msg {
                t.Fatalf("msg = %q; want %q", buf, msg)
        } 

        doRead <- true

        if _, err := io.ReadFull(res.Body, buf); err != nil {
                t.Fatal(err)
        }
        if string(buf) != msg2 {
                t.Fatalf("second msg = %q; want %q", buf, msg2)
        }
}

And it results in:

=== RUN   TestExpect100ContinueAfterHandlerWrites
--- FAIL: TestExpect100ContinueAfterHandlerWrites (0.03s)
        transport_test.go:1963: connection error: PROTOCOL_ERROR
FAIL
exit status 1   

(where line 1963 is the second read....)

So we are sending a bogus 100 continue mid-data.

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

@bradfitz bradfitz commented May 21, 2016

/cc @mholt

@bradfitz bradfitz changed the title x/net/http2: test interaction of Server concurrent read/write with 100-continue x/net/http2: Server can write invalid HTTP/2 if handler writes before reading 100-continue request body May 21, 2016
gopherbot pushed a commit to golang/net that referenced this issue May 21, 2016
Since Go's HTTP/2 Handlers can read & write concurrently, it was
previously possible for a Handler to get an HTTP Request with "Expect:
100-continue" and to first Write some data, then read some data, and
the read of the data was triggering the server's automatic
"100-continue" response path.  But if the server had already replied
with a non-100 HEADERS frame, sending a HEADERS frame with :status 100
later is in violation of the spec.

Fix it by tracking whether we've already replied with something, and
ignoring the 100-continue when it's inappropriate.

Updates golang/go#14030

Change-Id: I6745ba0f257a31eaf63816e4263cadbef5a896a2
Reviewed-on: https://go-review.googlesource.com/23311
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 21, 2016

CL https://golang.org/cl/23312 mentions this issue.

@gopherbot gopherbot closed this in def50f8 May 21, 2016
@mholt

This comment has been minimized.

Copy link

@mholt mholt commented May 21, 2016

👍 Enjoy your vacation!

@golang golang locked and limited conversation to collaborators May 21, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.