discovery: fix panic in DNS fallback SRV lookup#10914
Conversation
The fallback SRV lookup type-asserted each DNS Answer record to *dns.SRV unconditionally. If the response contains a non-SRV record (e.g. an A or CNAME), the type assertion panics and crashes the daemon. Use the comma-ok form to skip non-SRV records instead. Also guard against an empty LookupHost result for the shim, which would otherwise panic on an out-of-bounds index into addrs. This is safe to discuss and fix in public. The bug is very unlikely to be exploitable: triggering it requires either a DNS seeder to serve a malformed response, or an on-path MITM injecting one (the fallback response is unauthenticated). A malicious seeder already has far more direct ways to disrupt a node, and a MITM attack is hard to mount, so the panic does not meaningfully widen the attack surface.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses potential stability issues in the DNS fallback SRV lookup mechanism. By replacing unconditional type assertions with safe checks and adding validation for empty lookup results, the changes prevent the daemon from panicking when receiving malformed or unexpected DNS responses. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
erickcestari
force-pushed
the
fix-panic-discovery
branch
from
a29264c to
98f5645
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes potential panics in the DNS fallback SRV lookup by handling empty address results and safely skipping non-SRV records using a comma-ok type assertion. It also adds corresponding unit tests and updates the release notes. The reviewer recommended improving the unit tests by verifying that the type assertions on the bootstrapper interface succeed using require.True instead of ignoring the boolean result, which could lead to nil pointer dereferences.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| [][2]string{{target, "soa.lightning.directory"}}, | ||
| netStub, time.Second, | ||
| ) | ||
| d, _ := bs.(*DNSSeedBootstrapper) |
There was a problem hiding this comment.
Using the comma-ok type assertion but ignoring the boolean result (_) can lead to a nil pointer dereference panic on the next line if the assertion fails. It is safer and more idiomatic to assert that the type assertion succeeded using require.True.
| d, _ := bs.(*DNSSeedBootstrapper) | |
| d, ok := bs.(*DNSSeedBootstrapper) | |
| require.True(t, ok) |
References
- Unit tests must always use the require library. (link)
| netStub := &fallbackNet{shimAddrs: nil} | ||
|
|
||
| bs := NewDNSSeedBootstrapper(nil, netStub, time.Second) | ||
| d, _ := bs.(*DNSSeedBootstrapper) |
There was a problem hiding this comment.
Using the comma-ok type assertion but ignoring the boolean result (_) can lead to a nil pointer dereference panic on the next line if the assertion fails. It is safer and more idiomatic to assert that the type assertion succeeded using require.True.
| d, _ := bs.(*DNSSeedBootstrapper) | |
| d, ok := bs.(*DNSSeedBootstrapper) | |
| require.True(t, ok) |
References
- Unit tests must always use the require library. (link)
github-actions
Bot
added
the
severity-high
PR Severity: HIGH (severity-high)
High (1 file):
Low (1 file):
AnalysisThe primary change is in The test file ( No severity bump was triggered: only 2 non-test files changed (threshold: >20) and 21 non-test lines changed (threshold: >500). To override, add a |
1 participant
The fallback SRV lookup type-asserted each DNS Answer record to
*dns.SRVunconditionally. If the response contains a non-SRV record (e.g. an A or
CNAME), the type assertion panics and crashes the daemon. Use the
comma-ok form to skip non-SRV records instead.
Also guard against an empty
LookupHostresult for the shim, which wouldotherwise panic on an out-of-bounds index into addrs.
This is safe to discuss and fix in public. The bug is very unlikely to be
exploitable: triggering it requires either a DNS seeder to serve a
malformed response, or an on-path MITM injecting one (the fallback
response is unauthenticated). A malicious seeder already has far more
direct ways to disrupt a node, and a MITM attack is hard to mount, so the
panic does not meaningfully widen the attack surface.