-
Notifications
You must be signed in to change notification settings - Fork 12
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
Ignore incomplete RRs #136
Conversation
4aa059a
to
2e32534
Compare
t/rr.t
Outdated
# Alg 8 will replace 5. Now (December 2017) both are used. | ||
ok( $rr->algorithm == 5 or $rr->algorithm == 8 ); | ||
# Alg 8 has replaced 5. Now (February 2022) only alg 8 is used. | ||
ok( $rr->algorithm == 8 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really related to the PR description? I think this should be in a separate PR with a relevant description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's under the Changes section:
And I've included @pnax's fix for the broken tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preferably it should be in a separate PR, and "@pnax's fix for the broken tests" is not much of documentation.
t/rr.t
Outdated
ok( $rr->keytag == 59747 or $rr->keytag == 59407 ); | ||
# .SE KSK should not change very often. 59407 has replaced 59747. | ||
# Now (February 2022) only 59407 is used. | ||
ok( $rr->keytag == 59407 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above. And more changes below.
How is this PR related to #135? They both introduce the new defintion of Zonemaster::LDNS::Packet::answer method. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, a welcome change here as well
Will this PR resolve #110? |
0ca82ce
Hi, I was conducting some additional tests on this pull requests because I was wondering whether it would solve the crashes I witnessed when fuzz testing Zonemaster. One of my runs ended with the following fatal error:
I found the solution to this small issue, and I think it should be added to this pull request in order to avoid a regression when a server returns SIG records. |
0ca82ce
to
0541cca
Compare
I've rebased this onto current develop. Please re-review and I'll merge it. |
SIG resource records were missing methods defined in LDNS.xs because of a missing use statement in Zonemaster::LDNS::RR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand this PR, incomplete RRs will be silently ignored from processing. Since Zonemaster is testing tool I think it would make more sence if the information is signalled to Engine, that can output a message that this has happened, e.g. (ERROR, "MX record with invalid format has been ignored from response of X query for DOMAIN to server SERVER/IP"
If this PR prevents crashes I think it is OK if it is seen as a preliminary work-around, but that should then be documented in the code and an issue being created.
Correct. Broken RRs are silently dropped when using the API the way Engine does today. This prevents crashes. While Zonemaster::LDNS does not actively signal that a broken RR has been found, Engine could use the current Zonemaster::LDNS API without too much trouble to find out if any RRs have been dropped and the RRtypes of the dropped RRs. Doing this might put a little extra pressure on the garbage collector so it would be interesting to have a look at the performance impact. If the impact is significant we could provide a more efficient API in Zonemaster::LDNS. Edit: For context, the dropping mechanism was introduced in #135. |
Purpose
This PR makes sure that we're not assuming all RRs come with all fields included. Out-of-the-box ldns returns such RRs even though they're incomplete. If we're unlucky we could be accessing such non-existing fields and that could lead to assertion errors or unexpected
undef
values.Context
I found this when investigating #110.
Changes
And I've included @pnax's fix from #133 for the broken tests.
How to test this PR
Just run
make test
. New unit test are added to verify that incomplete RRs are filtered out. The old tests verify that complete RRs are not filtered out.