code review 12843043: net: fix dial to raw IP networks on Windows

net: fix dial to raw IP networks on Windows

Also avoids platform-dependent datagram truncation in raw IP tests.
At least it's different between Windows and others.

Fixes issue 6122.

[mikio, 2013-08-13 07:01:05]

Hello alex.brainman@gmail.com, dvyukov@google.com (cc:
golang-dev@googlegroups.com),

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

[mikio, 2013-08-13 07:08:07]

Sorry, I have no windows stuff; didn't test this CL on windows.
I'm happy if you have a spare time to run "go test -rawip" w/ Windows
special priv., thanks.
Also I expect that some tests, multicast related stuff, will fail on
some versions, but it's fine for now.

On Tue, Aug 13, 2013 at 4:01 PM,  <mikioh.mikioh@gmail.com> wrote:
> Reviewers: brainman, dvyukov,
>
> Message:
> Hello alex.brainman@gmail.com, dvyukov@google.com (cc:
> golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> net: fix dial to raw IP networks on Windows
>
> Also adds -rawip flag to enable raw IP facility access tests on Windows.
>
> Fixes issue 6122.
>
> Please review this at https://codereview.appspot.com/12843043/
>
> Affected files:
>   M src/pkg/net/fd_windows.go
>   M src/pkg/net/ipraw_test.go
>   M src/pkg/net/multicast_test.go
>   M src/pkg/net/packetconn_test.go
>   M src/pkg/net/protoconn_test.go
>
>
> Index: src/pkg/net/fd_windows.go
> ===================================================================
> --- a/src/pkg/net/fd_windows.go
> +++ b/src/pkg/net/fd_windows.go
> @@ -72,7 +72,8 @@
>  }
>
>  func canUseConnectEx(net string) bool {
> -       if net == "udp" || net == "udp4" || net == "udp6" {
> +       switch net {
> +       case "udp", "udp4", "udp6", "ip", "ip4", "ip6":
>                 // ConnectEx windows API does not support connectionless
> sockets.
>                 return false
>         }
> Index: src/pkg/net/ipraw_test.go
> ===================================================================
> --- a/src/pkg/net/ipraw_test.go
> +++ b/src/pkg/net/ipraw_test.go
> @@ -7,13 +7,17 @@
>  import (
>         "bytes"
>         "errors"
> +       "flag"
>         "fmt"
>         "os"
>         "reflect"
> +       "runtime"
>         "testing"
>         "time"
>  )
>
> +var testRawIP = flag.Bool("rawip", false, "run test requires raw IP
> facility access privilege")
> +
>  type resolveIPAddrTest struct {
>         net     string
>         litAddr string
> @@ -72,8 +76,13 @@
>  }
>
>  func TestConnICMPEcho(t *testing.T) {
> -       if os.Getuid() != 0 {
> -               t.Skip("skipping test; must be root")
> +       switch runtime.GOOS {
> +       case "plan9":
> +               t.Skipf("skipping test on %q", runtime.GOOS)
> +       default:
> +               if !*testRawIP {
> +                       t.Skip("test disabled; use -rawip to enable")
> +               }
>         }
>
>         for i, tt := range icmpEchoTests {
> @@ -139,8 +148,13 @@
>  }
>
>  func TestPacketConnICMPEcho(t *testing.T) {
> -       if os.Getuid() != 0 {
> -               t.Skip("skipping test; must be root")
> +       switch runtime.GOOS {
> +       case "plan9":
> +               t.Skipf("skipping test on %q", runtime.GOOS)
> +       default:
> +               if !*testRawIP {
> +                       t.Skip("test disabled; use -rawip to enable")
> +               }
>         }
>
>         for i, tt := range icmpEchoTests {
> @@ -337,8 +351,13 @@
>  }
>
>  func TestIPConnLocalName(t *testing.T) {
> -       if os.Getuid() != 0 {
> -               t.Skip("skipping test; must be root")
> +       switch runtime.GOOS {
> +       case "plan9":
> +               t.Skipf("skipping test on %q", runtime.GOOS)
> +       default:
> +               if !*testRawIP {
> +                       t.Skip("test disabled; use -rawip to enable")
> +               }
>         }
>
>         for _, tt := range ipConnLocalNameTests {
> @@ -354,8 +373,13 @@
>  }
>
>  func TestIPConnRemoteName(t *testing.T) {
> -       if os.Getuid() != 0 {
> -               t.Skip("skipping test; must be root")
> +       switch runtime.GOOS {
> +       case "plan9":
> +               t.Skipf("skipping test on %q", runtime.GOOS)
> +       default:
> +               if !*testRawIP {
> +                       t.Skip("test disabled; use -rawip to enable")
> +               }
>         }
>
>         raddr := &IPAddr{IP: IPv4(127, 0, 0, 10).To4()}
> Index: src/pkg/net/multicast_test.go
> ===================================================================
> --- a/src/pkg/net/multicast_test.go
> +++ b/src/pkg/net/multicast_test.go
> @@ -6,7 +6,6 @@
>
>  import (
>         "fmt"
> -       "os"
>         "runtime"
>         "testing"
>  )
> @@ -95,13 +94,14 @@
>         switch runtime.GOOS {
>         case "plan9", "solaris":
>                 t.Skipf("skipping test on %q", runtime.GOOS)
> +       default:
> +               if !*testRawIP {
> +                       t.Skip("test disabled; use -rawip to enable")
> +               }
>         }
>         if !supportsIPv6 {
>                 t.Skip("ipv6 is not supported")
>         }
> -       if os.Getuid() != 0 {
> -               t.Skip("skipping test; must be root")
> -       }
>
>         closer := func(cs []*UDPConn) {
>                 for _, c := range cs {
> Index: src/pkg/net/packetconn_test.go
> ===================================================================
> --- a/src/pkg/net/packetconn_test.go
> +++ b/src/pkg/net/packetconn_test.go
> @@ -51,9 +51,11 @@
>                         switch runtime.GOOS {
>                         case "plan9":
>                                 continue
> -                       }
> -                       if os.Getuid() != 0 {
> -                               continue
> +                       default:
> +                               if !*testRawIP {
> +                                       t.Log("test disabled; use -rawip to
> enable")
> +                                       continue
> +                               }
>                         }
>                         var err error
>                         wb, err = (&icmpMessage{
> @@ -134,9 +136,11 @@
>                         switch runtime.GOOS {
>                         case "plan9":
>                                 continue
> -                       }
> -                       if os.Getuid() != 0 {
> -                               continue
> +                       default:
> +                               if !*testRawIP {
> +                                       t.Log("test disabled; use -rawip to
> enable")
> +                                       continue
> +                               }
>                         }
>                         var err error
>                         wb, err = (&icmpMessage{
> Index: src/pkg/net/protoconn_test.go
> ===================================================================
> --- a/src/pkg/net/protoconn_test.go
> +++ b/src/pkg/net/protoconn_test.go
> @@ -167,9 +167,10 @@
>         switch runtime.GOOS {
>         case "plan9":
>                 t.Skipf("skipping test on %q", runtime.GOOS)
> -       }
> -       if os.Getuid() != 0 {
> -               t.Skipf("skipping test; must be root")
> +       default:
> +               if !*testRawIP {
> +                       t.Skip("test disabled; use -rawip to enable")
> +               }
>         }
>
>         la, err := ResolveIPAddr("ip4", "127.0.0.1")
>
>

[brainman, 2013-08-13 07:16:43]

On 2013/08/13 07:08:07, mikio wrote:
> Sorry, I have no windows stuff; didn't test this CL on windows.
> I'm happy if you have a spare time to run "go test -rawip" w/ Windows
> special priv., thanks.
> Also I expect that some tests, multicast related stuff, will fail on
> some versions, but it's fine for now.
> 

G:\src\pkg\net>net.test.exe -rawip
--- FAIL: TestConnICMPEcho (0.00 seconds)
        ipraw_test.go:125: Conn.Read failed: WSARecv ip4 127.0.0.1: More data is
available.
--- FAIL: TestPacketConnICMPEcho (0.00 seconds)
        ipraw_test.go:201: PacketConn.ReadFrom failed: WSARecvFrom ip4 0.0.0.0:
More data is available.
--- FAIL: TestIPConnLocalName (0.00 seconds)
        ipraw_test.go:370: IPConn.LocalAddr failed
--- FAIL: TestIPConnRemoteName (0.00 seconds)
        ipraw_test.go:388: DialIP failed: dial ip 127.0.0.10: bind: An invalid
argument was supplied.
FAIL

What should we do now?

Alex

[mikio, 2013-08-13 07:21:40]

Thanks. Wow, looks like,

- I/O thru PacketConn: okay
- I/O thru Conn: ng
- Endpoint property: ng

Hm, I'll try MSDN tomorrow.

On Tue, Aug 13, 2013 at 4:16 PM,  <alex.brainman@gmail.com> wrote:
> On 2013/08/13 07:08:07, mikio wrote:
>>
>> Sorry, I have no windows stuff; didn't test this CL on windows.
>> I'm happy if you have a spare time to run "go test -rawip" w/ Windows
>> special priv., thanks.
>> Also I expect that some tests, multicast related stuff, will fail on
>> some versions, but it's fine for now.
>
>
>
> G:\src\pkg\net>net.test.exe -rawip
> --- FAIL: TestConnICMPEcho (0.00 seconds)
>         ipraw_test.go:125: Conn.Read failed: WSARecv ip4 127.0.0.1: More
> data is available.
> --- FAIL: TestPacketConnICMPEcho (0.00 seconds)
>         ipraw_test.go:201: PacketConn.ReadFrom failed: WSARecvFrom ip4
> 0.0.0.0: More data is available.
> --- FAIL: TestIPConnLocalName (0.00 seconds)
>         ipraw_test.go:370: IPConn.LocalAddr failed
> --- FAIL: TestIPConnRemoteName (0.00 seconds)
>         ipraw_test.go:388: DialIP failed: dial ip 127.0.0.10: bind: An
> invalid argument was supplied.
> FAIL
>
> What should we do now?
>
> Alex
>
> https://codereview.appspot.com/12843043/

[brainman, 2013-08-13 07:27:26]

My previous post was on windows-386 Windows XP.

The following is windows-amd64 Windows 7 Pro:

G:\src\pkg\net>net.test.exe -rawip
--- FAIL: TestConnICMPEcho (0.00 seconds)
        ipraw_test.go:125: Conn.Read failed: WSARecv ip4 127.0.0.1: A message
sent on a datagram socket was larger than the internal message buffer or some
other network limit, or the buffer used to receive a datagram into was smaller
than the datagram itself.
--- FAIL: TestPacketConnICMPEcho (0.00 seconds)
        ipraw_test.go:201: PacketConn.ReadFrom failed: WSARecvFrom ip4 0.0.0.0:
A message sent on a datagram socket was larger than the internal message buffer
or some other network limit, or the buffer used to receive a datagram into was
smaller than the datagram itself.
--- FAIL: TestIPConnLocalName (0.00 seconds)
        ipraw_test.go:370: IPConn.LocalAddr failed
--- FAIL: TestIPConnRemoteName (0.00 seconds)
        ipraw_test.go:388: DialIP failed: dial ip 127.0.0.10: bind: An invalid
argument was supplied.
FAIL

[mikio, 2013-08-13 07:31:37]

sobbing...

On Tue, Aug 13, 2013 at 4:27 PM,  <alex.brainman@gmail.com> wrote:
> My previous post was on windows-386 Windows XP.
>
> The following is windows-amd64 Windows 7 Pro:
>
>
> G:\src\pkg\net>net.test.exe -rawip
> --- FAIL: TestConnICMPEcho (0.00 seconds)
>         ipraw_test.go:125: Conn.Read failed: WSARecv ip4 127.0.0.1: A
> message sent on a datagram socket was larger than the internal message
> buffer or some other network limit, or the buffer used to receive a
> datagram into was smaller than the datagram itself.
>
> --- FAIL: TestPacketConnICMPEcho (0.00 seconds)
>         ipraw_test.go:201: PacketConn.ReadFrom failed: WSARecvFrom ip4
> 0.0.0.0: A message sent on a datagram socket was larger than the
> internal message buffer or some other network limit, or the buffer used
> to receive a datagram into was smaller than the datagram itself.
>
> --- FAIL: TestIPConnLocalName (0.00 seconds)
>         ipraw_test.go:370: IPConn.LocalAddr failed
> --- FAIL: TestIPConnRemoteName (0.00 seconds)
>         ipraw_test.go:388: DialIP failed: dial ip 127.0.0.10: bind: An
> invalid argument was supplied.
> FAIL
>
>
> https://codereview.appspot.com/12843043/

[brainman, 2013-08-21 05:47:26]

mikio,

I think we need to fix this before 1.2. The change is fine, but, please, add a
working test, so we don't break it again.

Alex

[mikio, 2013-08-21 08:27:35]

yup, just sent a few CLs for that purpose; to run raw IP tests on windows.

On Wed, Aug 21, 2013 at 2:47 PM,  <alex.brainman@gmail.com> wrote:
> mikio,
>
> I think we need to fix this before 1.2. The change is fine, but, please,
> add a working test, so we don't break it again.
>
> Alex
>
> https://codereview.appspot.com/12843043/

[brainman, 2013-08-22 01:07:42]

On 2013/08/21 08:27:35, mikio wrote:
> yup, just sent a few CLs for that purpose; to run raw IP tests on windows.

I am not sure which CL you are referring, but, please, add the test to THIS CL
instead. The test will have to fail if changes you made to fd_windows.go in this
CL are reverted. And it would succeed with the change. This is so we won't break
this functionality in the future.

This test will need to be run by our windows builders, so no manual flags will
do. It HAS to run with -short test flag. We need to come up with a way to do it.
You start, and I will try and see if we can run it everywhere. Perhaps it needs
to be run by Administrator on windows, but lets not worry about it until after
we see the test.

Thank you.

Alex

[mikio, 2013-08-22 01:31:09]

Will add test case for this regression once preliminary CLs landed,
12796047 and 12898045. Please take a look If you have a spare time.

On Thu, Aug 22, 2013 at 10:07 AM,  <alex.brainman@gmail.com> wrote:

> This test will need to be run by our windows builders, so no manual
> flags will do. It HAS to run with -short test flag. We need to come up
> with a way to do it. You start, and I will try and see if we can run it
> everywhere. Perhaps it needs to be run by Administrator on windows, but
> lets not worry about it until after we see the test.

Hm, without new flag, fine, but bot sure how to get, deal with the right
privilege for accessing raw IP facilities on Windows. On Unix we can use
Getuid. Do you have any good suggestions?

[brainman, 2013-08-22 01:44:53]

On 2013/08/22 01:31:09, mikio wrote:
> Will add test case for this regression once preliminary CLs landed,
> ...

No. Please do it now. One thing at a time. Otherwise I get confused. I get
confused easily ;-).

> ... 12796047 and 12898045. Please take a look If you have a spare time.

I will. But lets fix this issue first.

> Hm, without new flag, fine, but bot sure how to get, deal with the right
> privilege for accessing raw IP facilities on Windows. On Unix we can use
> Getuid. Do you have any good suggestions?

I already suggested for you not to worry about that. Please, add required test,
then we will see what we need to run it on the builder.

Thank you.

Alex

[mikio, 2013-08-22 02:06:19]

On Thu, Aug 22, 2013 at 10:44 AM,  <alex.brainman@gmail.com> wrote:

> No. Please do it now. One thing at a time. Otherwise I get confused. I
> get confused easily ;-).

As you wish. ;)

>> Getuid. Do you have any good suggestions?
>
> I already suggested for you not to worry about that. Please, add
> required test, then we will see what we need to run it on the builder.

Huh? Sorry, I completely missed that.

[mikio, 2013-08-22 02:56:45]

Here you go.

I think now all tests for raw IP stuff runs on windows except endpoint
property test: TestIPConnLocalName and TestIPConnRemoteName.

Please try TestConnICMPEcho, TestPacketConnICMPEcho and/or
TestPacketConn, TestIPConnSpecificMethods with removing if os.Getuid()
block in each test.

[brainman, 2013-08-23 06:39:30]

On 2013/08/22 02:56:45, mikio wrote:
> 
> Please try TestConnICMPEcho, TestPacketConnICMPEcho and/or
> TestPacketConn, TestIPConnSpecificMethods with removing if os.Getuid()
> block in each test.

Done. Everything runs fine. I suggest you just run these test on windows
unconditionally, even in -short mode. I tested it works fine even on our
builders. If we discover that these test require some special privileges, we
will change them then. Worse comes to worse we can disable it everywhere but on
windows computers that have USERNAME=Administrator environment variable set
(that is how our both windows builders run), but lets wait until we have people
complaining about it first.

Thank you.

Alex

[mikio, 2013-08-23 08:01:48]

Hello alex.brainman@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[mikio, 2013-08-23 08:02:31]

Thanks for the confirmation.

On Fri, Aug 23, 2013 at 3:39 PM,  <alex.brainman@gmail.com> wrote:
> On 2013/08/22 02:56:45, mikio wrote:
>
>> Please try TestConnICMPEcho, TestPacketConnICMPEcho and/or
>> TestPacketConn, TestIPConnSpecificMethods with removing if os.Getuid()
>> block in each test.
>
>
> Done. Everything runs fine. I suggest you just run these test on windows
> unconditionally, even in -short mode. I tested it works fine even on our
> builders. If we discover that these test require some special
> privileges, we will change them then. Worse comes to worse we can
> disable it everywhere but on windows computers that have
> USERNAME=Administrator environment variable set (that is how our both
> windows builders run), but lets wait until we have people complaining
> about it first.
>
>
> Thank you.
>
> Alex
>
> https://codereview.appspot.com/12843043/

[brainman, 2013-08-23 10:01:21]

LGTM

Fingers crossed. :-)

Alex

[mikio, 2013-08-23 10:31:35]

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

net: fix dial to raw IP networks on Windows

Also avoids platform-dependent datagram truncation in raw IP tests.
At least it's different between Windows and others.

Fixes issue 6122.

R=alex.brainman
CC=golang-dev
https://codereview.appspot.com/12843043

[tegoor_gmail.com, 2013-09-02 09:14:46]

--- FAIL: TestConnICMPEcho (0.33 seconds)
        ipraw_test.go:132: Conn.Read failed: WSARecv ip6 ::1: i/o timeout
--- FAIL: TestPacketConnICMPEcho (0.12 seconds)
        ipraw_test.go:210: PacketConn.ReadFrom failed: WSARecvFrom ip6 ::: 
i/o t
imeout
FAIL
FAIL    net     8.203s

ICMPv6 checksum missed?

On Friday, August 23, 2013 6:31:35 PM UTC+8, Mikio Hara wrote:
>
> *** Submitted as 
> https://code.google.com/p/go/source/detail?r=9810b5abd0ab *** 
>
> net: fix dial to raw IP networks on Windows 
>
> Also avoids platform-dependent datagram truncation in raw IP tests. 
> At least it's different between Windows and others. 
>
> Fixes issue 6122. 
>
> R=alex.brainman 
> CC=golang-dev 
> https://codereview.appspot.com/12843043 
>
>
> https://codereview.appspot.com/12843043/ 
>

[mikio, 2013-09-02 21:54:29]

Hi,

> --- FAIL: TestConnICMPEcho (0.33 seconds)
>         ipraw_test.go:132: Conn.Read failed: WSARecv ip6 ::1: i/o timeout
> --- FAIL: TestPacketConnICMPEcho (0.12 seconds)
>         ipraw_test.go:210: PacketConn.ReadFrom failed: WSARecvFrom ip6 :::
> i/o t
> imeout
> FAIL
> FAIL    net     8.203s

Thanks, please file an issue.
Windows version, result of "ping -6 ::1", and captured data on
loopback interface (if possible) might be great help.

> ICMPv6 checksum missed?

I have no clue.