Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revisit ID randomness decision based on new performance data #1037

Closed
jsha opened this issue Nov 15, 2019 · 10 comments
Closed

Revisit ID randomness decision based on new performance data #1037

jsha opened this issue Nov 15, 2019 · 10 comments

Comments

@jsha
Copy link
Contributor

jsha commented Nov 15, 2019 •
edited
Loading

In #48 and #102 some benchmarking data was provided showing that math/rand is faster than crypto/rand (for use in generating query IDs). However, those tests took too narrow a view. It's important to look at ID generation in the actual context in which it's used.

I wrote a more robust benchmark that stands up a trivial DNS server, and sends a lot of queries at it. You can fiddle with values of parallel to try and max out local CPU. On my laptop, -parallel 40 came close to maxing out my CPUs, and I was able to get an average qps of 147,210 for crypto/rand vs 147,350 for math/rand. This is a pretty tiny difference, and that's in optimal conditions. Across a real network, the qps would be much lower, which means the impact of using crypto/rand would be much lower.

I'd like to propose making crypto/rand the default, so users don't have to worry about whether they need more secure ID generation.

$ go run main.go 
2019/11/14 16:43:11    math/rand N = 100000, parallel = 40, qps = 149075.287112
2019/11/14 16:43:12  crypto/rand N = 100000, parallel = 40, qps = 149218.081585
2019/11/14 16:43:13    math/rand N = 100000, parallel = 40, qps = 146252.205194
2019/11/14 16:43:13  crypto/rand N = 100000, parallel = 40, qps = 147203.384613
2019/11/14 16:43:14    math/rand N = 100000, parallel = 40, qps = 147398.219853
2019/11/14 16:43:15  crypto/rand N = 100000, parallel = 40, qps = 147448.185616
2019/11/14 16:43:15    math/rand N = 100000, parallel = 40, qps = 147396.953876
2019/11/14 16:43:16  crypto/rand N = 100000, parallel = 40, qps = 145910.799292
2019/11/14 16:43:17    math/rand N = 100000, parallel = 40, qps = 146785.044813
2019/11/14 16:43:18  crypto/rand N = 100000, parallel = 40, qps = 147818.343743
2019/11/14 16:43:18    math/rand N = 100000, parallel = 40, qps = 146877.602488
2019/11/14 16:43:19  crypto/rand N = 100000, parallel = 40, qps = 147186.064584
2019/11/14 16:43:20    math/rand N = 100000, parallel = 40, qps = 148628.014722
2019/11/14 16:43:20  crypto/rand N = 100000, parallel = 40, qps = 146466.368147
2019/11/14 16:43:21    math/rand N = 100000, parallel = 40, qps = 146388.435965
2019/11/14 16:43:22  crypto/rand N = 100000, parallel = 40, qps = 146432.207382
@jsha jsha changed the title Revisit randomness decision based on new performance data Revisit ID randomness decision based on new performance data Nov 15, 2019
@jsha
Copy link
Contributor Author

jsha commented Nov 15, 2019

Also, here's a CPU profile of that benchmark showing the CPU usage of crypto/rand is very low relative to the rest of the program.

$ ~/go/bin/pprof -text cpu.prof
File: ben.test
Type: cpu
Time: Nov 14, 2019 at 5:02pm (PST)
Duration: 12.02s, Total samples = 28.90s (240.46%)
Showing nodes accounting for 23.46s, 81.18% of 28.90s total
Dropped 345 nodes (cum <= 0.14s)
      flat  flat%   sum%        cum   cum%
     8.26s 28.58% 28.58%      8.68s 30.03%  syscall.Syscall
     1.45s  5.02% 33.60%      1.45s  5.02%  runtime.epollwait
     1.39s  4.81% 38.41%      1.39s  4.81%  syscall.RawSyscall
     1.19s  4.12% 42.53%      1.19s  4.12%  runtime.epollctl
     0.69s  2.39% 44.91%      0.69s  2.39%  runtime.futex
     0.59s  2.04% 46.96%      0.66s  2.28%  runtime.step
     0.55s  1.90% 48.86%      0.55s  1.90%  runtime.usleep
     0.41s  1.42% 50.28%      0.78s  2.70%  runtime.lock
     0.41s  1.42% 51.70%      0.68s  2.35%  runtime.scanobject
     0.40s  1.38% 53.08%      3.35s 11.59%  runtime.mallocgc
     0.37s  1.28% 54.36%      1.07s  3.70%  runtime.pcvalue
     0.34s  1.18% 55.54%      0.34s  1.18%  runtime.procyield
     0.30s  1.04% 56.57%      1.90s  6.57%  runtime.gentraceback
     0.28s  0.97% 57.54%      0.28s  0.97%  runtime.nextFreeFast
     0.26s   0.9% 58.44%      0.37s  1.28%  runtime.heapBitsSetType
     0.25s  0.87% 59.31%      3.61s 12.49%  runtime.findrunnable
     0.25s  0.87% 60.17%      0.79s  2.73%  runtime.runqgrab
     0.24s  0.83% 61.00%      0.27s  0.93%  runtime.casgstatus
     0.23s   0.8% 61.80%      0.24s  0.83%  runtime.unlock
     0.22s  0.76% 62.56%      0.24s  0.83%  syscall.Syscall6
     0.19s  0.66% 63.22%      0.35s  1.21%  github.com/miekg/dns.packDomainName
     0.19s  0.66% 63.88%      2.33s  8.06%  runtime.newobject
     0.18s  0.62% 64.50%      0.19s  0.66%  runtime.deferreturn
     0.18s  0.62% 65.12%      0.89s  3.08%  runtime.getStackMap
     0.17s  0.59% 65.71%      0.20s  0.69%  runtime.findObject
     0.16s  0.55% 66.26%      0.16s  0.55%  runtime.memclrNoHeapPointers
     0.13s  0.45% 66.71%      0.21s  0.73%  runtime.reentersyscall
     0.12s  0.42% 67.13%      2.54s  8.79%  github.com/miekg/dns.(*Msg).unpack
     0.12s  0.42% 67.54%      3.94s 13.63%  runtime.schedule
     0.10s  0.35% 67.89%      2.10s  7.27%  github.com/miekg/dns.UnpackDomainName
     0.10s  0.35% 68.24%      0.29s  1.00%  internal/poll.runtime_pollSetDeadline
     0.10s  0.35% 68.58%      0.46s  1.59%  net.(*Resolver).internetAddrList
     0.10s  0.35% 68.93%      0.38s  1.31%  runtime.growslice
     0.09s  0.31% 69.24%      0.27s  0.93%  internal/poll.(*fdMutex).rwlock
     0.09s  0.31% 69.55%      1.70s  5.88%  runtime.netpoll
     0.08s  0.28% 69.83%     14.29s 49.45%  github.com/miekg/dns.(*Client).exchange
     0.08s  0.28% 70.10%      0.81s  2.80%  github.com/miekg/dns.(*Msg).packBufferWithCompressionMap
     0.08s  0.28% 70.38%      5.23s 18.10%  net.(*Dialer).DialContext
     0.08s  0.28% 70.66%      0.46s  1.59%  runtime.(*mcache).prepareForSweep
     0.08s  0.28% 70.93%      0.93s  3.22%  runtime.gcDrainN
     0.07s  0.24% 71.18%     14.36s 49.69%  github.com/miekg/dns.(*Client).Exchange
     0.07s  0.24% 71.42%      0.20s  0.69%  internal/poll.(*fdMutex).rwunlock
     0.07s  0.24% 71.66%      3.05s 10.55%  net.(*UDPConn).writeMsg
     0.07s  0.24% 71.90%      0.32s  1.11%  runtime.(*mcentral).uncacheSpan
     0.07s  0.24% 72.15%      0.21s  0.73%  runtime.exitsyscall
     0.06s  0.21% 72.35%      6.47s 22.39%  github.com/miekg/dns.(*Server).serveDNS
     0.06s  0.21% 72.56%      1.42s  4.91%  net.(*netFD).Read
     0.06s  0.21% 72.77%      3.77s 13.04%  net.(*sysDialer).dialSingle
     0.06s  0.21% 72.98%      0.90s  3.11%  runtime.adjustframe
     0.06s  0.21% 73.18%      0.25s  0.87%  runtime.semacquire1
     0.06s  0.21% 73.39%      2.74s  9.48%  runtime.systemstack
     0.05s  0.17% 73.56%      1.44s  4.98%  github.com/miekg/dns.(*Server).readUDP
     0.05s  0.17% 73.74%      0.18s  0.62%  internal/poll.(*pollDesc).wait
     0.05s  0.17% 73.91%      0.37s  1.28%  io.ReadAtLeast
     0.05s  0.17% 74.08%      0.50s  1.73%  runtime.(*mspan).sweep
     0.05s  0.17% 74.26%      0.71s  2.46%  runtime.makeslice
     0.04s  0.14% 74.39%      0.39s  1.35%  context.WithDeadline
     0.04s  0.14% 74.53%      0.32s  1.11%  crypto/rand.(*devReader).Read
     0.04s  0.14% 74.67%      6.83s 23.63%  github.com/miekg/dns.(*Server).serveUDPPacket
     0.04s  0.14% 74.81%      2.91s 10.07%  net.(*netFD).writeMsg
     0.04s  0.14% 74.95%      0.17s  0.59%  net.filterAddrList
     0.04s  0.14% 75.09%      0.54s  1.87%  runtime.(*mcentral).cacheSpan
     0.04s  0.14% 75.22%      0.83s  2.87%  runtime.runqsteal
     0.04s  0.14% 75.36%      0.19s  0.66%  sync.(*Pool).Get
     0.04s  0.14% 75.50%      0.26s   0.9%  syscall.anyToSockaddr
     0.03s   0.1% 75.61%     15.08s 52.18%  _/home/jsha/ben.doRun.func1
     0.03s   0.1% 75.71%      0.84s  2.91%  github.com/miekg/dns.(*Msg).PackBuffer
     0.03s   0.1% 75.81%      0.40s  1.38%  github.com/miekg/dns.(*Question).pack
     0.03s   0.1% 75.92%      3.48s 12.04%  github.com/miekg/dns.WriteToSessionUDP
     0.03s   0.1% 76.02%      1.36s  4.71%  internal/poll.(*FD).Read
     0.03s   0.1% 76.12%      0.21s  0.73%  internal/poll.(*FD).writeUnlock
     0.03s   0.1% 76.23%      0.21s  0.73%  internal/poll.(*pollDesc).waitRead
     0.03s   0.1% 76.33%      0.57s  1.97%  net.(*Resolver).resolveAddrList
     0.03s   0.1% 76.44%      0.17s  0.59%  net.(*UDPAddr).sockaddr
     0.03s   0.1% 76.54%      1.28s  4.43%  net.(*netFD).connect
     0.03s   0.1% 76.64%      2.04s  7.06%  net.(*netFD).dial
     0.03s   0.1% 76.75%      3.89s 13.46%  net.(*sysDialer).dialSerial
     0.03s   0.1% 76.85%      3.71s 12.84%  net.(*sysDialer).dialUDP
     0.03s   0.1% 76.96%      0.57s  1.97%  runtime.(*mcache).refill
     0.03s   0.1% 77.06%      0.20s  0.69%  runtime.injectglist
     0.03s   0.1% 77.16%      0.16s  0.55%  runtime.netpollready
     0.03s   0.1% 77.27%      3.88s 13.43%  runtime.park_m
     0.03s   0.1% 77.37%      0.32s  1.11%  runtime.sweepone
     0.03s   0.1% 77.47%      0.28s  0.97%  syscall.Getsockname
     0.03s   0.1% 77.58%      1.05s  3.63%  syscall.Read
     0.02s 0.069% 77.65%      4.03s 13.94%  _/home/jsha/ben.dnsServer.func1
     0.02s 0.069% 77.72%      1.70s  5.88%  github.com/miekg/dns.(*Conn).ReadMsgHeader
     0.02s 0.069% 77.79%      3.06s 10.59%  github.com/miekg/dns.(*Conn).WriteMsg
     0.02s 0.069% 77.85%      0.63s  2.18%  github.com/miekg/dns.(*Msg).SetQuestion
     0.02s 0.069% 77.92%      1.71s  5.92%  github.com/miekg/dns.(*Server).serveUDP
     0.02s 0.069% 77.99%      4.05s 14.01%  github.com/miekg/dns.HandlerFunc.ServeDNS
     0.02s 0.069% 78.06%      2.12s  7.34%  github.com/miekg/dns.unpackQuestion
     0.02s 0.069% 78.13%      0.17s  0.59%  golang.org/x/net/ipv6.(*ControlMessage).Parse
     0.02s 0.069% 78.20%      2.52s  8.72%  internal/poll.(*FD).Write
     0.02s 0.069% 78.27%      2.87s  9.93%  internal/poll.(*FD).WriteMsg
     0.02s 0.069% 78.34%      2.21s  7.65%  internal/poll.(*FD).decref
     0.02s 0.069% 78.41%      0.40s  1.38%  internal/poll.setDeadlineImpl
     0.02s 0.069% 78.48%      2.56s  8.86%  net.(*conn).Write
     0.02s 0.069% 78.55%      2.54s  8.79%  net.(*netFD).Write
     0.02s 0.069% 78.62%      3.64s 12.60%  net.internetSocket
     0.02s 0.069% 78.69%      0.34s  1.18%  runtime.(*mcache).releaseAll
     0.02s 0.069% 78.75%      0.25s  0.87%  runtime.SetFinalizer
     0.02s 0.069% 78.82%      1.73s  5.99%  runtime.copystack
     0.02s 0.069% 78.89%      0.32s  1.11%  runtime.deductSweepCredit
     0.02s 0.069% 78.96%      0.23s   0.8%  runtime.entersyscall
     0.02s 0.069% 79.03%      0.45s  1.56%  runtime.funcspdelta
     0.02s 0.069% 79.10%      0.96s  3.32%  runtime.gcAssistAlloc1
     0.02s 0.069% 79.17%      0.30s  1.04%  runtime.notesleep
     0.02s 0.069% 79.24%      0.29s  1.00%  runtime.notewakeup
     0.02s 0.069% 79.31%      0.15s  0.52%  runtime.semrelease1
     0.02s 0.069% 79.38%      0.24s  0.83%  runtime.slicebytetostring
     0.02s 0.069% 79.45%      0.40s  1.38%  runtime.stopm
     0.02s 0.069% 79.52%      1.35s  4.67%  syscall.Close
     0.02s 0.069% 79.58%      2.37s  8.20%  syscall.SendmsgN
     0.01s 0.035% 79.62%      0.42s  1.45%  _/home/jsha/ben.TestM.func1
     0.01s 0.035% 79.65%      0.28s  0.97%  crypto/rand.batched.func1
     0.01s 0.035% 79.69%      0.38s  1.31%  encoding/binary.Read
     0.01s 0.035% 79.72%      5.42s 18.75%  github.com/miekg/dns.(*Client).Dial
     0.01s 0.035% 79.76%      2.73s  9.45%  github.com/miekg/dns.(*Conn).ReadMsg
     0.01s 0.035% 79.79%      2.60s  9.00%  github.com/miekg/dns.(*Conn).Write
     0.01s 0.035% 79.83%      0.85s  2.94%  github.com/miekg/dns.(*Msg).Pack
     0.01s 0.035% 79.86%      0.53s  1.83%  github.com/miekg/dns.(*Msg).Unpack
     0.01s 0.035% 79.90%      3.91s 13.53%  github.com/miekg/dns.(*response).WriteMsg
     0.01s 0.035% 79.93%      0.18s  0.62%  github.com/miekg/dns.IsFqdn
     0.01s 0.035% 79.97%      0.40s  1.38%  github.com/miekg/dns.correctSource
     0.01s 0.035% 80.00%      2.37s  8.20%  internal/poll.(*FD).Close
     0.01s 0.035% 80.03%      0.90s  3.11%  internal/poll.(*FD).ReadMsg
     0.01s 0.035% 80.07%      0.18s  0.62%  internal/poll.(*FD).SetWriteDeadline
     0.01s 0.035% 80.10%      2.17s  7.51%  internal/poll.(*FD).destroy
     0.01s 0.035% 80.14%      0.55s  1.90%  internal/poll.(*pollDesc).init
     0.01s 0.035% 80.17%      0.16s  0.55%  internal/poll.runtime_Semrelease
     0.01s 0.035% 80.21%      0.54s  1.87%  internal/poll.runtime_pollOpen
     0.01s 0.035% 80.24%      5.24s 18.13%  net.(*Dialer).Dial
     0.01s 0.035% 80.28%      1.43s  4.95%  net.(*conn).Read
     0.01s 0.035% 80.31%      0.24s  0.83%  net.(*conn).SetReadDeadline
     0.01s 0.035% 80.35%      0.20s  0.69%  net.(*conn).SetWriteDeadline
     0.01s 0.035% 80.38%      2.54s  8.79%  net.(*netFD).Close
     0.01s 0.035% 80.42%      0.92s  3.18%  net.(*netFD).readMsg
     0.01s 0.035% 80.45%      3.56s 12.32%  net.socket
     0.01s 0.035% 80.48%      1.17s  4.05%  net.sysSocket
     0.01s 0.035% 80.52%      0.59s  2.04%  runtime.(*mcache).nextFree
     0.01s 0.035% 80.55%      0.31s  1.07%  runtime.(*mcentral).freeSpan
     0.01s 0.035% 80.59%      0.15s  0.52%  runtime.(*mheap).alloc
     0.01s 0.035% 80.62%      0.48s  1.66%  runtime.gcMarkTermination
     0.01s 0.035% 80.66%      0.19s  0.66%  runtime.goexit0
     0.01s 0.035% 80.69%      0.19s  0.66%  runtime.greyobject
     0.01s 0.035% 80.73%      0.16s  0.55%  runtime.isSystemGoroutine
     0.01s 0.035% 80.76%      4.21s 14.57%  runtime.mcall
     0.01s 0.035% 80.80%      0.48s  1.66%  runtime.netpollopen
     0.01s 0.035% 80.83%      0.19s  0.66%  runtime.newproc1
     0.01s 0.035% 80.87%      1.81s  6.26%  runtime.newstack
     0.01s 0.035% 80.90%      0.65s  2.25%  runtime.pcdatavalue
     0.01s 0.035% 80.93%      0.34s  1.18%  runtime.scang
     0.01s 0.035% 80.97%      0.33s  1.14%  runtime.scanstack
     0.01s 0.035% 81.00%      0.16s  0.55%  runtime.semrelease
     0.01s 0.035% 81.04%      0.27s  0.93%  runtime.startm
     0.01s 0.035% 81.07%      0.18s  0.62%  syscall.Getpeername
     0.01s 0.035% 81.11%      1.16s  4.01%  syscall.Socket
     0.01s 0.035% 81.14%      1.02s  3.53%  syscall.read
     0.01s 0.035% 81.18%      0.25s  0.87%  syscall.setsockopt
         0     0% 81.18%      1.71s  5.92%  _/home/jsha/ben.dnsServer
         0     0% 81.18%      0.27s  0.93%  crypto/rand.getRandomBatch
         0     0% 81.18%      1.44s  4.98%  github.com/miekg/dns.(*Conn).Read
         0     0% 81.18%      1.71s  5.92%  github.com/miekg/dns.(*Server).ListenAndServe
         0     0% 81.18%      3.48s 12.04%  github.com/miekg/dns.(*response).Write
         0     0% 81.18%      1.71s  5.92%  github.com/miekg/dns.ListenAndServe
         0     0% 81.18%      1.06s  3.67%  github.com/miekg/dns.ReadFromSessionUDP
         0     0% 81.18%      1.44s  4.98%  github.com/miekg/dns.defaultReader.ReadUDP
         0     0% 81.18%      0.26s   0.9%  github.com/miekg/dns.parseDstFromOOB
         0     0% 81.18%      0.55s  1.90%  internal/poll.(*FD).Init
         0     0% 81.18%      0.23s   0.8%  internal/poll.(*FD).SetReadDeadline
         0     0% 81.18%      0.26s   0.9%  internal/poll.(*FD).writeLock
         0     0% 81.18%      0.78s  2.70%  internal/poll.(*pollDesc).close
         0     0% 81.18%      0.23s   0.8%  internal/poll.runtime_Semacquire
         0     0% 81.18%      0.78s  2.70%  internal/poll.runtime_pollClose
         0     0% 81.18%      0.27s  0.93%  internal/syscall/unix.GetRandom
         0     0% 81.18%      0.37s  1.28%  io.ReadFull
         0     0% 81.18%         1s  3.46%  net.(*UDPConn).ReadMsgUDP
         0     0% 81.18%      3.05s 10.55%  net.(*UDPConn).WriteMsgUDP
         0     0% 81.18%         1s  3.46%  net.(*UDPConn).readMsg
         0     0% 81.18%      2.54s  8.79%  net.(*conn).Close
         0     0% 81.18%      0.23s   0.8%  net.(*netFD).SetReadDeadline
         0     0% 81.18%      0.18s  0.62%  net.(*netFD).SetWriteDeadline
         0     0% 81.18%      0.25s  0.87%  net.setDefaultSockopts
         0     0% 81.18%      0.16s  0.55%  runtime.(*mcentral).grow
         0     0% 81.18%      0.24s  0.83%  runtime.(*mheap).freeSpan
         0     0% 81.18%      0.24s  0.83%  runtime.(*mheap).freeSpan.func1
         0     0% 81.18%      0.36s  1.25%  runtime.forEachP
         0     0% 81.18%      0.41s  1.42%  runtime.futexsleep
         0     0% 81.18%      0.28s  0.97%  runtime.futexwakeup
         0     0% 81.18%      1.38s  4.78%  runtime.gcAssistAlloc
         0     0% 81.18%      0.94s  3.25%  runtime.gcAssistAlloc.func1
         0     0% 81.18%      0.52s  1.80%  runtime.gcBgMarkWorker
         0     0% 81.18%      0.36s  1.25%  runtime.gcBgMarkWorker.func2
         0     0% 81.18%      0.36s  1.25%  runtime.gcDrain
         0     0% 81.18%      0.58s  2.01%  runtime.gcMarkDone
         0     0% 81.18%      0.31s  1.07%  runtime.gcMarkTermination.func4
         0     0% 81.18%      0.29s  1.00%  runtime.gcMarkTermination.func4.1
         0     0% 81.18%      0.41s  1.42%  runtime.markroot
         0     0% 81.18%      0.34s  1.18%  runtime.markroot.func1
         0     0% 81.18%      0.72s  2.49%  runtime.netpollclose
         0     0% 81.18%      0.19s  0.66%  runtime.newproc
         0     0% 81.18%      0.19s  0.66%  runtime.newproc.func1
         0     0% 81.18%      0.20s  0.69%  runtime.scanframeworker
         0     0% 81.18%      0.20s  0.69%  runtime.scanstack.func1
         0     0% 81.18%      0.67s  2.32%  syscall.Connect
         0     0% 81.18%      0.80s  2.77%  syscall.Recvmsg
         0     0% 81.18%      0.25s  0.87%  syscall.SetsockoptInt
         0     0% 81.18%      2.41s  8.34%  syscall.Write
         0     0% 81.18%      0.67s  2.32%  syscall.connect
         0     0% 81.18%      0.67s  2.32%  syscall.recvmsg
         0     0% 81.18%      2.32s  8.03%  syscall.sendmsg
         0     0% 81.18%      1.15s  3.98%  syscall.socket
         0     0% 81.18%      2.41s  8.34%  syscall.write

@tmthrgd
Copy link
Collaborator

tmthrgd commented Nov 15, 2019

This seems reasonable to me. It’s probably how I would write the code. It might be better on (massively) multi-core systems as it avoids holding a mutex lock.

@miekg
Copy link
Owner

miekg commented Nov 15, 2019

I'm also fine with making this change. Thanks @jsha

@jsha
Copy link
Contributor Author

jsha commented Nov 15, 2019

Thanks! I started work on this today and ran into a bit of a hurdle: binary.Read(crand.Reader, binary.BigEndian, &v) can return an error, for instance if /dev/urandom doesn't exist or there is some error reading it.

Right now dns.Id is defined as a func that returns just uint16. We can't change that for backwards compatibility reasons. We could define a new, internal id function that gets called only when dns.Id is overridden, and is able to return errors. Unfortunately, the internal call sites, like SetQuestion, don't return errors themselves, so there's not a good way to bubble up (rare) errors to the caller.

I see two ways forward: (a) panic if binary.Read returns an error. Even though errors should be quite rare, this seems unnecessarily risky, or (b) silently fall back to the math/rand implementation if binary.Read returns error. I think (b) is probably better but it does somewhat complicate analysis of the code, since someone could be running the math/rand generator and not realize it. Gonna think on this some more.

@gibson042
Copy link
Collaborator

Is there anywhere to externalize the source of randomness, such that default behavior (presumably either math/rand or crypto/rand with a fallback) can be overridden by a caller-provided replacement, thereby allowing discovery of errors?

@jsha
Copy link
Contributor Author

jsha commented Nov 16, 2019

The exported Id function variable already provides that capability, so yes, we could do (b) and offer that applications that are willing to panic on randomness failure should use Id to do it. That's a good idea!

@miekg
Copy link
Owner

miekg commented Dec 4, 2019

I would like to just revert to the "random" the stdlib still uses, which uses time.Now() essentially. This also saves a syscall, although I'm not sure if Go does vDSO

@tmthrgd
Copy link
Collaborator

tmthrgd commented Dec 4, 2019

@miekg Go does use vDSO for certain syscalls. On Linux/amd64 this is only for gettimeofday and clock_gettime see vdso_linux_amd64.go.

The runtime uses an xorshift64+ generator for 'fast' random uses, seeded from /dev/urandom amongst other things.

I doubt the overhead of using crypto/rand would be noticeable in basically any use case and it seems far more secure then using math/rand.

@jsha Falling back to math/rand definitely seems like the better choice.

@miekg
Copy link
Owner

miekg commented Dec 4, 2019 via email

@FiloSottile FiloSottile mentioned this issue Dec 5, 2019
Closed
jsha added a commit to jsha/dns that referenced this issue Dec 5, 2019
@jsha
Use crypto/rand for random id generation.
400b5e3 
Fixes miekg#1043 and miekg#1037
@jsha jsha mentioned this issue Dec 5, 2019
Merged
miekg pushed a commit that referenced this issue Dec 11, 2019
@jsha @miekg
Use crypto/rand for random id generation. (#1044)
8ebf2e4 
* Use crypto/rand for random id generation.

Fixes #1043 and #1037

* Panic on rare crypto/rand error.

* Fixes in response to review.
@tmthrgd
Copy link
Collaborator

tmthrgd commented Dec 17, 2019

This should have been closed by #1044.

@tmthrgd tmthrgd closed this as completed Dec 17, 2019
aanm pushed a commit to cilium/dns that referenced this issue Jul 29, 2022
@jsha @aanm
Use crypto/rand for random id generation. (miekg#1044)
b7304d3 
* Use crypto/rand for random id generation.

Fixes miekg#1043 and miekg#1037

* Panic on rare crypto/rand error.

* Fixes in response to review.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@jsha @tmthrgd @miekg @gibson042