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.

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

panic in goroutine, rmsg is nil #126

Open
bradleyjkemp opened this issue Apr 18, 2024 · 2 comments
Open

panic in goroutine, rmsg is nil #126

bradleyjkemp opened this issue Apr 18, 2024 · 2 comments

Comments

@bradleyjkemp
Copy link

I'm getting a nil panic from this line:

if rmsg.Rcode == dns.RcodeNameError {

Because this occurs in a goroutine spawned by this library, it's not recoverable

Full trace

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x1066173]

goroutine 22668 [running]:
github.com/domainr/dnsr.(*Resolver).exchangeIP(0xc0011de090, {0x481cb28, 0xc003b10c30}, {0xc0055ffa28, 0x13}, {0xc01925a140, 0xe}, {0xc0055ff668, 0x12}, {0x1f3bc0a, ...}, ...)
   /root/.cache/go-build/github.com/domainr/[email protected]/resolver.go:374 +0x6b3
github.com/domainr/dnsr.(*Resolver).exchange(0xc0011de090, {0x481cb28, 0xc003b10c30}, {0xc0055ffa28, 0x13}, {0xc0055ff668, 0x12}, {0x1f3bc0a, 0x3}, 0x1)
   /root/.cache/go-build/github.com/domainr/[email protected]/resolver.go:294 +0x185
github.com/domainr/dnsr.(*Resolver).iterateParents.func1({0xc0055ffa28?, 0xc001f8aa20?})
   /root/.cache/go-build/github.com/domainr/[email protected]/resolver.go:235 +0x89
created by github.com/domainr/dnsr.(*Resolver).iterateParents in goroutine 19692
   /root/.cache/go-build/github.com/domainr/[email protected]/resolver.go:234 +0x9ae

I've not managed to reliably reproduce it but I'm reasonably sure the bug comes from these lines

dnsr/resolver.go

Lines 354 to 359 in aa0c407

conn, err := dialer.DialContext(ctx, "tcp", addr)
if err == nil {
dconn := &dns.Conn{Conn: conn}
rmsg, dur, err = client.ExchangeWithConnContext(ctx, &qmsg, dconn)
conn.Close()
}

The err return from client.ExchangeWithConnContext is never checked because it's been shadowed by line 354

This means that resolvers with dnsr.WithTCPRetry() can panic if:

  1. UDP lookup is truncated
  2. TCP connection succeeds but lookup fails

In this case, the error check is ineffective at guarding against nil rmsg values because the error from the TCP lookup has been lost to the shadowed err var

dnsr/resolver.go

Lines 369 to 371 in aa0c407

if err != nil {
return nil, err
}

@cee-dub
Copy link
Member

cee-dub commented Apr 18, 2024

Thanks for the report and digging in to investigate!

@cee-dub
Copy link
Member

cee-dub commented Apr 18, 2024

It does appear there are possible gaps in error handling in this area:

dnsr/resolver.go

Lines 337 to 369 in aa0c407

conn, err := dialer.DialContext(ctx, "udp", addr)
var rmsg *dns.Msg
var dur time.Duration
if err == nil {
dconn := &dns.Conn{Conn: conn}
rmsg, dur, err = client.ExchangeWithConnContext(ctx, &qmsg, dconn)
conn.Close()
}
if r.tcpRetry && rmsg != nil && rmsg.MsgHdr.Truncated {
// Since we are doing another query, we need to recheck the deadline
if dl, ok := ctx.Deadline(); ok {
if start.After(dl.Add(-TypicalResponseTime)) { // bail if we can't finish in time (start is too close to deadline)
return nil, ErrTimeout
}
client.Timeout = dl.Sub(start)
}
// Retry with TCP
conn, err := dialer.DialContext(ctx, "tcp", addr)
if err == nil {
dconn := &dns.Conn{Conn: conn}
rmsg, dur, err = client.ExchangeWithConnContext(ctx, &qmsg, dconn)
conn.Close()
}
}
select {
case <-ctx.Done(): // Finished too late
logCancellation(host, &qmsg, rmsg, depth, dur, client.Timeout)
return nil, ctx.Err()
default:
logExchange(host, &qmsg, rmsg, depth, dur, client.Timeout, err) // Log hostname instead of IP
}
if err != nil {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants