From: Ian Jackson Date: Sun, 12 Oct 2014 21:03:43 +0000 (+0100) Subject: Reentrancy: Avoid reentrant callbacks X-Git-Tag: wip.ipv6.2014-10-13.reentrancy-tip X-Git-Url: http://www.chiark.greenend.org.uk/ucgi/~ianmdlvl/git?p=adns.git;a=commitdiff_plain;h=fcadeb25968afc703fb8f6053b67bf9b3374f8b7 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 --- diff --git a/regress/case-manyptrwrong.out b/regress/case-manyptrwrong.out index 6a5309b..983422f 100644 --- 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 c65915d..6621afd 100644 --- 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 973a187..14d8a6b 100644 --- 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 3972ebe..21dca70 100644 --- 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 d53ea5f..92b0d10 100644 --- 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 0808534..a0723a5 100644 --- 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 3419cd6..e853ab7 100644 --- 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; inudp; i++) close(ads->udpsocket[i].fd);