WireGuard Archive on lore.kernel.org
 help / color / Atom feed
* wireguard-go data race in Peer.SendBuffer()
@ 2019-11-20 19:58 Ben Burkert
  2019-11-27 12:40 ` Jason A. Donenfeld
  0 siblings, 1 reply; 4+ messages in thread
From: Ben Burkert @ 2019-11-20 19:58 UTC (permalink / raw)
  To: wireguard

Greetings,

The Go race detector is reporting a data race in my application due to
what appears to be concurrent calls to Peer.SendHandshakeInitiation().
It seems that it is unsafe to call golang.org/x/sys/unix.SendmsgN()
concurrently due to the sockaddr() implementation which mutates
memory: https://github.com/golang/sys/blob/bd437916bb0eb726b873ee8e9b2dcf212d32e2fd/unix/syscall_aix.go#L53-L55

I believe this data race can be fixed by switching to an exclusive
lock in the Peer.SendBuffer() method:
https://github.com/WireGuard/wireguard-go/blob/f2ea85e9f9921bc29a7dcb2007212b153540c01a/device/peer.go#L144-L145

If it helps I can mail a patch to this effect. I've copied the output
of the race detector below.

Best,
-Ben

==================
==================
WARNING: DATA RACE
Write at 0x00c0011f2740 by goroutine 27:
  golang.org/x/sys/unix.(*SockaddrInet4).sockaddr()
      /go/pkg/mod/golang.org/x/sys@v0.0.0-20191105231009-c1f44814a5cd/unix/syscall_linux.go:384
+0x114
  golang.org/x/sys/unix.SendmsgN()
      /go/pkg/mod/golang.org/x/sys@v0.0.0-20191105231009-c1f44814a5cd/unix/syscall_linux.go:1304
+0x288
  golang.zx2c4.com/wireguard/device.send4()
      /go/pkg/mod/golang.zx2c4.com/wireguard@v0.0.20191012/device/conn_linux.go:485
+0x11f
  golang.zx2c4.com/wireguard/device.(*nativeBind).Send()
      /go/pkg/mod/golang.zx2c4.com/wireguard@v0.0.20191012/device/conn_linux.go:268
+0x1d6
  golang.zx2c4.com/wireguard/device.(*Peer).SendBuffer()
      /go/pkg/mod/golang.zx2c4.com/wireguard@v0.0.20191012/device/peer.go:151
+0x285
  golang.zx2c4.com/wireguard/device.(*Peer).SendHandshakeInitiation()
      /go/pkg/mod/golang.zx2c4.com/wireguard@v0.0.20191012/device/send.go:163
+0x692
  golang.zx2c4.com/wireguard/device.(*Device).RoutineReadFromTUN()
      /go/pkg/mod/golang.zx2c4.com/wireguard@v0.0.20191012/device/send.go:318
+0x4b8

Previous write at 0x00c0011f2740 by goroutine 386:
  golang.org/x/sys/unix.(*SockaddrInet4).sockaddr()
      /go/pkg/mod/golang.org/x/sys@v0.0.0-20191105231009-c1f44814a5cd/unix/syscall_linux.go:384
+0x114
  golang.org/x/sys/unix.SendmsgN()
      /go/pkg/mod/golang.org/x/sys@v0.0.0-20191105231009-c1f44814a5cd/unix/syscall_linux.go:1304
+0x288
  golang.zx2c4.com/wireguard/device.send4()
      /go/pkg/mod/golang.zx2c4.com/wireguard@v0.0.20191012/device/conn_linux.go:485
+0x11f
  golang.zx2c4.com/wireguard/device.(*nativeBind).Send()
      /go/pkg/mod/golang.zx2c4.com/wireguard@v0.0.20191012/device/conn_linux.go:268
+0x1d6
  golang.zx2c4.com/wireguard/device.(*Peer).SendBuffer()
      /go/pkg/mod/golang.zx2c4.com/wireguard@v0.0.20191012/device/peer.go:151
+0x285
  golang.zx2c4.com/wireguard/device.(*Peer).SendHandshakeInitiation()
      /go/pkg/mod/golang.zx2c4.com/wireguard@v0.0.20191012/device/send.go:163
+0x692
  golang.zx2c4.com/wireguard/device.expiredRetransmitHandshake()
      /go/pkg/mod/golang.zx2c4.com/wireguard@v0.0.20191012/device/timers.go:110
+0x40c
  golang.zx2c4.com/wireguard/device.(*Peer).NewTimer.func1()
      /go/pkg/mod/golang.zx2c4.com/wireguard@v0.0.20191012/device/timers.go:42
+0xd8

Goroutine 27 (running) created at:
  golang.zx2c4.com/wireguard/device.NewDevice()
      /go/pkg/mod/golang.zx2c4.com/wireguard@v0.0.20191012/device/device.go:322
+0x5e8
  main.main()
      /go/src/x/main.go:102 +0x58e

Goroutine 386 (finished) created at:
  time.goFunc()
      /usr/local/go/src/time/sleep.go:168 +0x51
==================
_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: wireguard-go data race in Peer.SendBuffer()
  2019-11-20 19:58 wireguard-go data race in Peer.SendBuffer() Ben Burkert
@ 2019-11-27 12:40 ` Jason A. Donenfeld
  2019-11-27 17:47   ` Ben Burkert
  0 siblings, 1 reply; 4+ messages in thread
From: Jason A. Donenfeld @ 2019-11-27 12:40 UTC (permalink / raw)
  To: Ben Burkert; +Cc: WireGuard mailing list

Thanks for the report. Does this fix it for you?

https://git.zx2c4.com/wireguard-go/commit/?id=cd26ba8ee4a523437aba3b489422127f1293a16f
_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: wireguard-go data race in Peer.SendBuffer()
  2019-11-27 12:40 ` Jason A. Donenfeld
@ 2019-11-27 17:47   ` Ben Burkert
  2019-11-28 10:09     ` Jason A. Donenfeld
  0 siblings, 1 reply; 4+ messages in thread
From: Ben Burkert @ 2019-11-27 17:47 UTC (permalink / raw)
  To: Jason A. Donenfeld; +Cc: WireGuard mailing list

Thanks, unfortunately I'm getting a build error now. Looks like the
patch needs to switch "sync.Lock" to "sync.Mutex". I'm testing this
change and haven't hit any race conditions yet, I'll let you know if I
see any more issues.

On Wed, Nov 27, 2019 at 4:40 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Thanks for the report. Does this fix it for you?
>
> https://git.zx2c4.com/wireguard-go/commit/?id=cd26ba8ee4a523437aba3b489422127f1293a16f
_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: wireguard-go data race in Peer.SendBuffer()
  2019-11-27 17:47   ` Ben Burkert
@ 2019-11-28 10:09     ` Jason A. Donenfeld
  0 siblings, 0 replies; 4+ messages in thread
From: Jason A. Donenfeld @ 2019-11-28 10:09 UTC (permalink / raw)
  To: Ben Burkert; +Cc: WireGuard mailing list

How on earth did that get committed... Sorry. Fixed.
_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-20 19:58 wireguard-go data race in Peer.SendBuffer() Ben Burkert
2019-11-27 12:40 ` Jason A. Donenfeld
2019-11-27 17:47   ` Ben Burkert
2019-11-28 10:09     ` Jason A. Donenfeld

WireGuard Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/wireguard/0 wireguard/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 wireguard wireguard/ https://lore.kernel.org/wireguard \
		wireguard@lists.zx2c4.com
	public-inbox-index wireguard

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/com.zx2c4.lists.wireguard


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git