chiark / gitweb /
Reentrancy: Avoid reentrant callbacks
authorIan Jackson <ijackson@chiark.greenend.org.uk>
Sun, 12 Oct 2014 21:03:43 +0000 (22:03 +0100)
committerIan Jackson <ijackson@chiark.greenend.org.uk>
Sun, 19 Oct 2014 20:09:56 +0000 (21:09 +0100)
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>
regress/case-manyptrwrong.out
regress/case-manyptrwrongrty.out
regress/case-norecurse.out
src/check.c
src/internal.h
src/query.c
src/setup.c

index 6a5309b..983422f 100644 (file)
@@ -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
index c65915d..6621afd 100644 (file)
@@ -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)
index 973a187..14d8a6b 100644 (file)
@@ -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
index 3972ebe..21dca70 100644 (file)
@@ -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");
index d53ea5f..92b0d10 100644 (file)
@@ -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: */
 
index 0808534..a0723a5 100644 (file)
@@ -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;
   }
 }
 
index 3419cd6..e853ab7 100644 (file)
@@ -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);