Bugfix/237 Resolver uses nameserver commented out in /etc/resolv.conf #238
Conversation
This /etc/resolv.conf:
# x
# nameserver 8.8.8.8
Still configured 8.8.8.8 as nameserver, because the comment detection in `ldns_resolver_new_frm_fp_l()` didn't anticipate empty lines before the comment.
This fix removed all comment handling from `ldns_resolver_new_frm_fp_l()`. Instead a new function is introduced `ldns_fget_token_l_resolv_conf()` that skips comments that start with '#' and ';'. The old `ldns_fget_token_l()` (that is used for zonefiles too) still accepts only ';' for comments.
|
Patch looks reasonable. I tested it for my limited use case and it works as expected. |
|
Are you planning to merge this any time soon? |
Yes, I will merge it with Wouter's recommendation to add the comment |
|
I don't think you fully appreciate the seriousness of this issue... |
resolver.c
Outdated
| @@ -761,6 +761,8 @@ ldns_resolver_new_frm_fp(ldns_resolver **res, FILE *fp) | |||
| return ldns_resolver_new_frm_fp_l(res, fp, NULL); | |||
| } | |||
|
|
|||
| ssize_t ldns_fget_token_l_resolv_conf(FILE *f, char *token, const char *delim, | |||
| size_t limit, int *line_nr); | |||
There was a problem hiding this comment.
Please move this prototype to <ldns/parse.h>.
To manage expections around this free, open-source project, I'd like to note that unlike projects with production-grade support such as NSD and Unbound, work on |
You may want to consider your audience before you start handing out life lessons. |
|
@AlexanderBand Just to make it clear: Your work on ldns is much appreciated.
Maybe it would make sense to clarify the development vision, as the way it is currently written (quoting in full, emphasis is mine)
makes it sound (to me) like you would actually do bug fixes outside of research projects and IETF Hackathons. Using commented out parts of the configuration can have dire consequences, hence the sense of urgency. |
|
Yes, we do bug fixes on |
Including a doxygen comment stating the limitations
|
@wtoorop & @AlexanderBand Thank you! |
Fixed issue #237