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

Fix NPE in SimpleResolver #277

Merged
merged 1 commit into from
Feb 5, 2023
Merged

Fix NPE in SimpleResolver #277

merged 1 commit into from
Feb 5, 2023

Conversation

PhroZenOne
Copy link
Contributor

When reading a response that is REFUSED with no more data. The code could crash while comparing the DNS query with the response.

This change skips the comparison if the response is denied.

@codecov-commenter
Copy link

Codecov Report

Base: 64.18% // Head: 64.17% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (f348300) compared to base (442fff4).
Patch coverage: 100.00% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #277      +/-   ##
============================================
- Coverage     64.18%   64.17%   -0.02%     
- Complexity     2766     2770       +4     
============================================
  Files           178      178              
  Lines         12536    12540       +4     
  Branches       1911     1912       +1     
============================================
+ Hits           8046     8047       +1     
- Misses         4001     4003       +2     
- Partials        489      490       +1     
Impacted Files Coverage Δ
src/main/java/org/xbill/DNS/SimpleResolver.java 34.05% <100.00%> (+2.56%) ⬆️
src/main/java/org/xbill/DNS/NioTcpClient.java 65.38% <0.00%> (-2.57%) ⬇️
src/main/java/org/xbill/DNS/DohResolver.java 50.51% <0.00%> (-0.69%) ⬇️
src/main/java/org/xbill/DNS/Message.java 78.35% <0.00%> (+0.37%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ibauersachs
Copy link
Member

Hi, thanks for your contribution!

I'm very hesitant to accept this as is, as I'm currently considering the answer you received from the server is invalid. Specifically, the DNS specification requires the validation on all of id/name/class/type (see RFC 5452, 9.1). However, you're right that the missing name shouldn't cause a NPE: the response from SimpleResolver (and for that matter, the other implementations too) should be failed otherwise.

Can you please share some details about the setup where you got this, especially the name and version of the server?

When reading a response that is REFUSED with no more data. The code could crash while comparing the DNS query with the response.

This change skips the comparison if the response is denied.
@PhroZenOne
Copy link
Contributor Author

The DNS server is something I'm accessing as a third party so I have no info about the server itself. I have notified them and hope that they fix stuff on their side.

However, I changed the fix to throw an WireParseException instead. Only issue I have with this solution is that the DENIED flag might be a good indicator on what is wrong and that is hidden right now.

Maybe returning the message with the exception is a solution?

@PhroZenOne
Copy link
Contributor Author

@ibauersachs have you had time to think about this? :)

@ibauersachs
Copy link
Member

No, not yet. I'll might get to it on Sunday, but I can't make any promises.

@ibauersachs ibauersachs merged commit bd5177d into dnsjava:master Feb 5, 2023
@ibauersachs
Copy link
Member

It's fine like this, thank you!

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

Successfully merging this pull request may close these issues.

None yet

3 participants