note: combination of previous 0210+0220 -- From: Daniel Kahn Gillmor Date: Sat, 29 Oct 2016 01:25:05 -0400 Subject: dirmngr: hkp: Avoid potential race condition when some hosts die. * dirmngr/ks-engine-hkp.c (select_random_host): Use atomic pass through the host table instead of risking out-of-bounds write. -- Multiple threads may write to hosttable[x]->dead while select_random_host() is running. For example, a housekeeping thread might clear the ->dead bit on some entries, or another connection to dirmngr might manually mark a host as alive. If one or more hosts are resurrected between the two loops over a given table in select_random_host(), then the allocation of tbl might not be large enough, resulting in a write past the end of tbl on the second loop. This change collapses the two loops into a single loop to avoid this discrepancy: each host's "dead" bit is now only checked once. As Werner points out, this isn't currently strictly necessary, since npth will not switch threads unless a blocking system call is made, and no blocking system call is made in these two loops. However, in a subsequent change in this series, we will call a function in this loop, and that function may sometimes write(2), or call other functions, which may themselves block. Keeping this as a single-pass loop avoids the need to keep track of what might block and what might not. Signed-off-by: Daniel Kahn Gillmor --- dirmngr/ks-engine-hkp.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) Patch-Source: https://sources.debian.org/data/main/g/gnupg2/2.2.27-2/debian/patches/dirmngr-idling/dirmngr-hkp-Avoid-potential-race-condition-when-some.patch From: Daniel Kahn Gillmor Date: Sat, 29 Oct 2016 02:00:50 -0400 Subject: dirmngr: Avoid need for hkp housekeeping. * dirmngr/ks-engine-hkp.c (host_is_alive): New function. Test whether host is alive and resurrects it if it has been dead long enough. (select_random_host, map_host, ks_hkp_mark_host): Use host_is_alive instead of testing hostinfo_t->dead directly. (ks_hkp_housekeeping): Remove function, no longer needed. * dirmngr/dirmngr.c (housekeeping_thread): Remove call to ks_hkp_housekeeping. -- Rather than resurrecting hosts upon scheduled resurrection times, test whether hosts should be resurrected as they're inspected for being dead. This removes the need for explicit housekeeping, and makes host resurrections happen "just in time", rather than being clustered on HOUSEKEEPING_INTERVAL seconds. According to 392e068e9f143d41f6350345619543cbcd47380f, dns_stuff_housekeeping only works on Windows, so it also isn't necessary in debian, but it remains in place for now. Signed-off-by: Daniel Kahn Gillmor --- dirmngr/dirmngr.c | 3 --- dirmngr/dirmngr.h | 1 - dirmngr/ks-engine-hkp.c | 72 ++++++++++++++++++++++++------------------------- 3 files changed, 35 insertions(+), 41 deletions(-) Patch-Source: https://sources.debian.org/data/main/g/gnupg2/2.2.27-2/debian/patches/dirmngr-idling/dirmngr-Avoid-need-for-hkp-housekeeping.patch diff --git a/dirmngr/dirmngr.c b/dirmngr/dirmngr.c index 977e7dd..5017c3f 100644 --- a/dirmngr/dirmngr.c +++ b/dirmngr/dirmngr.c @@ -2085,7 +2085,6 @@ housekeeping_thread (void *arg) dirmngr_init_default_ctrl (&ctrlbuf); dns_stuff_housekeeping (); - ks_hkp_housekeeping (curtime); if (network_activity_seen) { network_activity_seen = 0; diff --git a/dirmngr/ks-engine-hkp.c b/dirmngr/ks-engine-hkp.c index 5292da8..690b37d 100644 --- a/dirmngr/ks-engine-hkp.c +++ b/dirmngr/ks-engine-hkp.c @@ -221,6 +221,24 @@ host_in_pool_p (hostinfo_t hi, int tblidx) return 0; } +static int +host_is_alive (hostinfo_t hi, time_t curtime) +{ + if (!hi) + return 0; + if (!hi->dead) + return 1; + if (!hi->died_at) + return 0; /* manually marked dead */ + if (hi->died_at + RESURRECT_INTERVAL <= curtime + || hi->died_at > curtime) + { + hi->dead = 0; + log_info ("resurrected host '%s'", hi->name); + return 1; + } + return 0; +} /* Select a random host. Consult HI->pool which indices into the global hosttable. Returns index into HI->pool or -1 if no host could be @@ -228,32 +246,32 @@ host_in_pool_p (hostinfo_t hi, int tblidx) static int select_random_host (hostinfo_t hi) { - int *tbl; - size_t tblsize; + int *tbl = NULL; + size_t tblsize = 0; int pidx, idx; + time_t curtime; + + curtime = gnupg_get_time (); /* CHECKTHIS(); See */ /* https://sources.debian.org/patches/gnupg2/2.2.20-1/dirmngr-idling/dirmngr-hkp-Avoid-potential-race-condition-when-some.patch/ */ /* We create a new table so that we randomly select only from currently alive hosts. */ - for (idx = 0, tblsize = 0; + for (idx = 0; idx < hi->pool_len && (pidx = hi->pool[idx]) != -1; idx++) - if (hosttable[pidx] && !hosttable[pidx]->dead) - tblsize++; + if (hosttable[pidx] && host_is_alive (hosttable[pidx], curtime)) + { + tblsize++; + tbl = xtryrealloc(tbl, tblsize * sizeof *tbl); + if (!tbl) + return -1; /* memory allocation failed! */ + tbl[tblsize-1] = pidx; + } if (!tblsize) return -1; /* No hosts. */ - tbl = xtrymalloc (tblsize * sizeof *tbl); - if (!tbl) - return -1; - for (idx = 0, tblsize = 0; - idx < hi->pool_len && (pidx = hi->pool[idx]) != -1; - idx++) - if (hosttable[pidx] && !hosttable[pidx]->dead) - tbl[tblsize++] = pidx; - if (tblsize == 1) /* Save a get_uint_nonce. */ pidx = tbl[0]; else @@ -471,6 +489,7 @@ map_host (ctrl_t ctrl, const char *name, const char *srvtag, int force_reselect, int is_pool; int new_hosts = 0; char *cname; + time_t curtime; *r_host = NULL; if (r_httpflags) @@ -510,6 +529,7 @@ map_host (ctrl_t ctrl, const char *name, const char *srvtag, int force_reselect, } else hi = hosttable[idx]; + curtime = gnupg_get_time (); is_pool = hi->pool != NULL; @@ -616,7 +636,7 @@ map_host (ctrl_t ctrl, const char *name, const char *srvtag, int force_reselect, if (force_reselect) hi->poolidx = -1; else if (hi->poolidx >= 0 && hi->poolidx < hosttable_size - && hosttable[hi->poolidx] && hosttable[hi->poolidx]->dead) + && hosttable[hi->poolidx] && !host_is_alive (hosttable[hi->poolidx], curtime)) hi->poolidx = -1; /* Select a host if needed. */ @@ -675,7 +695,7 @@ map_host (ctrl_t ctrl, const char *name, const char *srvtag, int force_reselect, return gpg_error_from_syserror (); } - if (hi->dead) + if (!host_is_alive (hi, curtime)) { log_error ("host '%s' marked as dead\n", hi->name); if (r_httphost) @@ -781,7 +801,8 @@ ks_hkp_mark_host (ctrl_t ctrl, const char *name, int alive) { gpg_error_t err = 0; hostinfo_t hi, hi2; - int idx, idx2, idx3, n; + int idx, idx2, idx3, n, is_alive; + time_t curtime; if (!name || !*name || !strcmp (name, "localhost")) return 0; @@ -796,13 +817,15 @@ ks_hkp_mark_host (ctrl_t ctrl, const char *name, int alive) goto leave; } + curtime = gnupg_get_time (); hi = hosttable[idx]; - if (alive && hi->dead) + is_alive = host_is_alive (hi, curtime); + if (alive && !is_alive) { hi->dead = 0; err = ks_printf_help (ctrl, "marking '%s' as alive", name); } - else if (!alive && !hi->dead) + else if (!alive && is_alive) { hi->dead = 1; hi->died_at = 0; /* Manually set dead. */ @@ -836,14 +859,15 @@ ks_hkp_mark_host (ctrl_t ctrl, const char *name, int alive) hi2 = hosttable[n]; if (!hi2) - ; - else if (alive && hi2->dead) + continue; + is_alive = host_is_alive (hi2, curtime); + if (alive && !is_alive) { hi2->dead = 0; err = ks_printf_help (ctrl, "marking '%s' as alive", hi2->name); } - else if (!alive && !hi2->dead) + else if (!alive && is_alive) { hi2->dead = 1; hi2->died_at = 0; /* Manually set dead. */ @@ -1149,40 +1173,6 @@ ks_hkp_resolve (ctrl_t ctrl, parsed_uri_t uri) } -/* Housekeeping function called from the housekeeping thread. It is - used to mark dead hosts alive so that they may be tried again after - some time. */ -void -ks_hkp_housekeeping (time_t curtime) -{ - int idx; - hostinfo_t hi; - - if (npth_mutex_lock (&hosttable_lock)) - log_fatal ("failed to acquire mutex\n"); - - for (idx=0; idx < hosttable_size; idx++) - { - hi = hosttable[idx]; - if (!hi) - continue; - if (!hi->dead) - continue; - if (!hi->died_at) - continue; /* Do not resurrect manually shot hosts. */ - if (hi->died_at + RESURRECT_INTERVAL <= curtime - || hi->died_at > curtime) - { - hi->dead = 0; - log_info ("resurrected host '%s'", hi->name); - } - } - - if (npth_mutex_unlock (&hosttable_lock)) - log_fatal ("failed to release mutex\n"); -} - - /* Reload (SIGHUP) action for this module. We mark all host alive * even those which have been manually shot. */ void