code review 57890043: syscall: fix leakage of file descriptors after a fork o...

syscall: fix leakage of file descriptors after a fork on Plan 9

Instead of trying to emulate the racy method used on Unix, we
simply read the list of open file descriptors from the dup(3)
device once we're inside the child and close everything that
we don't want to keep.

This change was inspired by similar code in the 9front version
of page(1).

Also remove all uses of the ForkLock since it's unneccesary.

Fixes issue 7118.
Fixes issue 5605.

[ality, 2014-01-28 20:38:31]

Hello golang-codereviews@googlegroups.com (cc: 0intro@gmail.com,
jas@corpus-callosum.com),

I'd like you to review this change to
https://code.google.com/p/go/

[0intro, 2014-01-29 05:39:19]

Thanks. It looks pretty interesting.

I'm occasionally seeing some failures when running
the tests here. Does it happen on your side?

--- FAIL: TestPipeLookPathLeak (0.76 seconds)
	exec_test.go:201: leaked 3 fds; want ~0

-- 
David du Colombier

[bradfitz, 2014-01-29 09:10:57]

That's a lot of deleted lines. :)

https://codereview.appspot.com/57890043/diff/20001/src/pkg/syscall/exec_plan9.go
File src/pkg/syscall/exec_plan9.go (right):

https://codereview.appspot.com/57890043/diff/20001/src/pkg/syscall/exec_plan9...
src/pkg/syscall/exec_plan9.go:140: b := statbuf[i:]
check that this is at least 2 bytes long before you read the size?

[ality, 2014-01-29 10:28:15]

David du Colombier <0intro@gmail.com> once said:
> Thanks. It looks pretty interesting.
> 
> I'm occasionally seeing some failures when running
> the tests here. Does it happen on your side?
> 
> --- FAIL: TestPipeLookPathLeak (0.76 seconds)
> 	exec_test.go:201: leaked 3 fds; want ~0

I haven't seen that on my machine or on hydra. Any
chance I could get access to the machine that it's
failing on?

I made a small change to the CL to fix a failing
test in the os package (TestStartProcess) but it
probably won't have an effect on the failure that
you're seeing.

It's worth a try, though.

Thanks for testing,
  Anthony

[0intro, 2014-01-29 20:44:23]

os.test 29015: suicide: sys: trap: fault read addr=0x36 pc=0x0002226c
--- FAIL: TestKillStartProcess (1.60 seconds)
	os_test.go:1270: Test process succeeded, but expected to fail
--- FAIL: TestKillFindProcess (1.50 seconds)
	os_test.go:1270: Test process succeeded, but expected to fail
FAIL
FAIL	os	6.960s
--- FAIL: TestPipeLookPathLeak (0.86 seconds)
	exec_test.go:201: leaked 3 fds; want ~0
FAIL
FAIL	os/exec	3.604s

I'm only encountering this failure when running all.rc on one of my development
machines (and not when running "go test -short" in os/exec). I've never succeed
to reproduce it on hydra (old builder) or orion (new builder).

By the way, the new builder is located in the same public subnet, but is on
another authentication domain. I'll email you the login details soon, so you may
give a try.

Otherwise, I did many tests on various machines and it seems to be working fine
so far. So, I think this change is good enough to be submitted.

[rminnich, 2014-01-31 14:55:45]

What I like about this is you're really using Plan 9 to effect. 

Seems it gets you further. I can't test but in terms of what it's doing it seems
right.

LGTM

[rsc, 2014-02-13 15:25:52]

Removing ForkLock is fine but the new ForkExec is buggy.

https://codereview.appspot.com/57890043/diff/80001/src/pkg/syscall/exec_plan9.go
File src/pkg/syscall/exec_plan9.go (left):

https://codereview.appspot.com/57890043/diff/80001/src/pkg/syscall/exec_plan9...
src/pkg/syscall/exec_plan9.go:186: // Guard against side effects of shuffling
fds below.
This is still necessary.
The problem it is fixing is that if you have attr.Files = {3,2,1}, then you have
to take care not to execute:

dup(3, 0)
dup(2, 1)
dup(1, 2)

because the last two dups will collide and you will end up having arranged
attr.Files = {3,2,2} instead.

That's what the Pass 1 / Pass 2 / Pass 3 is in the original code.

[rsc, 2014-04-17 02:49:43]

R=close

hg mail after 1.3.

[bradfitz, 2014-10-02 18:51:46]

Is somebody going to pick this up again, now that 1.3 is out?

[lucio, 2014-10-03 04:43:51]

https://codereview.appspot.com/57890043/diff/80001/src/pkg/syscall/exec_plan9.go
File src/pkg/syscall/exec_plan9.go (left):

https://codereview.appspot.com/57890043/diff/80001/src/pkg/syscall/exec_plan9...
src/pkg/syscall/exec_plan9.go:186: // Guard against side effects of shuffling
fds below.
On 2014/02/13 15:25:52, rsc wrote:
> This is still necessary.

Can I suggest that we declare and set nextfd := len(attr.Files) here rather than
restore it in the declaration higher up?  Note the "fd :=" just above it.

Lucio.

[mischief, 2014-10-29 19:53:46]

ping on this. hopefully this can solve the flaky builder problem with
plan9/amd64.

[lucio, 2014-10-30 03:57:56]

The last two comments (a trivial one by me, but the one from Russ
seems quite relevant) haven't been addressed yet.  I don't think this
will pass...

Lucio.


On 10/29/14, mischief@offblast.org <mischief@offblast.org> wrote:
> ping on this. hopefully this can solve the flaky builder problem with
> plan9/amd64.
>
> https://codereview.appspot.com/57890043/
>

[lucio, 2014-10-30 03:58:10]

On 10/30/14, Lucio De Re <lucio.dere@gmail.com> wrote:
> The last two comments (a trivial one by me, but the one from Russ
> seems quite relevant) haven't been addressed yet.  I don't think this
> will pass...
>
> Lucio.
>
>
> On 10/29/14, mischief@offblast.org <mischief@offblast.org> wrote:
>> ping on this. hopefully this can solve the flaky builder problem with
>> plan9/amd64.
>>
>> https://codereview.appspot.com/57890043/
>>
>