[Dnsmasq-discuss] [dnsmasq][dns query]dns query failed if the first server replis REFUSE

2017-04-24 Thread Mi Bear
Hello Everyone,

I found an issue about DNS query. In my test scenario, there are two DNS
servers, and the first one will always return REFUSE, and the second one
can work properly. And the strict order option is on.

In this case, I expect the a domain name can be resolved correctly by the
second DNS server.

But I saw a DNS query packet was sent to the first server, and received a
REFUSE from it, and I got REFUSED as the the final result at the LAN side
PC. I did not see the DNS query packet sent to the second DNS server.


I checked the source code, I think the following part of code is hard to be
understood.

-
I copied it here from dnsmasq-2.76

Line 788,function reply_query, in forward.c:

  /* Note: if we send extra options in the EDNS0 header, we can't recreate
 the query from the reply. */
  if (RCODE(header) == REFUSED &&
  *!*option_bool(OPT_ORDER) &&
  forward->forwardall == 0 &&
  !(forward->flags & FREC_HAS_EXTRADATA))
/* for broken servers, attempt to send to another one. */
{

The meaning of this part code is, for broken servers, attempt to send to
another one, if:
1. strict order is *NOT* set
2. REFUSED got from a server
3. forwardall is 0
4. some conditions else

according to my understanding, if the option strict order is *set*, I think
dnsmasq will forward the DNS query packet to DNS servers one by one in the
list. If the first refused the query, dnsmasq should forward the query to
the second one.

But in this part of code, if the option strict order is *NOT* set and got
refused, (also with some other conditions), dnsmasq would try to send to
another one. It's different from my understanding.


Also in the source code of function forward_query, I can see, if option
strict order is *NOT *set, forwardall would be set as* 1*.

So the condition 1(strict order is* not *set) and 3(forwardall is* 0*) in
function reply_query would never be matched together, and no dns query
would be sent to the second DNS server in my test case, just as what I saw.


I think the "!" in the condition 1 in function reply_query should be
removed as below. It's more reasonable. I tested the modified source code,
and it worked fine in my test case.


  /* Note: if we send extra options in the EDNS0 header, we can't recreate
 the query from the reply. */
  if (RCODE(header) == REFUSED &&
  option_bool(OPT_ORDER) &&
  forward->forwardall == 0 &&
  !(forward->flags & FREC_HAS_EXTRADATA))
/* for broken servers, attempt to send to another one. */
{


I beg your help or comments on this issue.


-- 
Best Regards and Many Thanks
Bear Mi
___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss


Re: [Dnsmasq-discuss] [dnsmasq][dns query]dns query failed if the first server replis REFUSE

2017-05-16 Thread Mi Bear
Hi, Baptiste and all,

Thank you for your reply, and sorry for my late reply. :b

I made a test build based on 2.76, with this diff in the link in your mail,
but it did not fixed my issue.

Let me give more detail information of my case:
1. option strictorder (OPT_ORDER in source code) is on, that dnsmasq should
send query to upstream DNS servers one by one.

2. there are two upstream DNS servers, and the first one would always
return REFUSED, and the second one can work well


-
But with following diff, it can work well, and got domain name resolved
correctly.


diff --git a/forward.c b/forward.c


index 726b5df..7fcf960 100644
--- a/forward.c
+++ b/forward.c
@@ -788,7 +788,7 @@ void reply_query(int fd, int family, time_t now)
   /* Note: if we send extra options in the EDNS0 header, we can't recreate
  the query from the reply. */
   if (RCODE(header) == REFUSED &&
-  !option_bool(OPT_ORDER) &&
+  option_bool(OPT_ORDER) &&
   forward->forwardall == 0 &&
   !(forward->flags & FREC_HAS_EXTRADATA))
 /* for broken servers, attempt to send to another one. */


I think it's a logical error, that from this in-line comment "*/* for
broken servers, attempt to send to another one. */*", it's obviously a
strict order scenario (to send to upstream server one by one).

But the condition selector is the strict order option (OPT_ORDER) should
NOT be set.

So I think it's a logical error. For more source code analyzing, please see
my previous mail.


Please help me to check this issue.


Thanks a lot
Mi Feng


2017-05-08 1:23 GMT+08:00 Baptiste Jonglez <bapti...@bitsofnetworks.org>:

> Hi,
>
> On Tue, Apr 25, 2017 at 12:13:40PM +0800, Mi Bear wrote:
> > Hello Everyone,
> >
> > I found an issue about DNS query. In my test scenario, there are two DNS
> > servers, and the first one will always return REFUSE, and the second one
> > can work properly. And the strict order option is on.
> >
> > In this case, I expect the a domain name can be resolved correctly by the
> > second DNS server.
> >
> > But I saw a DNS query packet was sent to the first server, and received a
> > REFUSE from it, and I got REFUSED as the the final result at the LAN side
> > PC. I did not see the DNS query packet sent to the second DNS server.
>
> You're right, it's a bug, introduced in 2.76.  It has been fixed in
> v2.77test2, but unfortunately the final version of 2.77 has apparently not
> been released yet.
>
> More details here: http://thekelleys.org.uk/gitweb/?p=dnsmasq.git;a=
> commit;h=68f6312d4bae30b78daafcd6f51dc441b8685b1e
>
> >
> > I checked the source code, I think the following part of code is hard to
> be
> > understood.
> >
> > -
> > I copied it here from dnsmasq-2.76
> >
> > Line 788,function reply_query, in forward.c:
> >
> >   /* Note: if we send extra options in the EDNS0 header, we can't
> recreate
> >  the query from the reply. */
> >   if (RCODE(header) == REFUSED &&
> >   *!*option_bool(OPT_ORDER) &&
> >   forward->forwardall == 0 &&
> >   !(forward->flags & FREC_HAS_EXTRADATA))
> > /* for broken servers, attempt to send to another one. */
> > {
> >
> > The meaning of this part code is, for broken servers, attempt to send to
> > another one, if:
> > 1. strict order is *NOT* set
> > 2. REFUSED got from a server
> > 3. forwardall is 0
> > 4. some conditions else
> >
> > according to my understanding, if the option strict order is *set*, I
> think
> > dnsmasq will forward the DNS query packet to DNS servers one by one in
> the
> > list. If the first refused the query, dnsmasq should forward the query to
> > the second one.
> >
> > But in this part of code, if the option strict order is *NOT* set and got
> > refused, (also with some other conditions), dnsmasq would try to send to
> > another one. It's different from my understanding.
> >
> > 
> > Also in the source code of function forward_query, I can see, if option
> > strict order is *NOT *set, forwardall would be set as* 1*.
> >
> > So the condition 1(strict order is* not *set) and 3(forwardall is* 0*) in
> > function reply_query would never be matched together, and no dns query
> > would be sent to the second DNS server in my test case, just as what I
> saw.
> >
> >
> > I think the "!" in the condition 1 in function reply_query should be
> > removed as below. It's more reasonable. I tested the modified source
> code,
> > and it worked fine in my test case.
>