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

Jump to bottom

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

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

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

rkollar opened this issue Nov 21, 2019 · 9 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.

Comments

@rkollar
Copy link
Contributor

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
@bradfitz bradfitz self-assigned this Nov 21, 2019
@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 21, 2019
@bradfitz
Copy link
Contributor

Wow, whoops. Thanks for the report!

@rkollar rkollar mentioned this issue Nov 21, 2019
Closed
@gopherbot
Copy link

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

@rkollar
Copy link
Contributor Author

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
Copy link
Contributor

@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
Copy link
Contributor Author

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
Copy link
Contributor

@gopherbot, please backport to Go 1.13.

@gopherbot gopherbot mentioned this issue Nov 21, 2019
Closed
@gopherbot
Copy link

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
Copy link

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
@rkollar @bradfitz
[release-branch.go1.13] net/http: fix Server.ConnContext modifying co…
94b6eec 
…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
Copy link

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!

Vitaly-kandasoft added a commit to lifepod-solutions/go that referenced this issue May 4, 2020
@Vitaly-kandasoft
VG-1354 backport of golang#35750
acb083a
@Vitaly-kandasoft Vitaly-kandasoft mentioned this issue May 4, 2020
Merged
@simonswine simonswine mentioned this issue Sep 11, 2020
Merged
@golang golang locked and limited conversation to collaborators Dec 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

net/http: fix Server.ConnContext modifying context for all new connections
4 participants
@bradfitz @agirbal @gopherbot @rkollar