Reentrancy: Avoid reentrant callbacks

Avoid making reentrant callbacks for internal queries during other
processing.

Currently there is a theoretical reentrancy bug in
adns__submit_internal, if the internal query can itself be persuaded
to fail immediately.  The result would be a reentrant call to
callback() for the child query inside functions like typei->parse.

The possibility of such reentrancy is a bug waiting to happen - and
indeed we are going to introduce more complicated query submissions
which are more likely to fail immediately, turning this from a
theoretical to a real bug.

Solve this as follows: when an internal query completes, just put it
on a list.  Whenever we are on our way out of adns, we look through
this list and make the callbacks (until the list is empty).

This means that a fair amount of code needs to be taught that it might
encounter queries in this callback pending state.

We have to update some of the tests' expected output:

Because adns now processes the callback later, a number of the
parallel subqueries made by some of the tests end up finishing before
being cancelled - and therefore the replies to those subqueries don't
show up in the debug output as unrecognised.

Also, in case-norecurse, the deferral of the callback causes the order
of result reporting to be changed: the main query due to subqueries
finishing (`PTR(checked)') ends up later on the results queue than the
other queries dealt with in the same event loop iteration.  (The
actual answer packet to final query, `CNAME(-)', arrives later.)

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

diff --git a/regress/case-manyptrwrong.out b/regress/case-manyptrwrong.out
index 6a5309b615cf4a7155ebc8273778f4407d7157ed..983422fb1531e9515142cd368799d6d3f047c0c9 100644 (file)
--- a/regress/case-manyptrwrong.out
+++ b/regress/case-manyptrwrong.out
@@ -110,32 +110,6 @@ adns debug: TCP connected (NS=195.224.55.129)
  ns.asis.org.nz
  ns.bouquets.co.nz
  agate.co.nz
-adns debug: reply not found, id 313c, query owner security.gen.nz (NS=195.224.55.129)
-adns debug: reply not found, id 313d, query owner ns.tetra.co.nz (NS=195.224.55.129)
-adns debug: reply not found, id 313e, query owner mail.tetra.co.nz (NS=195.224.55.129)
-adns debug: reply not found, id 313f, query owner ns.securicard.co.nz (NS=195.224.55.129)
-adns debug: reply not found, id 3140, query owner ns.underhour.co.nz (NS=195.224.55.129)
-adns debug: reply not found, id 3141, query owner bcc.co.nz (NS=195.224.55.129)
-adns debug: reply not found, id 3142, query owner security.org.nz (NS=195.224.55.129)
-adns debug: reply not found, id 3143, query owner burglaralarms.co.nz (NS=195.224.55.129)
-adns debug: reply not found, id 3144, query owner ns.safes.co.nz (NS=195.224.55.129)
-adns debug: reply not found, id 3145, query owner ns.security.org.nz (NS=195.224.55.129)
-adns debug: reply not found, id 3146, query owner couperconsulting.co.nz (NS=195.224.55.129)
-adns debug: reply not found, id 3147, query owner securityguards.co.nz (NS=195.224.55.129)
-adns debug: reply not found, id 3148, query owner ns.guards.co.nz (NS=195.224.55.129)
-adns debug: reply not found, id 3149, query owner asis.org.nz (NS=195.224.55.129)
-adns debug: reply not found, id 314a, query owner neru.co.nz (NS=195.224.55.129)
-adns debug: reply not found, id 314b, query owner giftbasket.co.nz (NS=195.224.55.129)
-adns debug: reply not found, id 314c, query owner magic.co.nz (NS=195.224.55.129)
-adns debug: reply not found, id 314d, query owner mail.bcc.co.nz (NS=195.224.55.129)
-adns debug: reply not found, id 314e, query owner ns.investigation.co.nz (NS=195.224.55.129)
-adns debug: reply not found, id 314f, query owner nzipi.org.nz (NS=195.224.55.129)
-adns debug: reply not found, id 3150, query owner ns.bouquet.co.nz (NS=195.224.55.129)
-adns debug: reply not found, id 3151, query owner mail.safes.co.nz (NS=195.224.55.129)
-adns debug: reply not found, id 3152, query owner ns.bcc.co.nz (NS=195.224.55.129)
-adns debug: reply not found, id 3153, query owner ns.burglaralarms.co.nz (NS=195.224.55.129)
-adns debug: reply not found, id 3154, query owner ns.securityguards.co.nz (NS=195.224.55.129)
-adns debug: reply not found, id 318c, query owner agate.co.nz (NS=195.224.55.129)
 254.0.99.203.in-addr.arpa flags 0 type PTR(checked): Inconsistent resource records in DNS; nrrs=0; cname=$; owner=$; ttl=80790
 254.0.99.203.in-addr.arpa flags 0 type A(-): No such data; nrrs=0; cname=$; owner=$; ttl=86400
 254.0.99.203.in-addr.arpa flags 0 type NS(raw): No such data; nrrs=0; cname=$; owner=$; ttl=86400

diff --git a/regress/case-manyptrwrongrty.out b/regress/case-manyptrwrongrty.out
index c65915dcab55aafee2f6b0e0e2a01f10efade55c..6621afdabc75b1db05fe8562a54a48bfe782dc4f 100644 (file)
--- a/regress/case-manyptrwrongrty.out
+++ b/regress/case-manyptrwrongrty.out
@@ -122,27 +122,6 @@ adns debug: TCP connected (NS=172.18.45.6)
 254.0.99.203.in-addr.arpa flags 292 type MX(+addr): No such data; nrrs=0; cname=$; owner=254.0.99.203.in-addr.arpa; ttl=540
 254.0.99.203.in-addr.arpa flags 292 type SOA(822): No such data; nrrs=0; cname=$; owner=254.0.99.203.in-addr.arpa; ttl=540
 254.0.99.203.in-addr.arpa flags 292 type RP(822): No such data; nrrs=0; cname=$; owner=254.0.99.203.in-addr.arpa; ttl=539
-adns debug: reply not found, id 313c, query owner security.gen.nz (NS=172.18.45.6)
-adns debug: reply not found, id 313d, query owner ns.tetra.co.nz (NS=172.18.45.6)
-adns debug: reply not found, id 313e, query owner mail.tetra.co.nz (NS=172.18.45.6)
-adns debug: reply not found, id 313f, query owner ns.securicard.co.nz (NS=172.18.45.6)
-adns debug: reply not found, id 3140, query owner ns.underhour.co.nz (NS=172.18.45.6)
-adns debug: reply not found, id 3141, query owner bcc.co.nz (NS=172.18.45.6)
-adns debug: reply not found, id 3142, query owner security.org.nz (NS=172.18.45.6)
-adns debug: reply not found, id 3143, query owner burglaralarms.co.nz (NS=172.18.45.6)
-adns debug: reply not found, id 3144, query owner ns.safes.co.nz (NS=172.18.45.6)
-adns debug: reply not found, id 3145, query owner ns.security.org.nz (NS=172.18.45.6)
-adns debug: reply not found, id 3146, query owner couperconsulting.co.nz (NS=172.18.45.6)
-adns debug: reply not found, id 3147, query owner securityguards.co.nz (NS=172.18.45.6)
-adns debug: reply not found, id 3148, query owner ns.guards.co.nz (NS=172.18.45.6)
-adns debug: reply not found, id 3149, query owner asis.org.nz (NS=172.18.45.6)
-adns debug: reply not found, id 314a, query owner neru.co.nz (NS=172.18.45.6)
-adns debug: reply not found, id 314b, query owner giftbasket.co.nz (NS=172.18.45.6)
-adns debug: reply not found, id 314c, query owner magic.co.nz (NS=172.18.45.6)
-adns debug: reply not found, id 314d, query owner mail.bcc.co.nz (NS=172.18.45.6)
-adns debug: reply not found, id 314e, query owner ns.investigation.co.nz (NS=172.18.45.6)
-adns debug: reply not found, id 314f, query owner nzipi.org.nz (NS=172.18.45.6)
-adns debug: reply not found, id 3150, query owner ns.bouquet.co.nz (NS=172.18.45.6)
 254.0.99.203.in-addr.arpa flags 292 type PTR(checked): Inconsistent resource records in DNS; nrrs=0; cname=$; owner=254.0.99.203.in-addr.arpa; ttl=86351
 adns debug: reply not found, id 3151, query owner mail.safes.co.nz (NS=172.18.45.6)
 adns debug: reply not found, id 3152, query owner ns.bcc.co.nz (NS=172.18.45.6)

diff --git a/regress/case-norecurse.out b/regress/case-norecurse.out
index 973a1878d8bff1b3de5d42bf18f9cb0aed3c2f78..14d8a6b4720b25790f312ca0781c5a7909511947 100644 (file)
--- a/regress/case-norecurse.out
+++ b/regress/case-norecurse.out
@@ -14,10 +14,7 @@ adns debug: using nameserver 172.18.45.6
 4.204.50.158.in-addr.arpa flags 0 type 65548 PTR(checked) submitted
 4.204.50.158.in-addr.arpa flags 0 type 65551 MX(+addr) submitted
 4.204.50.158.in-addr.arpa flags 0 type 131078 SOA(822) submitted
-4.204.50.158.in-addr.arpa flags 0 type 131089adns debug: reply not found, id 3142, query owner ns2.afpdoc.com (NS=172.18.45.6)
-adns debug: reply not found, id 3143, query owner ns2.afp-notes.com (NS=172.18.45.6)
-adns debug: reply not found, id 3144, query owner ns2.afp-domino.com (NS=172.18.45.6)
- RP(822) submitted
+4.204.50.158.in-addr.arpa flags 0 type 131089 RP(822) submitted
 4.204.50.158.in-addr.arpa flags 0 type A(-): No such data; nrrs=0; cname=$; owner=$; ttl=0
 4.204.50.158.in-addr.arpa flags 0 type NS(raw): No such data; nrrs=0; cname=$; owner=$; ttl=0
 4.204.50.158.in-addr.arpa flags 0 type SOA(raw): No such data; nrrs=0; cname=$; owner=$; ttl=0
@@ -53,7 +50,7 @@ adns debug: reply not found, id 3144, query owner ns2.afp-domino.com (NS=172.18.
 4.204.50.158.in-addr.arpa flags 0 type NS(+addr): No such data; nrrs=0; cname=$; owner=$; ttl=0
 4.204.50.158.in-addr.arpa flags 0 type MX(+addr): No such data; nrrs=0; cname=$; owner=$; ttl=0
 4.204.50.158.in-addr.arpa flags 0 type SOA(822): No such data; nrrs=0; cname=$; owner=$; ttl=0
-4.204.50.158.in-addr.arpa flags 0 type PTR(checked): Inconsistent resource records in DNS; nrrs=0; cname=$; owner=$; ttl=77948
 4.204.50.158.in-addr.arpa flags 0 type RP(822): No such data; nrrs=0; cname=$; owner=$; ttl=0
+4.204.50.158.in-addr.arpa flags 0 type PTR(checked): Inconsistent resource records in DNS; nrrs=0; cname=$; owner=$; ttl=77948
 4.204.50.158.in-addr.arpa flags 0 type CNAME(-): No such data; nrrs=0; cname=$; owner=$; ttl=0
 rc=0

diff --git a/src/check.c b/src/check.c
index 3972ebef153c97f4449c8ea0269637596488f776..21dca706300fdd714eb94d3a9c8ec77a60ef8136 100644 (file)
--- a/src/check.c
+++ b/src/check.c
@@ -148,15 +148,29 @@ static void checkc_queue_childw(adns_state ads) {
   });
 }
 
+static void checkc_query_done(adns_state ads, adns_query qu) {
+  assert(qu->state == query_done);
+  assert(!qu->children.head && !qu->children.tail);
+  checkc_query(ads,qu);
+}
+
 static void checkc_queue_output(adns_state ads) {
   adns_query qu;
   
   DLIST_CHECK(ads->output, qu, , {
-    assert(qu->state == query_done);
-    assert(!qu->children.head && !qu->children.tail);
     assert(!qu->parent);
     assert(!qu->allocations.head && !qu->allocations.tail);
-    checkc_query(ads,qu);
+    checkc_query_done(ads,qu);
+  });
+}
+
+static void checkc_queue_intdone(adns_state ads) {
+  adns_query qu;
+  
+  DLIST_CHECK(ads->intdone, qu, , {
+    assert(qu->parent);
+    assert(qu->ctx.callback);
+    checkc_query_done(ads,qu);
   });
 }
 
@@ -168,6 +182,7 @@ void adns__consistency(adns_state ads, adns_query qu, consistency_checks cc) {
     break;
   case cc_entex:
     if (!(ads->iflags & adns_if_checkc_entex)) return;
+    assert(!ads->intdone.head);
     break;
   case cc_freq:
     if ((ads->iflags & adns_if_checkc_freq) != adns_if_checkc_freq) return;
@@ -181,6 +196,7 @@ void adns__consistency(adns_state ads, adns_query qu, consistency_checks cc) {
   checkc_queue_tcpw(ads);
   checkc_queue_childw(ads);
   checkc_queue_output(ads);
+  checkc_queue_intdone(ads);
 
   if (qu) {
     switch (qu->state) {
@@ -194,7 +210,10 @@ void adns__consistency(adns_state ads, adns_query qu, consistency_checks cc) {
       DLIST_ASSERTON(qu, search, ads->childw, );
       break;
     case query_done:
-      DLIST_ASSERTON(qu, search, ads->output, );
+      if (qu->parent)
+       DLIST_ASSERTON(qu, search, ads->intdone, );
+      else
+       DLIST_ASSERTON(qu, search, ads->output, );
       break;
     default:
       assert(!"specific query state");

diff --git a/src/internal.h b/src/internal.h
index d53ea5f3a93c40542d1fc06e15904f9710521eda..92b0d107fd041f94f59cd975cb35b283a1bbdc22 100644 (file)
--- a/src/internal.h
+++ b/src/internal.h
@@ -319,6 +319,10 @@ struct adns__query {
    * Queries in state tcpw/tcpw have been sent (or are in the to-send buffer)
    * iff the tcp connection is in state server_ok.
    *
+   * Internal queries (from adns__submit_internal) end up on intdone
+   * instead of output, and the callbacks are made on the way out of
+   * adns, to avoid reentrancy hazards.
+   *
    *                         +------------------------+
    *             START -----> |      tosend/NONE       |
    *                         +------------------------+
@@ -364,7 +368,7 @@ struct adns__state {
   adns_logcallbackfn *logfn;
   void *logfndata;
   int configerrno;
-  struct query_queue udpw, tcpw, childw, output;
+  struct query_queue udpw, tcpw, childw, output, intdone;
   adns_query forallnext;
   int nextid, tcpsocket;
   struct udpsocket { int af; int fd; } udpsocket[MAXUDP];
@@ -711,7 +715,11 @@ void adns__cancel_children(adns_query qu);
 
 void adns__returning(adns_state ads, adns_query qu);
 /* Must be called before returning from adns any time that we have
- * progressed (including made, finished or destroyed) queries. */
+ * progressed (including made, finished or destroyed) queries.
+ *
+ * Might reenter adns via internal query callbacks, so
+ * external-faciing functions which call adns__returning should
+ * normally be avoided in internal code. */
 
 /* From reply.c: */
 

diff --git a/src/query.c b/src/query.c
index 08085342283a8faa5537316270b156ff99ac482b..a0723a5c41ef152d06c42ba879c1be3a097a3a37 100644 (file)
--- a/src/query.c
+++ b/src/query.c
@@ -497,6 +497,17 @@ static void free_query_allocs(adns_query qu) {
 }
 
 void adns__returning(adns_state ads, adns_query qu_for_caller) {
+  while (ads->intdone.head) {
+    adns_query iq= ads->intdone.head;
+    adns_query parent= iq->parent;
+    LIST_UNLINK_PART(parent->children,iq,siblings.);
+    LIST_UNLINK(iq->ads->childw,parent);
+    LIST_UNLINK(ads->intdone,iq);
+    iq->ctx.callback(parent,iq);
+    free_query_allocs(iq);
+    free(iq->answer);
+    free(iq);
+  }
   adns__consistency(ads,qu_for_caller,cc_entex);
 }
 
@@ -517,7 +528,10 @@ void adns__cancel(adns_query qu) {
     LIST_UNLINK(ads->childw,qu);
     break;
   case query_done:
-    LIST_UNLINK(ads->output,qu);
+    if (qu->parent)
+      LIST_UNLINK(ads->intdone,qu);
+    else
+      LIST_UNLINK(ads->output,qu);
     break;
   default:
     abort();
@@ -585,8 +599,8 @@ static void makefinal_query(adns_query qu) {
 }
 
 void adns__query_done(adns_query qu) {
+  adns_state ads=qu->ads;
   adns_answer *ans;
-  adns_query parent;
 
   adns__cancel_children(qu);
 
@@ -617,18 +631,12 @@ void adns__query_done(adns_query qu) {
   }
 
   ans->expires= qu->expires;
-  parent= qu->parent;
-  if (parent) {
-    LIST_UNLINK_PART(parent->children,qu,siblings.);
-    LIST_UNLINK(qu->ads->childw,parent);
-    qu->ctx.callback(parent,qu);
-    free_query_allocs(qu);
-    free(qu->answer);
-    free(qu);
+  qu->state= query_done;
+  if (qu->parent) {
+    LIST_LINK_TAIL(ads->intdone,qu);
   } else {
     makefinal_query(qu);
     LIST_LINK_TAIL(qu->ads->output,qu);
-    qu->state= query_done;
   }
 }
 

diff --git a/src/setup.c b/src/setup.c
index 3419cd6a5134f2157d710b541825ffa436d2ea36..e853ab797e36139b015a3f1bb628fd6a38fa3377 100644 (file)
--- a/src/setup.c
+++ b/src/setup.c
@@ -558,6 +558,7 @@ static int init_begin(adns_state *ads_r, adns_initflags flags,
   LIST_INIT(ads->tcpw);
   LIST_INIT(ads->childw);
   LIST_INIT(ads->output);
+  LIST_INIT(ads->intdone);
   ads->forallnext= 0;
   ads->nextid= 0x311f;
   ads->nudp= 0;
@@ -729,6 +730,7 @@ void adns_finish(adns_state ads) {
     else if (ads->tcpw.head) adns__cancel(ads->tcpw.head);
     else if (ads->childw.head) adns__cancel(ads->childw.head);
     else if (ads->output.head) adns__cancel(ads->output.head);
+    else if (ads->intdone.head) adns__cancel(ads->output.head);
     else break;
   }
   for (i=0; i<ads->nudp; i++) close(ads->udpsocket[i].fd);