dht-ping crashes when attempting to ping a non-Bittorrent server address

[dborzov]

Attempting to ping a non-server, for example with dht-ping google.com:80 crushes the program after the timeout:

> dht-ping.exe google.com:80
2015/12/04 15:24:50 main.go:34: dht server on [::]:64335
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xc0000005 code=0x1 addr=0x0 pc=0x401a65]

goroutine 1 [running]:
main.main()
        C:/Work/src/github.com/anacrolix/torrent/cmd/dht-ping/main.go:72 +0xa25

goroutine 5 [IO wait]:
net.runtime_pollWait(0x2335620, 0x72, 0xc0820ae520)
        c:/go/src/runtime/netpoll.go:157 +0x67
net.(*pollDesc).Wait(0xc082022d70, 0x72, 0x0, 0x0)
        c:/go/src/net/fd_poll_runtime.go:73 +0x41
net.(*ioSrv).ExecIO(0xc08202c038, 0xc082022c60, 0x7d24a0, 0xb, 0x866880, 0x557e90, 0x0, 0x0)
        c:/go/src/net/fd_windows.go:182 +0x177
net.(*netFD).readFrom(0xc082022c00, 0xc082094000, 0x10000, 0x10000, 0x0, 0x0, 0x0, 0x0, 0x0)
        c:/go/src/net/fd_windows.go:490 +0x1ee
net.(*UDPConn).ReadFromUDP(0xc08202c048, 0xc082094000, 0x10000, 0x10000, 0x0, 0x0, 0x0, 0x0)
        c:/go/src/net/udpsock_posix.go:61 +0x11e
net.(*UDPConn).ReadFrom(0xc08202c048, 0xc082094000, 0x10000, 0x10000, 0x23357a0, 0x0, 0x0, 0x0, 0x0)
        c:/go/src/net/udpsock_posix.go:79 +0x11d
github.com/anacrolix/torrent/dht.(*Server).serve(0xc082054900, 0x0, 0x0)
        C:/Work/src/github.com/anacrolix/torrent/dht/dht.go:574 +0xc4
github.com/anacrolix/torrent/dht.NewServer.func1(0xc08202c030)
        C:/Work/src/github.com/anacrolix/torrent/dht/dht.go:149 +0x2f
created by github.com/anacrolix/torrent/dht.NewServer
        C:/Work/src/github.com/anacrolix/torrent/dht/dht.go:158 +0x28f

goroutine 6 [select]:
github.com/anacrolix/torrent/dht.(*Server).bootstrap(0xc082054900, 0x0, 0x0)
        C:/Work/src/github.com/anacrolix/torrent/dht/dht.go:1030 +0x59f
github.com/anacrolix/torrent/dht.NewServer.func2(0xc08202c030)
        C:/Work/src/github.com/anacrolix/torrent/dht/dht.go:160 +0x2f
created by github.com/anacrolix/torrent/dht.NewServer
        C:/Work/src/github.com/anacrolix/torrent/dht/dht.go:168 +0x2b1

goroutine 18 [semacquire]:
sync.runtime_Semacquire(0xc0820d0e6c)
        c:/go/src/runtime/sema.go:43 +0x2d
sync.(*WaitGroup).Wait(0xc0820d0e60)
        c:/go/src/sync/waitgroup.go:126 +0xbb
github.com/anacrolix/torrent/dht.(*Server).bootstrap.func2(0xc0820d0e60, 0xc0820db740)
        C:/Work/src/github.com/anacrolix/torrent/dht/dht.go:1026 +0x28
created by github.com/anacrolix/torrent/dht.(*Server).bootstrap
        C:/Work/src/github.com/anacrolix/torrent/dht/dht.go:1028 +0x3ed

goroutine 58 [runnable]:
github.com/anacrolix/torrent/dht.(*Transaction).(github.com/anacrolix/torrent/dht.timerCallback)-fm()
        C:/Work/src/github.com/anacrolix/torrent/dht/dht.go:319
created by time.goFunc
        c:/go/src/time/sleep.go:129 +0x41

[anacrolix]

Oh dear, this looks non-trivial.

[anacrolix]

The first fix was bogus, see the later commit. Let me know how it goes.

[dborzov]

Nice fix.
Do you think transaction.userOnResponse() should be called even if the query fails? Or was it just the easiest fix you thought about?

It seems to me that when the ping fails, userOnResponse() is called by transaction.close() which is called a couple of times (!) by transaction.timeout() which is called by transaction.timerCallback() the timer for which is assigned by transaction.startTimer() which is assigned by jitterDuration... this stuff looks pretty convoluted.

What was the logic there? What is the purpose of jitterDuration?

[anacrolix]

I don't actually recall why it works like that, I think it kind of evolved that way. I don't like how complex the response handling is, but it seemed justified at the time.

If you can think of a clearer way, I welcome it. It mainly went this path due to a need to reduce the number of goroutines. When I first wrote the DHT package, the server could easily jump to 200k goroutines, which wasn't compatible with the race detector.

jitterDuration implements slightly randomized timeouts, this is common in many network protocols to reduce the chance of network storms. That is, if a bunch of timeouts all trigger at the same time, a lot of outbound packets are sent too quickly. This occurs when you write a lot of packets over something like UDP, and then set identical timeouts to resend them all.