adhole icon indicating copy to clipboard operation
adhole copied to clipboard
verified publisher icon drbig

The global queries is accessed by multiple goroutines, why not lock for it?

Open jannson opened this issue 9 years ago • 6 comments

Like the code here: https://github.com/drbig/adhole/blob/master/adhole/main.go#L333 Or here: https://github.com/drbig/adhole/blob/master/adhole/main.go#L341

jannson avatar Mar 08 '16 04:03 jannson

Because there's no reason to do so.

A query entry is written only in one place, and wherever it's accessed it has to be there for the access to succeed, ergo it doesn't need locking. Unless you can find a case where you can hack it to blow up :) Feel free to do so, it has been running smoothly 24h/7d for > 2 years now, but I never claimed it's bulletproof.

drbig avatar Mar 08 '16 22:03 drbig

I think I've found a place where it blows up. Some information about the circumstances:

  • Compiled from commit 9e54de5
  • Using go1.7.3 on OSX
  • I had two browsers trying to resolve the same domain if that is of any concern

Here there is the trace from the crash:

$ sudo adhole -hport 8080 not-so-secure 192.168.6.1 127.0.0.1 ~/playfield/ad-servers.txt
Password:
2016/10/27 21:45:20 DNS: Parsed 12932 entries from list
2016/10/27 21:45:20 HTTP: Started at 127.0.0.1:8080
2016/10/27 21:45:20 DNS: Started upstream server
2016/10/27 21:45:20 DNS: Started local server at 127.0.0.1:53
fatal error: concurrent map writes

goroutine 13 [running]:
runtime.throw(0x28fe21, 0x15)
    /usr/local/go/src/runtime/panic.go:566 +0x95 fp=0xc420246c60 sp=0xc420246c40
runtime.mapassign1(0x2417c0, 0xc420077890, 0xc420246dd8, 0xc420246e68)
    /usr/local/go/src/runtime/hashmap.go:458 +0x8ef fp=0xc420246d48 sp=0xc420246c60
main.handleDNS(0xc420230240, 0x27, 0x27, 0xc420230210)
    /Users/iron4o/gopath/src/github.com/drbig/adhole/adhole/main.go:328 +0x9d1 fp=0xc420246fa0 sp=0xc420246d48
runtime.goexit()
    /usr/local/go/src/runtime/asm_amd64.s:2086 +0x1 fp=0xc420246fa8 sp=0xc420246fa0
created by main.runServerLocalDNS
    /Users/iron4o/gopath/src/github.com/drbig/adhole/adhole/main.go:223 +0x3b7

goroutine 1 [chan receive]:
main.sigwait()
    /Users/iron4o/gopath/src/github.com/drbig/adhole/adhole/sigwait_unix.go:18 +0x1b4
main.main()
    /Users/iron4o/gopath/src/github.com/drbig/adhole/adhole/main.go:168 +0x51c

goroutine 17 [syscall, locked to thread]:
runtime.goexit()
    /usr/local/go/src/runtime/asm_amd64.s:2086 +0x1

goroutine 19 [syscall]:
os/signal.signal_recv(0x0)
    /usr/local/go/src/runtime/sigqueue.go:116 +0x157
os/signal.loop()
    /usr/local/go/src/os/signal/signal_unix.go:22 +0x22
created by os/signal.init.1
    /usr/local/go/src/os/signal/signal_unix.go:28 +0x41

goroutine 20 [IO wait]:
net.runtime_pollWait(0x4c8058, 0x72, 0x0)
    /usr/local/go/src/runtime/netpoll.go:160 +0x59
net.(*pollDesc).wait(0xc4201fa0d0, 0x72, 0xc420204c60, 0xc420074150)
    /usr/local/go/src/net/fd_poll_runtime.go:73 +0x38
net.(*pollDesc).waitRead(0xc4201fa0d0, 0x3969a0, 0xc420074150)
    /usr/local/go/src/net/fd_poll_runtime.go:78 +0x34
net.(*netFD).accept(0xc4201fa070, 0x0, 0x395420, 0xc4201fc100)
    /usr/local/go/src/net/fd_unix.go:419 +0x238
net.(*TCPListener).accept(0xc420216000, 0xc4202140c0, 0xc420204d70, 0x165a70)
    /usr/local/go/src/net/tcpsock_posix.go:132 +0x2e
net.(*TCPListener).AcceptTCP(0xc420216000, 0xc4201fe05c, 0xc420204d60, 0xfd2c0)
    /usr/local/go/src/net/tcpsock.go:209 +0x49
net/http.tcpKeepAliveListener.Accept(0xc420216000, 0xc420214090, 0x23e8e0, 0x3ab220, 0x25c6c0)
    /usr/local/go/src/net/http/server.go:2608 +0x2f
net/http.(*Server).Serve(0xc4201fe000, 0x398e60, 0xc420216000, 0x0, 0x0)
    /usr/local/go/src/net/http/server.go:2273 +0x1ce
net/http.(*Server).ListenAndServe(0xc4201fe000, 0xc4201fe000, 0x0)
    /usr/local/go/src/net/http/server.go:2219 +0xb4
net/http.ListenAndServe(0xc4201f2030, 0xe, 0x0, 0x0, 0xc4201f2050, 0xc4201f2030)
    /usr/local/go/src/net/http/server.go:2351 +0xa0
main.runServerHTTP(0xc4201b7674, 0x9)
    /Users/iron4o/gopath/src/github.com/drbig/adhole/adhole/main.go:395 +0x2de
created by main.main
    /Users/iron4o/gopath/src/github.com/drbig/adhole/adhole/main.go:164 +0x4e7

goroutine 21 [IO wait]:
net.runtime_pollWait(0x4c81d8, 0x72, 0x0)
    /usr/local/go/src/runtime/netpoll.go:160 +0x59
net.(*pollDesc).wait(0xc42009a0d0, 0x72, 0xc42003daf8, 0xc420074150)
    /usr/local/go/src/net/fd_poll_runtime.go:73 +0x38
net.(*pollDesc).waitRead(0xc42009a0d0, 0x3969a0, 0xc420074150)
    /usr/local/go/src/net/fd_poll_runtime.go:78 +0x34
net.(*netFD).readFrom(0xc42009a070, 0xc42003dcc8, 0x200, 0x200, 0x0, 0x0, 0x0, 0x3969a0, 0xc420074150)
    /usr/local/go/src/net/fd_unix.go:270 +0x1e9
net.(*UDPConn).readFrom(0xc420096028, 0xc42003dcc8, 0x200, 0x200, 0x0, 0x5d, 0x71e7a8024bc33d01, 0x7100000000000000)
    /usr/local/go/src/net/udpsock_posix.go:43 +0x6a
net.(*UDPConn).ReadFromUDP(0xc420096028, 0xc42003dcc8, 0x200, 0x200, 0x3975a0, 0xc420212330, 0x5d, 0x0)
    /usr/local/go/src/net/udpsock.go:85 +0x75
main.runServerUpstreamDNS()
    /Users/iron4o/gopath/src/github.com/drbig/adhole/adhole/main.go:233 +0x12d
created by main.main
    /Users/iron4o/gopath/src/github.com/drbig/adhole/adhole/main.go:165 +0x4ff

goroutine 22 [IO wait]:
net.runtime_pollWait(0x4c8118, 0x72, 0x0)
    /usr/local/go/src/runtime/netpoll.go:160 +0x59
net.(*pollDesc).wait(0xc42009a140, 0x72, 0xc420038b58, 0xc420074150)
    /usr/local/go/src/net/fd_poll_runtime.go:73 +0x38
net.(*pollDesc).waitRead(0xc42009a140, 0x3969a0, 0xc420074150)
    /usr/local/go/src/net/fd_poll_runtime.go:78 +0x34
net.(*netFD).readFrom(0xc42009a0e0, 0xc420038d10, 0x200, 0x200, 0x0, 0x0, 0x0, 0x3969a0, 0xc420074150)
    /usr/local/go/src/net/fd_unix.go:270 +0x1e9
net.(*UDPConn).readFrom(0xc420096030, 0xc420038d10, 0x200, 0x200, 0x43b0b, 0xc420038ca8, 0x3758e, 0xc420038c80)
    /usr/local/go/src/net/udpsock_posix.go:43 +0x6a
net.(*UDPConn).ReadFromUDP(0xc420096030, 0xc420038d10, 0x200, 0x200, 0x27, 0xc420230210, 0x0, 0x0)
    /usr/local/go/src/net/udpsock.go:85 +0x75
main.runServerLocalDNS()
    /Users/iron4o/gopath/src/github.com/drbig/adhole/adhole/main.go:213 +0x1b0
created by main.main
    /Users/iron4o/gopath/src/github.com/drbig/adhole/adhole/main.go:166 +0x517

goroutine 23 [select, locked to thread]:
runtime.gopark(0x2ad6b8, 0x0, 0x28bb1b, 0x6, 0x18, 0x2)
    /usr/local/go/src/runtime/proc.go:259 +0x13a
runtime.selectgoImpl(0xc420026f30, 0x0, 0x18)
    /usr/local/go/src/runtime/select.go:423 +0x11d9
runtime.selectgo(0xc420026f30)
    /usr/local/go/src/runtime/select.go:238 +0x1c
runtime.ensureSigM.func1()
    /usr/local/go/src/runtime/signal1_unix.go:304 +0x2d1
runtime.goexit()
    /usr/local/go/src/runtime/asm_amd64.s:2086 +0x1

goroutine 33 [sleep]:
time.Sleep(0x12a05f200)
    /usr/local/go/src/runtime/time.go:59 +0xe1
main.handleDNS.func1(0x41e6)
    /Users/iron4o/gopath/src/github.com/drbig/adhole/adhole/main.go:337 +0x37
created by main.handleDNS
    /Users/iron4o/gopath/src/github.com/drbig/adhole/adhole/main.go:344 +0xb94

My advice is to always lock around maps if there are more than one goroutines which use them. I do suspect my word does not weight much on the subject, but Russ Cox's probably does. Also, there is this excellent article on the golang blog.

I can see how easily a writing in the queries can coincide with a read which is happening in different goroutine from a previous handleDNS call.

Judging by your travis file I suspect that you've compiled your code with go1.3. So it is not a surprise that it has never crashed for you. For this version, GOMAXPROCS was still 1 by default which meant that no concurrent access was possible whatsoever.

If your lovely project fits my needs I would probably make a pull request which fixes this small issues.

ironsmile avatar Oct 27 '16 19:10 ironsmile

@ironsmile :D Thanks for a great issue submission. Indeed my first impression was go1.7.3 being the change here. Indeed adhole runs single process on my "production rpi" (which can't really run any more than one, but that's not the point).

Locking on both sides sounds like the proper solution. I'll very much welcome a PR for this, but this being now a real issue I'll fix it anyway whenever I get some free time and spare brain cycles. Thanks again!

drbig avatar Oct 27 '16 22:10 drbig

@jannson and now it seems you've been onto something. Seems I've taken this too lightly, which indeed is my bad.

drbig avatar Oct 27 '16 22:10 drbig

@jannson @ironsmile Hey guys, PRed a fix -> https://github.com/drbig/adhole/pull/3 but can't really test it with my RPi setup. Would prefer to merge after someone actually tested it in a reasonable environment. It'd be very much appreciated if you guys could give it a try :)

drbig avatar Nov 11 '16 19:11 drbig

Will do! Would be one of the first things I do once I am back from a short codeless vacation :)

ironsmile avatar Nov 13 '16 08:11 ironsmile