code review 7309050: net: do not use RLock around Accept

net: do not use RLock around Accept

It might be non-blocking, but it also might be blocking.
Cannot take the chance, as Accept might block indefinitely
and make it impossible to acquire ForkLock exclusively
(during fork+exec).

Fixes issue 4737.

[rsc, 2013-02-05 20:31:32]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg

[dave_cheney.net, 2013-02-05 20:36:10]

Your comment about kernels < 2.6.28 is relevant. Maybe it is time to bite
the bullet and say that Go 1.1 really won't work on RHEL5.
On 6 Feb 2013 07:31, <rsc@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg
>
>
> Description:
> net: do not use RLock around Accept
>
> It might be non-blocking, but it also might be blocking.
> Cannot take the chance, as Accept might block indefinitely
> and make it impossible to acquire ForkLock exclusively
> (during fork+exec).
>
> Fixes issue 4737.
>
> Please review this at
https://codereview.appspot.**com/7309050/<https://codereview.appspot.com/7309...
>
> Affected files:
>   M src/pkg/net/sock_cloexec.go
>   M src/pkg/net/sys_cloexec.go
>
>
> Index: src/pkg/net/sock_cloexec.go
> ==============================**==============================**=======
> --- a/src/pkg/net/sock_cloexec.go
> +++ b/src/pkg/net/sock_cloexec.go
> @@ -50,14 +50,14 @@
>         }
>
>         // See ../syscall/exec_unix.go for description of ForkLock.
> -       // It is okay to hold the lock across syscall.Accept
> +       // It is probably okay to hold the lock across syscall.Accept
>         // because we have put fd.sysfd into non-blocking mode.
> -       syscall.ForkLock.RLock()
> +       // However, a call to the File method will put it back into
> +       // blocking mode. We can't take that risk, so no use of ForkLock
> here.
>         nfd, sa, err = syscall.Accept(fd)
>         if err == nil {
>                 syscall.CloseOnExec(nfd)
>         }
> -       syscall.ForkLock.RUnlock()
>         if err != nil {
>                 return -1, nil, err
>         }
> Index: src/pkg/net/sys_cloexec.go
> ==============================**==============================**=======
> --- a/src/pkg/net/sys_cloexec.go
> +++ b/src/pkg/net/sys_cloexec.go
> @@ -35,14 +35,14 @@
>  // descriptor as nonblocking and close-on-exec.
>  func accept(fd int) (int, syscall.Sockaddr, error) {
>         // See ../syscall/exec_unix.go for description of ForkLock.
> -       // It is okay to hold the lock across syscall.Accept
> +       // It is probably okay to hold the lock across syscall.Accept
>         // because we have put fd.sysfd into non-blocking mode.
> -       syscall.ForkLock.RLock()
> +       // However, a call to the File method will put it back into
> +       // blocking mode. We can't take that risk, so no use of ForkLock
> here.
>         nfd, sa, err := syscall.Accept(fd)
>         if err == nil {
>                 syscall.CloseOnExec(nfd)
>         }
> -       syscall.ForkLock.RUnlock()
>         if err != nil {
>                 return -1, nil, err
>         }
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[rsc, 2013-02-05 20:38:22]

The CL is still necessary on all the BSDs, so I don't see the point to
dropping support for <2.6.28 kernels.

Russ

[rsc, 2013-02-05 20:40:34]

Hello golang-dev@googlegroups.com, dave@cheney.net (cc:
golang-dev@googlegroups.com),

Please take another look.

[dave_cheney.net, 2013-02-05 20:55:56]

Sure. I was not clear in my previous reply. I was not proposing we drop support
for RHEL5 in Go 1.1, because it wasn't properly supported in Go 1.0. As time
goes on "not supported" moves from a quirk to downright annoying for those who
want to keep trying to use 2.6.18 kernels.  

On 06/02/2013, at 7:38, Russ Cox <rsc@golang.org> wrote:

> The CL is still necessary on all the BSDs, so I don't see the point to
dropping support for <2.6.28 kernels.
> 
> Russ
> 

[iant, 2013-02-05 22:04:17]

LGTM

[mikio, 2013-02-08 03:16:46]

LGTM

[rsc, 2013-02-08 03:45:22]

*** Submitted as https://code.google.com/p/go/source/detail?r=a33346cdc901 ***

net: do not use RLock around Accept

It might be non-blocking, but it also might be blocking.
Cannot take the chance, as Accept might block indefinitely
and make it impossible to acquire ForkLock exclusively
(during fork+exec).

Fixes issue 4737.

R=golang-dev, dave, iant, mikioh.mikioh
CC=golang-dev
https://codereview.appspot.com/7309050