-
-
Notifications
You must be signed in to change notification settings - Fork 5
Initial fuzzing support, and failing fuzz test case. #24
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
base: main
Are you sure you want to change the base?
Conversation
tertsdiepraam
left a comment
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.
Looks good, but I suggest that we keep this unmerged until we can make this depend on the main branch of domain.
It now depends on the |
|
Revived! Maybe a controversial change is to make the stelline feature no longer optional, but that seemed like the easiest way to do this right now since |
@tertsdiepraam: I'm afraid I don't understand what is left to do here, the PR seems to work for me in its current form. |
| "tsig", | ||
| "unstable-client-transport", | ||
| "unstable-sign", | ||
| "unstable-stelline", |
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.
@ximon18 this is the "controversial" change. If you agree with this then its fine with me to merge it.
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.
I see.
The fuzz binary uses FakeEnv which causes this problem.
Could another way be to impl Env for FuzzEnv where FuzzEnv doesn't use Stelline (and so wouldn't be suitable for running dnst update or dnst notify unless some non-Stelline based impl of Env::dgram() and Env::stub_resolver_from_confg() would be possible)?
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.
I wonder if we should do ldns-testns. If so then we will get stelline as well.
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.
Are you referring to #56?
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.
Yes. We may need to rethink the name and design for the dnst version. But it seems a useful feature.
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.
Interesting idea... :-)
See
fuzz/README.md.Uses NLnetLabs/domain#441.