code review 7406050: syscall: fix FD passing on FreeBSD, NetBSD and OpenBSD

syscall: fix FD passing on FreeBSD and NetBSD

Fixes issue 3348.

[dave_cheney.net, 2013-02-26 12:08:33]

Hello devon.odell@gmail.com (cc: golang-dev@googlegroups.com),

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

[dave_cheney.net, 2013-02-26 12:10:57]

Tested on freebsd/amd64 and freebsd/arm, also linux/amd64. Can someone
please test on openbsd/*

On Tue, Feb 26, 2013 at 11:08 PM,  <dave@cheney.net> wrote:
> Reviewers: dho,
>
> Message:
> Hello devon.odell@gmail.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> syscall: fix FD passing on FreeBSD and OpenBSD
>
> Fixes issue 3348.
> Fixes issue 3349.
>
> Please review this at https://codereview.appspot.com/7406050/
>
> Affected files:
>   M src/pkg/syscall/passfd_test.go
>   M src/pkg/syscall/sockcmsg_unix.go
>
>
> Index: src/pkg/syscall/passfd_test.go
> ===================================================================
> --- a/src/pkg/syscall/passfd_test.go
> +++ b/src/pkg/syscall/passfd_test.go
> @@ -2,7 +2,7 @@
>  // Use of this source code is governed by a BSD-style
>  // license that can be found in the LICENSE file.
>
> -// +build linux darwin
> +// +build linux darwin freebsd openbsd
>
>  package syscall_test
>
> @@ -90,7 +90,7 @@
>                 t.Fatalf("syscall.ParseUnixRights: %v", err)
>         }
>         if len(gotFds) != 1 {
> -               t.Fatalf("wanted 1 fd; got %#v", gotFds)
> +               t.Errorf("wanted 1 fd; got %#v", gotFds)
>         }
>
>         f := os.NewFile(uintptr(gotFds[0]), "fd-from-child")
> Index: src/pkg/syscall/sockcmsg_unix.go
> ===================================================================
> --- a/src/pkg/syscall/sockcmsg_unix.go
> +++ b/src/pkg/syscall/sockcmsg_unix.go
> @@ -79,7 +79,7 @@
>         h.Level = SOL_SOCKET
>         h.Type = SCM_RIGHTS
>         h.SetLen(CmsgLen(datalen))
> -       data := uintptr(cmsgData(h))
> +       data := uintptr(cmsgAlignOf(int(uintptr(cmsgData(h)))))
>         for _, fd := range fds {
>                 *(*int32)(unsafe.Pointer(data)) = int32(fd)
>                 data += 4
>
>

[minux1, 2013-02-26 12:30:03]

On Tue, Feb 26, 2013 at 8:10 PM, Dave Cheney <dave@cheney.net> wrote:

> Tested on freebsd/amd64 and freebsd/arm, also linux/amd64. Can someone
> please test on openbsd/*

On OpenBSD/amd64:
=== RUN TestPassFD
--- PASS: TestPassFD (0.02 seconds)
panic: runtime error: slice bounds out of range [recovered]
panic: runtime error: slice bounds out of range

goroutine 3 [running]:
testing.func·004()
/home/minux/go.hg/src/pkg/testing/testing.go:341 +0xbc
syscall.ParseSocketControlMessage(0xc200056500, 0x14, 0x20, 0x20,
0xc200056500, ...)
 /home/minux/go.hg/src/pkg/syscall/sockcmsg_unix.go:60 +0x277
syscall_test.TestPassFD(0xc200000090)
/home/minux/go.hg/src/pkg/syscall/passfd_test.go:80 +0xa29
testing.tRunner(0xc200000090, 0x61b618)
/home/minux/go.hg/src/pkg/testing/testing.go:346 +0x8a
created by testing.RunTests
/home/minux/go.hg/src/pkg/testing/testing.go:426 +0x86b

goroutine 1 [chan receive]:
testing.RunTests(0x56a820, 0x61b618, 0x1, 0x1, 0x1, ...)
/home/minux/go.hg/src/pkg/testing/testing.go:427 +0x88e
testing.Main(0x56a820, 0x61b618, 0x1, 0x1, 0x624b28, ...)
/home/minux/go.hg/src/pkg/testing/testing.go:358 +0x8a
main.main()
 syscall/_test/_testmain.go:43 +0x9a

goroutine 0 [syscall]:

goroutine 5 [syscall]:
syscall.Syscall6()
 /home/minux/go.hg/src/pkg/syscall/asm_openbsd_amd64.s:38 +0x5
syscall.kevent(0x8, 0x0, 0x0, 0xc200094148, 0xa, ...)
/home/minux/go.hg/src/pkg/syscall/zsyscall_openbsd_amd64.go:199 +0x83
syscall.Kevent(0x8, 0x0, 0x0, 0x0, 0xc200094148, ...)
/home/minux/go.hg/src/pkg/syscall/syscall_bsd.go:554 +0x9b
net.(*pollster).WaitFD(0xc200094140, 0xc2000748a0, 0x0, 0xc200074690,
0xc2000752a0, ...)
 /home/minux/go.hg/src/pkg/net/fd_bsd.go:98 +0x1af
net.(*pollServer).Run(0xc2000748a0)
/home/minux/go.hg/src/pkg/net/fd_unix.go:212 +0x10a
created by net.newPollServer
/home/minux/go.hg/src/pkg/net/newpollserver_unix.go:33 +0x2d5
exit status 2
FAIL syscall 0.876s

[dave_cheney.net, 2013-02-26 12:58:46]

OK, thanks, I'll update the description and leave the OpenBSD issue open.
On 26 Feb 2013 23:30, "minux" <minux.ma@gmail.com> wrote:

>
> On Tue, Feb 26, 2013 at 8:10 PM, Dave Cheney <dave@cheney.net> wrote:
>
>> Tested on freebsd/amd64 and freebsd/arm, also linux/amd64. Can someone
>> please test on openbsd/*
>
> On OpenBSD/amd64:
> === RUN TestPassFD
> --- PASS: TestPassFD (0.02 seconds)
> panic: runtime error: slice bounds out of range [recovered]
> panic: runtime error: slice bounds out of range
>
> goroutine 3 [running]:
> testing.func·004()
> /home/minux/go.hg/src/pkg/testing/testing.go:341 +0xbc
> syscall.ParseSocketControlMessage(0xc200056500, 0x14, 0x20, 0x20,
> 0xc200056500, ...)
>  /home/minux/go.hg/src/pkg/syscall/sockcmsg_unix.go:60 +0x277
> syscall_test.TestPassFD(0xc200000090)
> /home/minux/go.hg/src/pkg/syscall/passfd_test.go:80 +0xa29
> testing.tRunner(0xc200000090, 0x61b618)
> /home/minux/go.hg/src/pkg/testing/testing.go:346 +0x8a
> created by testing.RunTests
> /home/minux/go.hg/src/pkg/testing/testing.go:426 +0x86b
>
> goroutine 1 [chan receive]:
> testing.RunTests(0x56a820, 0x61b618, 0x1, 0x1, 0x1, ...)
> /home/minux/go.hg/src/pkg/testing/testing.go:427 +0x88e
> testing.Main(0x56a820, 0x61b618, 0x1, 0x1, 0x624b28, ...)
> /home/minux/go.hg/src/pkg/testing/testing.go:358 +0x8a
> main.main()
>  syscall/_test/_testmain.go:43 +0x9a
>
> goroutine 0 [syscall]:
>
> goroutine 5 [syscall]:
> syscall.Syscall6()
>  /home/minux/go.hg/src/pkg/syscall/asm_openbsd_amd64.s:38 +0x5
> syscall.kevent(0x8, 0x0, 0x0, 0xc200094148, 0xa, ...)
> /home/minux/go.hg/src/pkg/syscall/zsyscall_openbsd_amd64.go:199 +0x83
> syscall.Kevent(0x8, 0x0, 0x0, 0x0, 0xc200094148, ...)
> /home/minux/go.hg/src/pkg/syscall/syscall_bsd.go:554 +0x9b
> net.(*pollster).WaitFD(0xc200094140, 0xc2000748a0, 0x0, 0xc200074690,
> 0xc2000752a0, ...)
>  /home/minux/go.hg/src/pkg/net/fd_bsd.go:98 +0x1af
> net.(*pollServer).Run(0xc2000748a0)
> /home/minux/go.hg/src/pkg/net/fd_unix.go:212 +0x10a
> created by net.newPollServer
> /home/minux/go.hg/src/pkg/net/newpollserver_unix.go:33 +0x2d5
> exit status 2
> FAIL syscall 0.876s
>
>

[bradfitz, 2013-02-26 15:43:52]

LGTM once you're happy with your testing.

[minux1, 2013-02-26 16:02:28]

The test also passes on NetBSD/amd64 and NetBSD/386.
(I re-imaged my RPi, so I can't test there for now)

NetBSD/amd64 needs the fix to sockcmsg_unix.go.

[minux1, 2013-02-26 16:25:38]

This patch makes the test pass on OpenBSD/amd64:
diff -r 512627a9cdb8 src/pkg/syscall/sockcmsg_unix.go
--- a/src/pkg/syscall/sockcmsg_unix.go Mon Feb 25 22:06:58 2013 -0800
+++ b/src/pkg/syscall/sockcmsg_unix.go Wed Feb 27 00:17:55 2013 +0800
@@ -57,7 +57,7 @@
  }
  m := SocketControlMessage{Header: *h, Data:
dbuf[:int(h.Len)-cmsgAlignOf(SizeofCmsghdr)]}
  msgs = append(msgs, m)
- b = b[cmsgAlignOf(int(h.Len)):]
+ b = b[h.Len:]
  }
  return msgs, nil
 }

It doesn't affect Darwin/amd64, FreeBSD/amd64, NetBSD/amd64 and NetBSD/386.

[dho, 2013-02-26 16:42:33]

Does it work with b[cmsgAlignOf(h.Len):]? This stuff is
(unfortunately) really sensitive to alignment and sizing issues. The
int cast might be the problem.

2013/2/26 minux <minux.ma@gmail.com>:
> This patch makes the test pass on OpenBSD/amd64:
> diff -r 512627a9cdb8 src/pkg/syscall/sockcmsg_unix.go
> --- a/src/pkg/syscall/sockcmsg_unix.go Mon Feb 25 22:06:58 2013 -0800
> +++ b/src/pkg/syscall/sockcmsg_unix.go Wed Feb 27 00:17:55 2013 +0800
> @@ -57,7 +57,7 @@
>   }
>   m := SocketControlMessage{Header: *h, Data:
> dbuf[:int(h.Len)-cmsgAlignOf(SizeofCmsghdr)]}
>   msgs = append(msgs, m)
> - b = b[cmsgAlignOf(int(h.Len)):]
> + b = b[h.Len:]
>   }
>   return msgs, nil
>  }
>
> It doesn't affect Darwin/amd64, FreeBSD/amd64, NetBSD/amd64 and NetBSD/386.

[minux1, 2013-02-26 16:49:14]

On Wed, Feb 27, 2013 at 12:42 AM, Devon H. O'Dell <devon.odell@gmail.com>wrote:

> Does it work with b[cmsgAlignOf(h.Len):]? This stuff is
> (unfortunately) really sensitive to alignment and sizing issues. The
> int cast might be the problem.
>
When the out-of-bound error happens:
h.Len = 20
cmsgAlignOf(h.Len) = 24
len(b) = 20

We probably should create more tests for the control message parsing stuff.

[mdempsky, 2013-02-26 18:50:29]

https://codereview.appspot.com/7406050/diff/4006/src/pkg/syscall/sockcmsg_uni...
File src/pkg/syscall/sockcmsg_unix.go (right):

https://codereview.appspot.com/7406050/diff/4006/src/pkg/syscall/sockcmsg_uni...
src/pkg/syscall/sockcmsg_unix.go:82: data :=
uintptr(cmsgAlignOf(int(uintptr(cmsgData(h)))))
Shouldn't instead cmsgData be fixed instead to return a properly aligned
pointer?  E.g., the macro on OpenBSD for CMSG_DATA is:

/* given pointer to struct cmsghdr, return pointer to data */
#define CMSG_DATA(cmsg) \
        ((u_char *)(cmsg) + _ALIGN(sizeof(struct cmsghdr)))

Which means on amd64 (where _ALIGN rounds to a multiple of 8), then CMSG_DATA()
starts 16 bytes after cmsg, not 12 bytes like on i386.

[dave_cheney.net, 2013-02-26 21:40:29]

Thank you to all who tested this patch, @mdempsky is correct, the fix should go
in cmsgData() and match the original macro, which I have now done. 

I have also expanded the // +build tag to include netbsd, as it _should_ work on
this platform as well.

[dave_cheney.net, 2013-02-26 21:41:27]

Hello devon.odell@gmail.com, minux.ma@gmail.com, bradfitz@golang.org,
mdempsky@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2013-02-26 21:42:03]

LGTM

[mdempsky, 2013-02-26 21:43:45]

LGTM

[dave_cheney.net, 2013-02-26 21:43:52]

> We probably should create more tests for the control message parsing stuff.

@minux, I agree. I am concerned that the we are passing fd's, which are size_t
types between processes, but always casting them to int32s. This might be wrong
and being happily covered up by the fact the parsing at the other side always
assumes int32's.

[dave_cheney.net, 2013-02-26 21:47:39]

On 2013/02/26 21:43:45, mdempsky wrote:
> LGTM

Ok, i'll suck in my gut and commit this in a few mins. A quick survey of the
builders says that netbsd will be able to test this before hitting,
http://build.golang.org/log/32413119bb1ba5867462d83f5c35240d007b73c3, openbsd is
similar, but broken at a different point.

Is anyone able to test on netbsd or openbsd before I commit ?

[mdempsky, 2013-02-26 21:48:06]

> I am concerned that the we are passing fd's, which are size_t
> types between processes

What do you mean?  POSIX guarantees that fds fit in a C int, and all POSIX
systems I know of have 32-bit ints.

[mdempsky, 2013-02-26 21:48:35]

On 2013/02/26 21:47:39, dfc wrote:
> Is anyone able to test on netbsd or openbsd before I commit ?

Sure, I'll test on OpenBSD.

[dave_cheney.net, 2013-02-26 21:49:11]

Ahh, that was my misunderstanding, I was working from the definition
of most fds passed into functions like mmap, which is size_t. In that
case, please ignore my incorrect concerns.

On Wed, Feb 27, 2013 at 8:48 AM,  <mdempsky@google.com> wrote:
>> I am concerned that the we are passing fd's, which are size_t
>> types between processes
>
>
> What do you mean?  POSIX guarantees that fds fit in a C int, and all
> POSIX systems I know of have 32-bit ints.
>
> https://codereview.appspot.com/7406050/

[mdempsky, 2013-02-26 21:56:30]

Hm, still slice bounds out of range on OpenBSD:

=== RUN TestPassFD
--- PASS: TestPassFD (0.02 seconds)
panic: runtime error: slice bounds out of range [recovered]
        panic: runtime error: slice bounds out of range

goroutine 3 [running]:
testing.func·004()
        /tmp/go/src/pkg/testing/testing.go:341 +0xbc
syscall.ParseSocketControlMessage(0xc200058640, 0x14, 0x20, 0x20, 0xc200058640,
...)
        /tmp/go/src/pkg/syscall/sockcmsg_unix.go:60 +0x277
syscall_test.TestPassFD(0xc200000090)
        /tmp/go/src/pkg/syscall/passfd_test.go:80 +0xa29
testing.tRunner(0xc200000090, 0x61b618)
        /tmp/go/src/pkg/testing/testing.go:346 +0x8a
created by testing.RunTests
        /tmp/go/src/pkg/testing/testing.go:426 +0x86b

goroutine 1 [chan receive]:
testing.RunTests(0x56a890, 0x61b618, 0x1, 0x1, 0x1, ...)
        /tmp/go/src/pkg/testing/testing.go:427 +0x88e
testing.Main(0x56a890, 0x61b618, 0x1, 0x1, 0x624b28, ...)
        /tmp/go/src/pkg/testing/testing.go:358 +0x8a
main.main()
        syscall/_test/_testmain.go:43 +0x9a

goroutine 0 [syscall]:

goroutine 5 [syscall]:
syscall.Syscall6()
        /tmp/go/src/pkg/syscall/asm_openbsd_amd64.s:38 +0x5
syscall.kevent(0x8, 0x0, 0x0, 0xc200096288, 0xa, ...)
        /tmp/go/src/pkg/syscall/zsyscall_openbsd_amd64.go:199 +0x83
syscall.Kevent(0x8, 0x0, 0x0, 0x0, 0xc200096288, ...)
        /tmp/go/src/pkg/syscall/syscall_bsd.go:554 +0x9b
net.(*pollster).WaitFD(0xc200096280, 0xc200076840, 0x0, 0xc200076660,
0xc2000772a0, ...)
        /tmp/go/src/pkg/net/fd_bsd.go:98 +0x1af
net.(*pollServer).Run(0xc200076840)
        /tmp/go/src/pkg/net/fd_unix.go:212 +0x10a
created by net.newPollServer
        /tmp/go/src/pkg/net/newpollserver_unix.go:33 +0x2d5
exit status 2
FAIL    syscall 0.028s

[mdempsky, 2013-02-26 21:56:50]

On 2013/02/26 21:56:30, mdempsky wrote:
> Hm, still slice bounds out of range on OpenBSD:

(And that's OpenBSD/amd64 to clarify.)

[dave_cheney.net, 2013-02-26 22:01:23]

Ok, I'll reduce the scope of this CL to freebsd and netbsd, and leave
issue 3349 open.

On Wed, Feb 27, 2013 at 8:56 AM,  <mdempsky@google.com> wrote:
> On 2013/02/26 21:56:30, mdempsky wrote:
>>
>> Hm, still slice bounds out of range on OpenBSD:
>
>
> (And that's OpenBSD/amd64 to clarify.)
>
> https://codereview.appspot.com/7406050/

[dave_cheney.net, 2013-02-26 22:13:29]

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

syscall: fix FD passing on FreeBSD and NetBSD

Fixes issue 3348.

R=devon.odell, minux.ma, bradfitz, mdempsky
CC=golang-dev
https://codereview.appspot.com/7406050