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: data race on fd.sysfd #4369

Closed
dvyukov opened this issue Nov 8, 2012 · 9 comments

Comments

Projects
None yet
6 participants
@dvyukov
Copy link
Member

commented Nov 8, 2012

14836:38937a8d0911 tip

It's not yet reproducible with tip race detector (obtained with experimental version),
but the bug is definitely there.
At least it can lead to non-deterministic error value returned from
(*TCPListener).Accept(), which in turn leads to non-deterministic return value from
http.Serve(). I can imagine it makes some tests flaky.

WARNING: DATA RACE
Write by goroutine 48:
  net.(*netFD).decref()
      src/pkg/net/fd_unix.go:382 +0x10b
  net.(*netFD).Close()
      src/pkg/net/fd_unix.go:399 +0x11e
  net.(*TCPListener).Close()
      src/pkg/net/tcpsock_posix.go:271 +0x64
  net/http/httptest.(*historyListener).Close()
      src/pkg/net/http/httptest/recorder.go:0 +0x48
  net/http/httptest.(*Server).Close()
      src/pkg/net/http/httptest/server.go:153 +0x4e
  net/http_test.TestRedirectCookiesOnRequest()
      src/pkg/net/http/client_test.go:325 +0x194
  testing.tRunner()
      src/pkg/testing/testing.go:301 +0x8f

Previous read by goroutine 49:
  net.(*TCPListener).AcceptTCP()
      src/pkg/net/tcpsock_posix.go:245 +0x87
  net.(*TCPListener).Accept()
      src/pkg/net/tcpsock_posix.go:258 +0x56
  net/http/httptest.(*historyListener).Accept()
      src/pkg/net/http/httptest/server.go:43 +0x72
  net/http.(*Server).Serve()
      src/pkg/net/http/server.go:1088 +0x97

Goroutine 48 (running) created at:
  testing.RunTests()
      src/pkg/testing/testing.go:377 +0xb1a
  testing.Main()
      src/pkg/testing/testing.go:313 +0xcd
  main.main()
      net/http/_test/_testmain.go:301 +0xda
  runtime.main()
      src/pkg/runtime/proc.c:248 +0x91

Goroutine 49 (running) created at:
  net/http/httptest.(*Server).Start()
      src/pkg/net/http/httptest/server.go:102 +0x3cb
  net/http/httptest.NewServer()
      src/pkg/net/http/httptest/server.go:77 +0x4f
  net/http_test.TestRedirectCookiesOnRequest()
      src/pkg/net/http/client_test.go:315 +0x62
  testing.tRunner()
      src/pkg/testing/testing.go:301 +0x8f
@robpike

This comment has been minimized.

Copy link
Contributor

commented Nov 10, 2012

Comment 1:

Labels changed: added priority-soon, removed priority-triage.

Status changed to Accepted.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 11, 2012

Comment 2:

What is this bug about?
Is this about changing TestRedirectCookiesOnRequest or defining the meaning of Accept's
return value when a concurrent Close is called?
I would hope the latter.  I think Accept should return io.EOF or something when a Close
comes in, and I'd like to see that documented and tested.
@dvyukov

This comment has been minimized.

Copy link
Member Author

commented Nov 14, 2012

Comment 3:

This is about fixing the data race in the first place. At least with gccgo it can lead
to execution of malicious code.
Defining return value of Accept() preempted by Close() and adding the test would be
great as well.
@davecheney

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2012

Comment 4:

Possibly https://golang.org/cl/6855047/ would suffice. I believe (but am
checking now) that access to members of netFD is gated by incref/decref which already
handle the closed and closing cases.
@dvyukov

This comment has been minimized.

Copy link
Member Author

commented Nov 14, 2012

Comment 5:

Just removing l.fd.sysfd check looks sufficient. I am not sure why it is there in the
first place (perhaps some historical artifacts).
Brad also mentioned returning io.EOF from Accept in this case.
@davecheney

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2012

Comment 6:

I can propose that change. Are you able to test this with your version of tsan ?
@dvyukov

This comment has been minimized.

Copy link
Member Author

commented Nov 14, 2012

Comment 7:

Not yet, tsan currently can't instrument right operands of || and && because of problems
in the compiler.
But if we all agree that the problem is here and the fix actually fixes it, it is OK to
land it. Tsan will validate it later.
@davecheney

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2012

Comment 8:

https://golang.org/cl/6843076/
This proposal does not change the return value of Accept(). I would like to handle that
on another issue, possibly https://golang.org/issue/4373.

Owner changed to @davecheney.

Status changed to Started.

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2012

Comment 9:

This issue was closed by revision c9856e7.

Status changed to Fixed.

@dvyukov dvyukov added fixed labels Nov 18, 2012

@rsc rsc added this to the Go1.1 milestone Apr 14, 2015

@rsc rsc removed the go1.1 label Apr 14, 2015

@golang golang locked and limited conversation to collaborators Jun 24, 2016

This issue was closed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.