code review 11809043: net: make protocol-specific WriteTo, WriteMsg methods r...

net: make protocol-specific WriteTo, WriteMsg methods return error instead of crash

[mikio, 2013-07-25 03:43:10]

Hello golang-dev@googlegroups.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-07-25 11:07:53]

On 2013/07/25 03:43:10, mikio wrote:
> Hello mailto:golang-dev@googlegroups.com (cc:
mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/go

The changes to the test look fine. I am not sure that there is a string
precedent for check if arguments passed to pkg/net methods are non nil. How did
this come up, is there a compelling reason for protecting the caller who
carelessly passed a nil addr to these methods ?

Also, please fix the typo in the issue title.

[mikio, 2013-07-25 14:54:32]

On Thu, Jul 25, 2013 at 8:07 PM,  <dave@cheney.net> wrote:

> How did this come up, is there a compelling reason for protecting the
> caller who carelessly passed a nil addr to these methods ?

Dial rejects nil address for its target, destination address (and
never crashes).
What's the difference btw Dial and WriteTo?

> Also, please fix the typo in the issue title.

Thanks.

[dave_cheney.net, 2013-07-26 03:38:24]

+cc adg/rsc

If you're talking about DialTCP, then I see your reasoning. I think this is a
reasonable improvement to the current behavior, but I would like to hear from
adg or rsc for confirmation.

[mikio, 2013-07-31 03:33:03]

ping

[bradfitz, 2013-08-01 21:27:27]

R=r

If you squint right, this looks like an API change.  Where we once paniced,
we now return an error.

I don't care either way, though.  I doubt it matters.



On Wed, Jul 24, 2013 at 8:43 PM, <mikioh.mikioh@gmail.com> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> net: make protocol-specific WriteTo, WriteMsg methods return error
> instaed of crash
>
> Please review this at
https://codereview.appspot.**com/11809043/<https://codereview.appspot.com/118...
>
> Affected files:
>   M src/pkg/net/iprawsock_posix.go
>   M src/pkg/net/protoconn_test.go
>   M src/pkg/net/udpsock_plan9.go
>   src/pkg/net/udpsock_posix.go
>   M src/pkg/net/unixsock_posix.go
>
>
> Index: src/pkg/net/iprawsock_posix.go
> ==============================**==============================**=======
> --- a/src/pkg/net/iprawsock_posix.**go
> +++ b/src/pkg/net/iprawsock_posix.**go
> @@ -125,6 +125,9 @@
>         if !c.ok() {
>                 return 0, syscall.EINVAL
>         }
> +       if addr == nil {
> +               return 0, &OpError{Op: "write", Net: c.fd.net, Addr: nil,
> Err: errMissingAddress}
> +       }
>         sa, err := addr.sockaddr(c.fd.family)
>         if err != nil {
>                 return 0, &OpError{"write", c.fd.net, addr, err}
> @@ -151,6 +154,9 @@
>         if !c.ok() {
>                 return 0, 0, syscall.EINVAL
>         }
> +       if addr == nil {
> +               return 0, 0, &OpError{Op: "write", Net: c.fd.net, Addr:
> nil, Err: errMissingAddress}
> +       }
>         sa, err := addr.sockaddr(c.fd.family)
>         if err != nil {
>                 return 0, 0, &OpError{"write", c.fd.net, addr, err}
> Index: src/pkg/net/protoconn_test.go
> ==============================**==============================**=======
> --- a/src/pkg/net/protoconn_test.**go
> +++ b/src/pkg/net/protoconn_test.**go
> @@ -160,6 +160,15 @@
>         } else {
>                 f.Close()
>         }
> +
> +       defer func() {
> +               if p := recover(); p != nil {
> +                       t.Fatalf("UDPConn.WriteToUDP or WriteMsgUDP
> panicked: %v", p)
> +               }
> +       }()
> +
> +       c.WriteToUDP(wb, nil)
> +       c.WriteMsgUDP(wb, nil, nil)
>  }
>
>  func TestIPConnSpecificMethods(t *testing.T) {
> @@ -217,6 +226,15 @@
>         } else {
>                 f.Close()
>         }
> +
> +       defer func() {
> +               if p := recover(); p != nil {
> +                       t.Fatalf("IPConn.WriteToIP or WriteMsgIP panicked:
> %v", p)
> +               }
> +       }()
> +
> +       c.WriteToIP(wb, nil)
> +       c.WriteMsgIP(wb, nil, nil)
>  }
>
>  func TestUnixListenerSpecificMethod**s(t *testing.T) {
> @@ -357,4 +375,15 @@
>         } else {
>                 f.Close()
>         }
> +
> +       defer func() {
> +               if p := recover(); p != nil {
> +                       t.Fatalf("UnixConn.WriteToUnix or WriteMsgUnix
> panicked: %v", p)
> +               }
> +       }()
> +
> +       c1.WriteToUnix(wb, nil)
> +       c1.WriteMsgUnix(wb, nil, nil)
> +       c3.WriteToUnix(wb, nil)
> +       c3.WriteMsgUnix(wb, nil, nil)
>  }
> Index: src/pkg/net/udpsock_plan9.go
> ==============================**==============================**=======
> --- a/src/pkg/net/udpsock_plan9.go
> +++ b/src/pkg/net/udpsock_plan9.go
> @@ -73,6 +73,9 @@
>         if !c.ok() || c.fd.data == nil {
>                 return 0, syscall.EINVAL
>         }
> +       if addr == nil {
> +               return 0, &OpError{Op: "write", Net: c.fd.dir, Addr: nil,
> errMissingAddress}
> +       }
>         h := new(udpHeader)
>         h.raddr = addr.IP.To16()
>         h.laddr = c.fd.laddr.(*UDPAddr).IP.To16(**)
> Index: src/pkg/net/udpsock_posix.go
> ==============================**==============================**=======
> --- a/src/pkg/net/udpsock_posix.go
> +++ b/src/pkg/net/udpsock_posix.go
> @@ -121,6 +121,9 @@
>         if c.fd.isConnected {
>                 return 0, &OpError{"write", c.fd.net, addr,
> ErrWriteToConnected}
>         }
> +       if addr == nil {
> +               return 0, &OpError{Op: "write", Net: c.fd.net, Addr: nil,
> Err: errMissingAddress}
> +       }
>         sa, err := addr.sockaddr(c.fd.family)
>         if err != nil {
>                 return 0, &OpError{"write", c.fd.net, addr, err}
> @@ -150,6 +153,9 @@
>         if c.fd.isConnected {
>                 return 0, 0, &OpError{"write", c.fd.net, addr,
> ErrWriteToConnected}
>         }
> +       if addr == nil {
> +               return 0, 0, &OpError{Op: "write", Net: c.fd.net, Addr:
> nil, Err: errMissingAddress}
> +       }
>         sa, err := addr.sockaddr(c.fd.family)
>         if err != nil {
>                 return 0, 0, &OpError{"write", c.fd.net, addr, err}
> Index: src/pkg/net/unixsock_posix.go
> ==============================**==============================**=======
> --- a/src/pkg/net/unixsock_posix.**go
> +++ b/src/pkg/net/unixsock_posix.**go
> @@ -172,7 +172,9 @@
>         if !c.ok() {
>                 return 0, syscall.EINVAL
>         }
> -       if addr.Net != sotypeToNet(c.fd.sotype) {
> +       if addr == nil {
> +               return 0, &OpError{Op: "write", Net: c.fd.net, Addr: nil,
> Err: errMissingAddress}
> +       } else if addr.Net != sotypeToNet(c.fd.sotype) {
>                 return 0, syscall.EAFNOSUPPORT
>         }
>         sa := &syscall.SockaddrUnix{Name: addr.Name}
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[mikio, 2013-08-12 21:55:07]

ping

[rsc, 2013-08-13 20:19:04]

LGTM

https://codereview.appspot.com/11809043/diff/13001/src/pkg/net/unixsock_posix.go
File src/pkg/net/unixsock_posix.go (right):

https://codereview.appspot.com/11809043/diff/13001/src/pkg/net/unixsock_posix...
src/pkg/net/unixsock_posix.go:177: } else if addr.Net !=
sotypeToNet(c.fd.sotype) {
no else after body that returns or goto's etc

if addr == nil {
   return ...
}
if addr.Net != ...

[mikio, 2013-08-18 10:19:47]

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

net: make protocol-specific WriteTo, WriteMsg methods return error instead of
crash

R=golang-dev, dave, rsc, adg, bradfitz
CC=golang-dev
https://codereview.appspot.com/11809043

https://codereview.appspot.com/11809043/diff/13001/src/pkg/net/unixsock_posix.go
File src/pkg/net/unixsock_posix.go (right):

https://codereview.appspot.com/11809043/diff/13001/src/pkg/net/unixsock_posix...
src/pkg/net/unixsock_posix.go:177: } else if addr.Net !=
sotypeToNet(c.fd.sotype) {
On 2013/08/13 20:19:05, rsc wrote:
> no else after body that returns or goto's etc
> 
> if addr == nil {
>    return ...
> }
> if addr.Net != ...

Done.