net: non-nil interface addresses containing nil values

[roger peppe]

I just encountered a widespread issue inside
net package, where addresses with concrete types
are assigned without nil checks to interface-typed values.

I have submitted a CL that should fix the issue, but
testing it will be a pain because it will involve
writing tests for every one of those error return points.
I've erred on the side of overkill, calling intf even when
we statically know that the address is non-nil. YMMV.

There is also some other code which looks wrong,
but may actually be OK.

Thus I'm posting here to get some feedback on the
approach. Another possibility might be simply
to test if Addr.String() == "<nil>" in OpError.Error,
but that feels like too much of a hack.

https://go-review.googlesource.com/#/c/10270

[Mikio Hara]

Please file a new issue.

You should use Addr instead of sockaddr, so helper should be:
func (a *UnixAddr) addr() Addr {
if a == nil {
return nil
}
return a
}

This occurs when a) a setup fails before booking its name, b) an
underlying protocol doesn't support endpoint name. Do you have any
opinions how should we represent a wildcard address in the case of (a)
and (b) ?

[roger peppe]

On 19 May 2015 at 14:46, Mikio Hara <mikioh...@gmail.com> wrote:
> Please file a new issue.

Thanks for the feedback. Will do.

> You should use Addr instead of sockaddr, so helper should be:
> func (a *UnixAddr) addr() Addr {
> if a == nil {
> return nil
> }
> return a
> }

I started off doing that, but much of the code expects to pass
the address to a sockaddr parameter, which we can't do
if the method returns an Address. And the sockaddr method name
is taken.

> This occurs when a) a setup fails before booking its name, b) an
> underlying protocol doesn't support endpoint name. Do you have any
> opinions how should we represent a wildcard address in the case of (a)
> and (b) ?

I'm don't understand this, sorry.

cheers,
rog.

[Mikio Hara]

On Tue, May 19, 2015 at 11:54 PM, roger peppe <rogp...@gmail.com> wrote:

> I started off doing that, but much of the code expects to pass
> the address to a sockaddr parameter

We cannot break plan9 build.

>> This occurs when a) a setup fails before booking its name, b) an
>> underlying protocol doesn't support endpoint name. Do you have any
>> opinions how should we represent a wildcard address in the case of (a)
>> and (b) ?
>
> I'm don't understand this, sorry.

When we kick DialTCP("tcp", nil, &TCPAddr{Port: 5963}), should we
return either a) OpError{Source: nil, Addr: &TCPAddr{Port: 5963}}, or
b) OpError{Source: &TCPAddr{}, Addr: &TCPAddr{Port: 5963}} ?

I guess you'd prefer the former, right?

When we invoke ListenTCP("tcp", nil, &TCPAddr{}), should we return
either a) OpError{Source: nil, Addr: &TCPAddr{}}, or b)
OpError{Source: nil, Addr: nil}} ?

I’d prefer the former but also fine with the latter.

[roger peppe]

On 19 May 2015 at 16:03, Mikio Hara <mikioh...@gmail.com> wrote:
> On Tue, May 19, 2015 at 11:54 PM, roger peppe <rogp...@gmail.com> wrote:
>
>> I started off doing that, but much of the code expects to pass
>> the address to a sockaddr parameter
>
> We cannot break plan9 build.

How would this break the plan9 build?

>>> This occurs when a) a setup fails before booking its name, b) an
>>> underlying protocol doesn't support endpoint name. Do you have any
>>> opinions how should we represent a wildcard address in the case of (a)
>>> and (b) ?
>>
>> I'm don't understand this, sorry.
>
> When we kick DialTCP("tcp", nil, &TCPAddr{Port: 5963}), should we
> return either a) OpError{Source: nil, Addr: &TCPAddr{Port: 5963}}, or
> b) OpError{Source: &TCPAddr{}, Addr: &TCPAddr{Port: 5963}} ?
>
> I guess you'd prefer the former, right?

I don't mind either way. What I *don't* want to see (and what
is happening now) is that we're getting:

OpError{Source: (*TCPAddr)(nil), Addr: &TCPAddr{Port: 5963}}

> When we invoke ListenTCP("tcp", nil, &TCPAddr{}), should we return
> either a) OpError{Source: nil, Addr: &TCPAddr{}}, or b)
> OpError{Source: nil, Addr: nil}} ?
>
> I’d prefer the former but also fine with the latter.

Definitely the former, as that's what we passed in.
But, to reiterate, not OpError{Source: (*TCPAddr)(nil), Addr: &TCPAddr{}}
which is what we get now.

[Mikio Hara]

On Wed, May 20, 2015 at 12:37 AM, roger peppe <rogp...@gmail.com> wrote:

> How would this break the plan9 build?

have you already taken a look at sock_posix.go? sockaddr interface
implements sockaddr method, which returns syscall.Sockaddr interface.
plan9 have no syscall.Sockaddr stuff because it depends on
well-abstracted os.File instead of fragmented socket stuff in the
syscall package.

> Definitely the former, as that's what we passed in.
> But, to reiterate, not OpError{Source: (*TCPAddr)(nil), Addr: &TCPAddr{}}
> which is what we get now.

yup, please open a new issue. i realized that the bug has been existed
from go1 and not revealed until now.

[roger peppe]

On 19 May 2015 at 16:51, Mikio Hara <mikioh...@gmail.com> wrote:
> On Wed, May 20, 2015 at 12:37 AM, roger peppe <rogp...@gmail.com> wrote:
>
>> How would this break the plan9 build?
>
> have you already taken a look at sock_posix.go? sockaddr interface
> implements sockaddr method, which returns syscall.Sockaddr interface.
> plan9 have no syscall.Sockaddr stuff because it depends on
> well-abstracted os.File instead of fragmented socket stuff in the
> syscall package.

Ah, thanks. I hadn't realised that the sockaddr type isn't defined
in the plan 9 code.
One possibility is to define intf differently under plan 9 and posix.
Or we could define sockaddrIntf and addrIntf. Or... (I'm
not keen on the "intf" name but haven't thought of anything better
yet) suggestions?

>> Definitely the former, as that's what we passed in.
>> But, to reiterate, not OpError{Source: (*TCPAddr)(nil), Addr: &TCPAddr{}}
>> which is what we get now.
>
> yup, please open a new issue. i realized that the bug has been existed
> from go1 and not revealed until now.

It wasn't externally visible until now because many operations have been
changed to return OpErrors that did not before.

[Mikio Hara]

On Wed, May 20, 2015 at 1:51 AM, roger peppe <rogp...@gmail.com> wrote:

> One possibility is to define intf differently under plan 9 and posix.

i don't see any good reason to introduce a new internal type. we can
use Addr and there's no reason to hesitate to use it.