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/rpc: data race on non-net transport #5792

Closed
dvyukov opened this issue Jun 26, 2013 · 21 comments
Closed

net/rpc: data race on non-net transport #5792

dvyukov opened this issue Jun 26, 2013 · 21 comments
Labels
Documentation FrozenDueToAge help wanted
Milestone
Go1.10

Comments

@dvyukov
Copy link
Member

dvyukov commented Jun 26, 2013 

Here is the reproducer:
https://golang.org/cl/10613043/diff/3001/src/pkg/net/rpc/server_test.go

Report is below.

The gist: it's impossible to use net/rpc client/server with any non-thread-safe
transport as pipe or socketpair. It's inevitably produces races between read/write and
close. Such races corrupt arbitrary files and write data to random network connections.

=== RUN TestUnixPipe
==================
WARNING: DATA RACE
Read by goroutine 59:
  os.(*File).read()
      src/pkg/os/file_unix.go:175 +0x5b
  os.(*File).Read()
      src/pkg/os/file.go:95 +0xb9
  bufio.(*Reader).fill()
      src/pkg/bufio/bufio.go:119 +0x272
  bufio.(*Reader).Read()
      src/pkg/bufio/bufio.go:187 +0x3b1
  io.ReadAtLeast()
      src/pkg/io/io.go:284 +0x11a
  io.ReadFull()
      src/pkg/io/io.go:302 +0x7c
  encoding/gob.decodeUintReader()
      src/pkg/encoding/gob/decode.go:65 +0xba
  encoding/gob.(*Decoder).recvMessage()
      src/pkg/encoding/gob/decoder.go:73 +0x81
  encoding/gob.(*Decoder).decodeTypeSequence()
      src/pkg/encoding/gob/decoder.go:159 +0x92
  encoding/gob.(*Decoder).DecodeValue()
      src/pkg/encoding/gob/decoder.go:223 +0x124
  encoding/gob.(*Decoder).Decode()
      src/pkg/encoding/gob/decoder.go:202 +0x227
  net/rpc.(*gobClientCodec).ReadResponseHeader()
      src/pkg/net/rpc/client.go:218 +0x5d
  net/rpc.(*Client).input()
      src/pkg/net/rpc/client.go:106 +0x118

Previous write by goroutine 57:
  os.(*file).close()
      src/pkg/os/file_unix.go:110 +0x164
  os.(*File).Close()
      src/pkg/os/file_unix.go:99 +0x43
  net/rpc.(*gobClientCodec).Close()
      src/pkg/net/rpc/client.go:226 +0x48
  net/rpc.(*Client).Close()
      src/pkg/net/rpc/client.go:280 +0xc6
  net/rpc.TestUnixPipe()
      src/pkg/net/rpc/server_test.go:567 +0x3d5
  testing.tRunner()
      src/pkg/testing/testing.go:361 +0x10c

Goroutine 59 (running) created at:
  net/rpc.NewClientWithCodec()
      src/pkg/net/rpc/client.go:196 +0xd5
  net/rpc.NewClient()
      src/pkg/net/rpc/client.go:186 +0x18d
  net/rpc.TestUnixPipe()
      src/pkg/net/rpc/server_test.go:566 +0x3c7
  testing.tRunner()
      src/pkg/testing/testing.go:361 +0x10c

Goroutine 57 (running) created at:
  testing.RunTests()
      src/pkg/testing/testing.go:441 +0xaef
  testing.Main()
      src/pkg/testing/testing.go:373 +0xab
  main.main()
      net/rpc/_test/_testmain.go:74 +0xda
==================
@dvyukov
Copy link
Member Author

dvyukov commented Jun 26, 2013

Comment 1:

net/rpc docs does not seem to impose any thread-safety requirements on ReaderWriter's.
Note that it's possible to properly close the connection from both client and server.

@davecheney
Copy link
Contributor

Comment 2:

We had exactly the same problem with the mock ssh server in go.crypto/ssh/test.

@rsc
Copy link
Contributor

rsc commented Jul 30, 2013

Comment 3:

Labels changed: added priority-later, go1.2, removed priority-triage.

Status changed to Accepted.

@rsc
Copy link
Contributor

rsc commented Jul 30, 2013

Comment 4:

Labels changed: added feature.

@robpike
Copy link
Contributor

robpike commented Aug 6, 2013

Comment 5:

Tangential question, but it surprises me that pipes are not thread-safe. I'm not even
sure what that means. Is it just that data from parallel writes can be intermingled? The
kernel doesn't lock the file descriptor during the write?
Sockets too? Well, tie me up and call the butcher.

@dvyukov
Copy link
Member Author

dvyukov commented Aug 6, 2013

Comment 6:

The race happens NOT inside of kernel, it happens in our code.
The race happens between read/write and close. Consider the following code:
// thread 1
... = write(pipe.fd, ....);
// thread 2
close(pipe.fd);
pipe.fd = -1;
Thread 1 loads pipe.fd value into register, then thread 2 closes the fd, then some other
thread 3 create a socket that happened to reuse the fd, then thread 1 sends your credit
card number into random socket.
In net.Conn we do very special things to prevent such things, i.e. read/write either
uses still alive correct fd or do not issue read/write at all.
All other things (files, pipes) are not like this, they are "single threaded", i.e. you
can not call read/write and concurrently close it.

@robpike
Copy link
Contributor

robpike commented Aug 6, 2013

Comment 7:

I realize there's a race in the code, but you said that pipes and socket
pairs were not thread-safe. Is that a different problem, or just the one
the race detector found?

@robpike
Copy link
Contributor

robpike commented Aug 6, 2013

Comment 8:

Ah, I see, the last paragraph makes it clear. Thanks.

@alexbrainman
Copy link
Member

Comment 9:

Dmitriy,
While we're on this subject, I would like to talk to you about (*netFD).Close. I could
be wrong, but, I think, semantically windows and unix (*netFD).Close are different. One
waits for all outstanding IO to finish, while the other does not. The questions here is:
"What is guaranteed on (*netFD).Close return? Can it be assumed that all outstanding IO
is completed and is flushed to OS or canceled? Can we be certain that OS socket is
closed?". Perhaps it is not important, but I would like to run this concern by you.
Alex

@rsc
Copy link
Contributor

rsc commented Aug 13, 2013

Comment 10:

The discussion of comment #9 moved to issue #6074.

@robpike
Copy link
Contributor

robpike commented Aug 29, 2013

Comment 11:

Won't happen for 1.2. It's easy to fix the send side but the read is harder.

Labels changed: removed go1.2.

@rsc
Copy link
Contributor

rsc commented Nov 27, 2013

Comment 12:

Labels changed: added go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Nov 27, 2013

Comment 13:

Labels changed: removed feature.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 14:

Labels changed: added release-none, removed go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 15:

Labels changed: added repo-main.

@robpike
Copy link
Contributor

robpike commented Dec 19, 2013

Comment 16:

Marking this 1.3Maybe so it gets looked at again. I have a half fix but not a full fix.
It should be addressed properly if it can be.

Labels changed: added release-go1.3, removed release-none.

@alexbrainman
Copy link
Member

Comment 17:

Here http://build.golang.org/log/06efb61af57fac64b4d04c3838eea04cb0eb37c6 is the race on
windows builder.
Alex

@dvyukov
Copy link
Member Author

dvyukov commented Mar 5, 2014

Comment 18:

Alex, that one is false positive, and it tracked in:
https://golang.org/issue/7460

@rsc
Copy link
Contributor

rsc commented May 11, 2014

Comment 19:

Rob and I have talked about this but we're still not sure how to do this correctly in
net/rpc. It's not even clear to me that net/rpc is buggy. It might be that we just need
to document what the expectations are for the interface.
We should probably also fix os.File not to have this race. I filed issue #7970 for that.

Labels changed: added release-go1.4, removed release-go1.3.

@rsc
Copy link
Contributor

rsc commented Oct 15, 2014

Comment 20:

Labels changed: added release-go1.5, removed release-go1.4.

@bradfitz bradfitz modified the milestone: Go1.5 Dec 16, 2014
@rsc rsc removed accepted labels Apr 14, 2015
@rsc rsc modified the milestones: Unplanned, Go1.5 Jul 15, 2015
@robpike robpike mentioned this issue Oct 4, 2016
Closed
@bradfitz bradfitz modified the milestones: Go1.9, Unplanned Jan 31, 2017
@bradfitz bradfitz modified the milestones: Go1.9Maybe, Go1.9 Jun 9, 2017
@bradfitz bradfitz modified the milestones: Go1.10, Go1.9Maybe Jul 20, 2017
@rsc
Copy link
Contributor

rsc commented Nov 22, 2017

We fixed #7970, so now there is no longer a race for pipes and socketpairs. I think everything is OK now.

@rsc rsc closed this as completed Nov 22, 2017
@golang golang locked and limited conversation to collaborators Nov 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Documentation FrozenDueToAge help wanted
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
7 participants
@bradfitz @davecheney @rsc @dvyukov @robpike @gopherbot @alexbrainman