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

net/http: Server.ConnContext accidentally modifies context for all connections #35750

Closed
rkollar opened this issue Nov 21, 2019 · 9 comments
Closed
Assignees
Labels

Comments

@rkollar
Copy link
Contributor

@rkollar rkollar commented Nov 21, 2019

What version of Go are you using (go version)?

$ go version
go version go1.13.4 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What did you do?

Docs of ConnContext say that:

ConnContext optionally specifies a function that modifies the context used for a new connection c.

However, it assigns this new context to a variable shared between connections in the accept loop. Thus creating a growing chain of contexts.

Example code:

package main

import (
	"context"
	"fmt"
	"net"
	"net/http"
)

func main() {
	server := &http.Server{
		Addr: ":4444",
		Handler: http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
			rw.Header().Set("Connection", "close")
		}),
		ConnContext: func(ctx context.Context, c net.Conn) context.Context {
			fmt.Printf("conn: %s\n", c.RemoteAddr())

			if c2 := ctx.Value("conn"); c2 != nil {
				fmt.Printf("existing: %s\n", c2.(net.Conn).RemoteAddr())
			}

			return context.WithValue(ctx, "conn", c)
		},
	}
	go func() {
		panic(server.ListenAndServe())
	}()

	var err error

	fmt.Printf("\nrequest 1:\n")
	_, err = http.Get("http://localhost:4444/")
	if err != nil {
		panic(err)
	}
	fmt.Printf("request 1 done\n")

	fmt.Printf("\nrequest 2:\n")
	_, err = http.Get("http://localhost:4444/")
	if err != nil {
		panic(err)
	}
	fmt.Printf("request 2 done\n")
}

What did you expect to see?

request 1:
conn: [::1]:34022
request 1 done

request 2:
conn: [::1]:34024
request 2 done

What did you see instead?

request 1:
conn: [::1]:34022
request 1 done

request 2:
conn: [::1]:34024
existing: [::1]:34022
request 2 done
rkollar added a commit to rkollar/go that referenced this issue Nov 21, 2019
@bradfitz bradfitz self-assigned this Nov 21, 2019
@bradfitz bradfitz added the NeedsFix label Nov 21, 2019
@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Nov 21, 2019

Wow, whoops. Thanks for the report!

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 21, 2019

Change https://golang.org/cl/208318 mentions this issue: net/http: fix Server.ConnContext modifying context for all new connections

@rkollar

This comment has been minimized.

Copy link
Contributor Author

@rkollar rkollar commented Nov 21, 2019

I would like to note that this could have some security implications. A large number of connections will eat both CPU and memory. For example: context.propagateCancel)

@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Nov 21, 2019

@rkollar, no need to convince me of its importance; I was already planning to backport this to previous releases. I'm just waiting on a test in that PR/CL. I can take the change over if you'd like.

@rkollar

This comment has been minimized.

Copy link
Contributor Author

@rkollar rkollar commented Nov 21, 2019

@bradfitz I'm already working on it, but it's my first submission and I have to go over the existing tests to learn what are the conventions. You can take over if you don't want to wait.

@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Nov 21, 2019

@gopherbot, please backport to Go 1.13.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 21, 2019

Backport issue(s) opened: #35765 (for 1.13).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@gopherbot gopherbot closed this in bbbc658 Nov 21, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 21, 2019

Change https://golang.org/cl/208235 mentions this issue: [release-branch.go1.13] net/http: fix Server.ConnContext modifying context for all new connections

gopherbot pushed a commit that referenced this issue Nov 22, 2019
…ntext for all new connections

Updates #35750
Fixes #35765

Change-Id: I65d38cfc5ddd66131777e104c269cc3559b2471d
GitHub-Last-Rev: 953fdfd
GitHub-Pull-Request: #35751
Reviewed-on: https://go-review.googlesource.com/c/go/+/208318
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
(cherry picked from commit bbbc658)
Reviewed-on: https://go-review.googlesource.com/c/go/+/208235
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@agirbal

This comment has been minimized.

Copy link

@agirbal agirbal commented Dec 5, 2019

Ah we just spent many hours trying to figure out why there seemed to be a massive memory leak following the use of this feature... Good catch and can't wait for fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.