nil ptr in defaults.go:152 msg.IsEdns0

[ignoramous]

A crash report from our app points to a nil ptr whilst iterating over Extra RRs:

dns/defaults.go

Lines 147 to 157 in b77d1ed

	func (dns *Msg) IsEdns0() *OPT {
	// RFC 6891, Section 6.1.1 allows the OPT record to appear
	// anywhere in the additional record section, but it's usually at
	// the end so start there.
	for i := len(dns.Extra) - 1; i >= 0; i-- {
	if dns.Extra[i].Header().Rrtype == TypeOPT {
	return dns.Extra[i].(*OPT)
	}
	}
	return nil
	}

11-03 05:50:57.349 32129 20730 E LibLogger: panic({0x763892d0c0?, 0x7638f537d0?})
11-03 05:50:57.349 32129 20730 E LibLogger:     /home/jitpack/golang/go/src/runtime/panic.go:785 +0x124
11-03 05:50:57.349 32129 20730 E LibLogger: github.com/miekg/dns.(*Msg).IsEdns0(0x40008b9710)
11-03 05:50:57.349 32129 20730 E LibLogger:     /tmp/gomobile-work-1841608193/pkg/mod/github.com/miekg/dns@v1.1.62/defaults.go:152 +0x60
11-03 05:50:57.349 32129 20730 E LibLogger: github.com/celzero/firestack/intra/xdns.ensureEDNS0(0x40008b9710)
11-03 05:50:57.349 32129 20730 E LibLogger:     /home/jitpack/build/intra/xdns/dnsutil.go:480 +0x20
11-03 05:50:57.349 32129 20730 E LibLogger: github.com/celzero/firestack/intra/xdns.AddEDNS0PaddingIfNoneFound(0x40008b9710)
11-03 05:50:57.349 32129 20730 E LibLogger:     /home/jitpack/build/intra/xdns/dnsutil.go:517 +0x24
11-03 05:50:57.349 32129 20730 E LibLogger: github.com/celzero/firestack/intra/doh.padQuery(0x40008b9710)
11-03 05:50:57.349 32129 20730 E LibLogger:     /home/jitpack/build/intra/doh/padding.go:35 +0x40
11-03 05:50:57.349 32129 20730 E LibLogger: github.com/celzero/firestack/intra/doh.(*transport).doDoh(0x40003dc160, {0x4000f84bdc, 0x4}, 0x40008b9710)
11-03 05:50:57.349 32129 20730 E LibLogger:     /home/jitpack/build/intra/doh/doh.go:372 +0x44
11-03 05:50:57.349 32129 20730 E LibLogger: github.com/celzero/firestack/intra/doh.(*transport).Query(0x40003dc160, {0x4000f84bd8, 0x8}, 0x40008b9710, 0x4000ec2600)
11-03 05:50:57.349 32129 20730 E LibLogger:     /home/jitpack/build/intra/doh/doh.go:679 +0xa8
11-03 05:50:57.349 32129 20730 E LibLogger: github.com/celzero/firestack/intra/dnsx.Req({0x7638a462f0, 0x40003dc160}, {0x4000f84bd8, 0x8}, 0x40008b9710, 0x4000ec2600)
11-03 05:50:57.349 32129 20730 E LibLogger:     /home/jitpack/build/intra/dnsx/alg.go:1129 +0x12c

Unsure if it is incorrect use of the lib by our code, or something that's indicative of other issues (as I see miekg/dns codebase accessing struct fields ex: Extra[x].Header() all too often).

(from: celzero/firestack#111)

[ignoramous]

If it helps, managed to grab a str(query) from a recent crash log:

;; opcode: QUERY, status: NOERROR, id: 0
;; flags: rd; QUERY: 1, ANSWER: 0, AUTHORITY: 0, ADDITIONAL: 1

;; OPT PSEUDOSECTION:
; EDNS: version 0; flags:; udp: 4096
; PADDING: 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000

;; QUESTION SECTION:
;github.com.	IN	 HTTPS

[miekg]

thanks for that. This indeed needs a fix. Maybe I have the cycles this weekend

[miekg]

(I obviously didn't have time this weekend, and also not soon)

so there is an OPT rr (PADDING), and then some if dns.Extra[i].Header().Rrtype == TypeOPT {

would nil....odd. This also happens if you (manually) in a Msg and call the above code? (PADDING is relative new, might be something fishy there)

[ignoramous]

Unsure how the DNS msg is crafted. The app is handling the DNS queries forwarded to it by dnsmasq (on Android). If I were to hazard a guess, these queries were sent by Chrome.

would nil....odd

Yeah, no matter what checks we added (in our code), we couldn't prevent the nil ptr .

[miekg]

odd

Do you happen to see which things nilled in dns.Extra[i].Header().Rrtype, i.e. the Extra[i] or does Header() return a nil?

[miekg]

and... tcpdump or some such?!

[ignoramous]

and... tcpdump or some such?!

Unfortunately, no. The logs are sent by app users as bug reports from Android devices in the field, if that makes sense. We've hit this nil ptr may be twice or thrice in our testing too but it is so so rare.

Do you happen to see which things nil'd in dns.Extra[i].Header().Rrtype, i.e. the Extra[i] or does Header() return a nil?

We added nil checks on dns, on dns.Extra[i], a "deep" nil check on dns.Extra[i].Header() ... but nothing stopped the crashes as the nil ptr would manifest elsewhere (the error logs after desperate fixes we tried are linked to from here).¹ I didn't open bugs for those, as the nils were emanating from our code & the padding stuff we were doing with OPT Extras. After many unsuccessful attempts, out of exasperation, we switched to Msg.IsEdns0() as the first thing our code did (AddEDNS0Padding -> ensureEDNS0 -> Msg.IsEdns0), and sure enough, hit a nil ptr, except the bug now manifested squarely in the lib and not our code.

Footnotes

1. Commits (in no particular order): a / b / c / d / e / f ↩

[miekg]

Hmm, I see, but without something to trigger this it will be very hard to figure it out. Can you get your test to dump something (even hex of a packet might help). There can be all kinds of garbage in a dns packet, so it would be nice to get a handle on this. [ Quoting ***@***.***> in "Re: [miekg/dns] nil ptr in defaults..." ]

…

and... tcpdump or some such?! Unfortunately, no. The logs are sent by app users as bug reports from Android devices in the field, if that makes sense. We've hit this nil ptr may be twice or thrice in our testing too but it is so so rare. Do you happen to see which things nil'd in dns.Extra[i].Header().Rrtype, i.e. the Extra[i] or does Header() return a nil? We tried adding a nil check on dns, on dns.Extra[i], a "deep" nil check on dns.Extra[i].Header() ... but nothing stopped a crash as the nil ptr would manifest elsewhere (you can follow the desperate fixes we tried here: https:// github.com/celzero/firestack/issues/96).[^0] I didn't open bugs for those, as the nils manifest from our code. After many unsuccessful attempts, out of exasperation, we switched to Msg.IsEdns0() as the first thing our code did, and sure enough, hit a nil ptr, except the bug now manifested squarely in the lib and not our code. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.*Message ID: <miekg/dns/issues/ ***@***.***>