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

net: re-implement Interfaces and InterfaceAddrs for IPNet, IPv6 on Windows #5395

Closed
gopherbot opened this issue May 2, 2013 · 33 comments

Comments

Projects
None yet
8 participants
@gopherbot
Copy link

commented May 2, 2013

by tom.walsh@expresshosting.net:

The following code returns different results based on the platform:

package main

import (
    "fmt"
    "net"
)

func main() {
    addrs, _ := net.InterfaceAddrs()
    for _, addr := range addrs {
        fmt.Println(addr.String())
    }
}

On Linux it produces:

127.0.0.1/8
192.168.1.10/24
::1/128
fe80::21b:21ff:fe14:3bd3/64

On Windows it produces:

192.168.11.71

0.0.0.0

192.168.1.107

192.168.56.1

Please note the lack of CIDR netmasks


What is the expected output?
Consistent output no matter which platform. I think that including the CIDR is a better
option (Linux has it right, Windows has it wrong)


What do you see instead?
Linux includes the CIDR with the address. Windows omits the CIDR notation.


Which compiler are you using (5g, 6g, 8g, gccgo)?
6g on both platforms


Which operating system are you using?
Linux and Windows


Which version are you using?  (run 'go version')
Windows is 1.1rc1 (also tested with 1.0.2 with the same results), Linux 1.0.3


Please provide any additional information below.
@mikioh

This comment has been minimized.

Copy link
Contributor

commented May 8, 2013

Comment 1:

unfortunately implementing network facility API for Windows does not complete yet. for
now both InterfaceAddrs and Addrs of Interface will return an IPAddr (just an addr)
instead of an IPNet (an address prefix). also it does not fetch IPv6 stuff inside the
kernel.
as usual, we very welcome a series of CLs on this issue.

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

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2013

Comment 3:

Labels changed: added go1.2.

Status changed to Accepted.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2013

Comment 4:

Labels changed: added feature.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2013

Comment 5:

Not for 1.2.

Labels changed: removed go1.2.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2013

Comment 6:

Labels changed: added go1.3maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2013

Comment 7:

Labels changed: removed feature.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2013

Comment 8:

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

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2013

Comment 9:

Labels changed: added repo-main.

@mikioh

This comment has been minimized.

Copy link
Contributor

commented May 8, 2014

Comment 10:

Issue #7955 has been merged into this issue.

@mikioh

This comment has been minimized.

Copy link
Contributor

commented May 8, 2014

Comment 11:

Labels changed: added os-windows, removed priority-later.

@mikioh

This comment has been minimized.

Copy link
Contributor

commented May 8, 2014

Comment 12:

go1.3 and beyond don't need to support windows 2000 which doesn't support IPv6 and fancy
network facilities by default, so writing a patch would be pretty straightforward now.
ah, patches welcome.
@gopherbot

This comment has been minimized.

Copy link
Author

commented May 10, 2014

Comment 13 by everton.marques:

I think one possible fix is to replace net.IPAddr from net/interface_windows.go, line
145, with net.IPNet:
current: 145 ifa := IPAddr{IP: parseIPv4(bytePtrToString(&ipl.IpAddress.String[0]))}
fix: 145 ifa := parseIPNet(ipl.IpAddress.String, ipl.IpMask.String)
(http://golang.org/src/pkg/net/interface_windows.go)
func parseIPNet(addr, mask [16]byte) IPNet {
    ipnet := IPNet{IP: parseIPv4(bytePtrToString(&addr[0]))}
    m := parseIPv4(bytePtrToString(&mask[0]))
    if m == nil {
        ipnet.Mask = IPMask{}
        return ipnet
    }
    p4 := ipnet.IP.To4()
    if len(p4) == IPv4len {
        m4 := m.To4()
        ipnet.Mask = net.IPv4Mask(m4[0], m4[1], m4[2], m4[3])
    } else {
        ipnet.Mask = net.IPMask{
            m[0], m[1], m[2], m[3],
            m[4], m[5], m[6], m[7],
            m[8], m[9], m[10], m[11],
            m[12], m[13], m[14], m[15],
        }
    }
    return ipnet
}
Unfortunately I don't have the skill to actually it. :(
@mikioh

This comment has been minimized.

Copy link
Contributor

commented May 10, 2014

Comment 14:

Thanks, but we don't use here for discussion and/or code review purposes. FWIW, here are
a few hints:
- use GetAdaptersAddresses instead
http://msdn.microsoft.com/en-us/library/windows/desktop/aa365915(v=vs.85).aspx
- then, parse IP_ADAPTER_ADDRESSES instead
http://msdn.microsoft.com/en-us/library/windows/desktop/aa366058(v=vs.85).aspx

@mikioh mikioh changed the title net: implement InterfaceAddrs netmasks on Windows net: re-implement Interfaces and InterfaceAddrs for IPNet, IPv6 on Windows Dec 23, 2014

@mikioh mikioh added the help wanted label Dec 23, 2014

@mattn

This comment has been minimized.

Copy link
Member

commented Jan 16, 2015

It seems that I was working on this. I will send mail to gerrit in later.

https://codereview.appspot.com/4590050/

@mattn

This comment has been minimized.

Copy link
Member

commented Jan 16, 2015

https://codereview.appspot.com/4590050/#msg11

alex says:

interfaceTable2 is using GetAdaptersAddresses() yet. And windows2000 don't have
GetAdaptersAddresses().
When the API is not found, syscall occur error and panic. net package is calling API dynamically.
i.e. it shouldn't back into syscall.

Go1.4 doesn't support windows2k no longer. So is it ok to re-implement by GetAdaptersAddresses() ?

@alexbrainman

This comment has been minimized.

Copy link
Member

commented Jan 16, 2015

Sure @mattn. Hopefuly I will have time to review it. Please no changes to syscall package. We have to put all new syscall changes into internal/syscall/windows. Thank you.

Alex

@mattn

This comment has been minimized.

Copy link
Member

commented Jan 19, 2015

@alexbrainman what structure of internal package do you expect? net/internal ? internal/syscall ?

@alexbrainman

This comment has been minimized.

Copy link
Member

commented Jan 19, 2015

I suggest we put all new windows syscalls into internal/syscall/windows. This we'll be able to use both old syscall and new windows package names in the same source file. We can always change our mind because it is under internal/...

Alex

@mattn

This comment has been minimized.

Copy link
Member

commented Jan 19, 2015

I wonder we should avoid new package name with renaming stdcall like newstdcall?

@alexbrainman

This comment has been minimized.

Copy link
Member

commented Jan 19, 2015

I don't like newstdcall package name. But you can ask others.

@mattn

This comment has been minimized.

Copy link
Member

commented Jan 19, 2015

rename old syscall to stdsyscall?

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2015

I don't like it either. A package's name should describe what it does, not what it contains.

On 19 Jan 2015, at 16:27, alexbrainman notifications@github.com wrote:

I don't like newstdcall package name. But you can ask others.


Reply to this email directly or view it on GitHub.

@alexbrainman

This comment has been minimized.

Copy link
Member

commented Jan 19, 2015

No we cannot touch syscall package. Other people use it.

@mattn

This comment has been minimized.

Copy link
Member

commented Jan 19, 2015

I don't talking about renaming package name of syscall. I'm talking that we must rename old syscall to use in new syscall like below.

internal/syscall/windows.go

package syscall

import (
  stdsyscall "syscall"
)
@alexbrainman

This comment has been minimized.

Copy link
Member

commented Jan 19, 2015

I didn't propose to create internal/syscall package. I proposed to create internal/syscall/windows package. We won't need to rename anything then.

@minux

This comment has been minimized.

Copy link
Member

commented Jan 19, 2015

That's OK. (import "syscall" as stdsyscall in package internal/syscall)

@mattn

This comment has been minimized.

Copy link
Member

commented Jan 19, 2015

I didn't propose to create internal/syscall package. I proposed to create internal/syscall/windows package. We won't need to rename anything then.

fair enough. we must put mksyscall_windows.go into there also.

@alexbrainman

This comment has been minimized.

Copy link
Member

commented Jan 19, 2015

I was hoping we just call mksyscall_windows.go from where it is now.

@minux

This comment has been minimized.

Copy link
Member

commented Jan 19, 2015

I think we should stick to "internal/syscall", instead of
"internal/syscall/windows".

Because the existing use of "internal/syscall“ isn't called
"internal/syscall/linux"
or "internal/syscall/unix".

@mattn

This comment has been minimized.

Copy link
Member

commented Jan 19, 2015

@minux
right!

@alexbrainman

I was hoping we just call mksyscall_windows.go from where it is now.

is this an another issue to file?

@alexbrainman

This comment has been minimized.

Copy link
Member

commented Jan 19, 2015

I think we should stick to "internal/syscall", instead of
"internal/syscall/windows".

I disagree. Once we start using new package we would have to use both old and new in the same source file most of the time. If we name both syscall, then one package would have to be renamed once imported. Why should we think about new name every time we import internal/syscall? Why not come up with good name now, name the package and use it from now on?

Alex

@mattn

This comment has been minimized.

Copy link
Member

commented Jan 19, 2015

@gopherbot

This comment has been minimized.

Copy link
Author

commented Dec 7, 2015

CL https://golang.org/cl/17412 mentions this issue.

@mikioh mikioh added the OS-Windows label Dec 8, 2015

mikioh pushed a commit that referenced this issue Dec 10, 2015

Mikio Hara
net, internal/syscall/windows: fix interface and address identificati…
…on on windows

The current implementation including Go 1.5 through 1.5.2 misuses
Windows API and mishandles the returned values from GetAdapterAddresses
on Windows. This change fixes various issues related to network facility
information by readjusting interface and interface address parsers.

Updates #5395.
Updates #10530.
Updates #12301.
Updates #12551.
Updates #13542.
Fixes #12691.
Fixes #12811.
Fixes #13476.
Fixes #13544.

Also fixes fragile screen scraping test cases in net_windows_test.go.

Additional information for reviewers:

It seems like almost all the issues above have the same root cause and
it is misunderstanding of Windows API. If my interpretation of the
information on MSDN is correctly, current implementation contains the
following bugs:

- SIO_GET_INTERFACE_LIST should not be used for IPv6. The behavior of
  SIO_GET_INTERFACE_LIST is different on kernels and probably it doesn't
  work correctly for IPv6 on old kernels such as Windows XP w/ SP2.
  Unfortunately MSDN doesn't describe the detail of
  SIO_GET_INTERFACE_LIST, but information on the net suggests so.

- Fetching IP_ADAPTER_ADDRESSES structures with fixed size area may not
  work when using IPv6. IPv6 generates ton of interface addresses for
  various addressing scopes. We need to adjust the area appropriately.

- PhysicalAddress field of IP_ADAPTER_ADDRESSES structure may have extra
  space. We cannot ignore PhysicalAddressLength field of
  IP_ADAPTER_ADDRESS structure.

- Flags field of IP_ADAPTER_ADDRESSES structure doesn't represent any of
  administratively and operatinal statuses. It just represents settings
  for windows network adapter.

- MTU field of IP_ADAPTER_ADDRESSES structure may have a uint32(-1) on
  64-bit platform. We need to convert the value to interger
  appropriately.

- IfType field of IP_ADAPTER_ADDRESSES structure is not a bit field.
  Bitwire operation for the field is completely wrong.

- OperStatus field of IP_ADAPTER_ADDRESSES structure is not a bit field.
  Bitwire operation for the field is completely wrong.

- IPv6IfIndex field of IP_ADAPTER_ADDRESSES structure is just a
  substitute for IfIndex field. We cannot prefer IPv6IfIndex to IfIndex.

- Windows XP, 2003 server and below don't set OnLinkPrefixLength field
  of IP_ADAPTER_UNICAST_ADDRESS structure. We cannot rely on the field
  on old kernels. We can use FirstPrefix field of IP_ADAPTER_ADDRESSES
  structure and IP_ADAPTER_PREFIX structure instead.

- Length field of IP_ADAPTER_{UNICAST,ANYCAST,MULTICAST}_ADDRESS
  sturecures doesn't represent an address prefix length. It just
  represents a socket address length.

Change-Id: Icabdaf7bd1d41360a981d2dad0b830b02b584528
Reviewed-on: https://go-review.googlesource.com/17412
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>

@mikioh mikioh modified the milestones: Go1.6, Go1.5 Feb 3, 2016

@golang golang locked and limited conversation to collaborators Feb 3, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.