Re: [PATCH] dnsproxy: Prefer results with domain name while domain_append is true
Hi Pasi, On to, 2014-07-03 at 15:14 +0300, pasi.sjoh...@jolla.com wrote: > From: Pasi Sjöholm > > If domain_append is set and forward_dns_reply() processes the response > for query without the domain name earlier than the response for one > with the domain name set we need to make sure that the response is > not sent back to the client if rcode and ancount are zero until the > last nameserver response is processed. This use case is not handled properly: - there are multiple DNS servers defined, some of them from ISP, user has also set his own DNS server that responds to some internal data - user queries a host without domain name that is only found in his own network - all the ISP name servers return "not found" - user's own dns server does not respond for some reason or the dns reply is lost What now happens is that the request timeouts and the user is given an error instead of more proper "not found" result. Cheers, Jukka > --- > src/dnsproxy.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/dnsproxy.c b/src/dnsproxy.c > index 7232b98..28e7cf7 100644 > --- a/src/dnsproxy.c > +++ b/src/dnsproxy.c > @@ -2068,7 +2068,8 @@ static int forward_dns_reply(unsigned char *reply, int > reply_len, int protocol, > } > > out: > - if (hdr->rcode > 0 && req->numresp < req->numserv) > + if ((hdr->rcode > 0 || (hdr->rcode == 0 && hdr->ancount == 0 && > + req->append_domain)) && req->numresp < req->numserv) > return -EINVAL; > > request_list = g_slist_remove(request_list, req); ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
RE: [PATCH] dnsproxy: Prefer results with domain name while domain_append is true
Hi Jukka, >> If domain_append is set and forward_dns_reply() processes the response >> for query without the domain name earlier than the response for one >> with the domain name set we need to make sure that the response is >> not sent back to the client if rcode and ancount are zero until the >> last nameserver response is processed. >This use case is not handled properly: > >- there are multiple DNS servers defined, some of them from ISP, user >has also set his own DNS server that responds to some internal data >- user queries a host without domain name that is only found in his own >network >- all the ISP name servers return "not found" >- user's own dns server does not respond for some reason or the dns >reply is lost > >What now happens is that the request timeouts and the user is given an >error instead of more proper "not found" result. My initial analysis on this bug was bit wrong. This patch does not change the current way of connman dnsproxy timeouting if the user's own nameserver reply gets lost or does not respond and other servers will send NXDomain-reply. However my patch resolves an issue when dnsproxy query is a valid domain tld. eg. com, bz, net, org or any other valid tld. Example: User wants to resolve A/-record for bz.domain.tld therefore connman queries first for bz and then for bz.domain.tld and it should prefer bz.domain.tld answer over the bz if domain.tld is set as a search path. However currently on 99% certainty the reply for bz will come earlier than bz.domain.tld and as it is rcode=0 and most probably with ancount=0 (tld's don't usually have A or -record) it will be sent to the client which is not what user probably wanted. My patch makes sure that on those cases the reply will not be sent to the client as it does not provide any valid records. Br, Pasi ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] dnsproxy: Prefer results with domain name while domain_append is true
Hi Pasi, On to, 2014-07-03 at 15:14 +0300, pasi.sjoh...@jolla.com wrote: > From: Pasi Sjöholm > > If domain_append is set and forward_dns_reply() processes the response > for query without the domain name earlier than the response for one > with the domain name set we need to make sure that the response is > not sent back to the client if rcode and ancount are zero until the > last nameserver response is processed. > --- > src/dnsproxy.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/dnsproxy.c b/src/dnsproxy.c > index 7232b98..28e7cf7 100644 > --- a/src/dnsproxy.c > +++ b/src/dnsproxy.c > @@ -2068,7 +2068,8 @@ static int forward_dns_reply(unsigned char *reply, int > reply_len, int protocol, > } > > out: > - if (hdr->rcode > 0 && req->numresp < req->numserv) > + if ((hdr->rcode > 0 || (hdr->rcode == 0 && hdr->ancount == 0 && > + req->append_domain)) && req->numresp < req->numserv) > return -EINVAL; > > request_list = g_slist_remove(request_list, req); Looks good, couple of comments - I sent a patch that changes how rcode values are compared, that patch does not touch this function thou. So if you could replace rcode 0 value with ns_r_noerror - Try to set the indentation of the second line so that it does not align with return statement, then it is easier to read the code, even better if the complex "if" statement could be written more clearly, you could probably even refactor it to two pieces - Add clarification to commit message about the "rcode == ns_r_noerror && ancount == 0" case, it is not very clear from looking the code when that situation happens. You had this explained partly in this mail thread already - Currently the commit message is just one big sentence, my head starts to hurt when reading it. Perhaps you could refactor it more. Cheers, Jukka ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
RE: [PATCH] dnsproxy: Prefer results with domain name while domain_append is true
>> --- a/src/dnsproxy.c >> +++ b/src/dnsproxy.c >> @@ -2068,7 +2068,8 @@ static int forward_dns_reply(unsigned char *reply, int >> reply_len, int protocol, >> } >> >> out: >> - if (hdr->rcode > 0 && req->numresp < req->numserv) >> + if ((hdr->rcode > 0 || (hdr->rcode == 0 && hdr->ancount == 0 && >> + req->append_domain)) && req->numresp < req->numserv) >> return -EINVAL; >> >> request_list = g_slist_remove(request_list, req); >Looks good, couple of comments I did the modifications requested, both the commit message and the patch itself should look more clear now. Revised patch has subject "[PATCH] dnsproxy: Prefer responses with ancount>0 if append_domain is true". Br, Pasi ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
