From 051a2489e6b1081c0a20a0b12e56e25eff464ccd Mon Sep 17 00:00:00 2001 From: Daniel Kahn Gillmor Date: Sat, 29 Oct 2016 01:25:05 -0400 Subject: [PATCH] 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 Gbp-Pq: Topic dirmngr-idling Gbp-Pq: Name 0001-dirmngr-hkp-Avoid-potential-race-condition-when-some.patch --- dirmngr/ks-engine-hkp.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/dirmngr/ks-engine-hkp.c b/dirmngr/ks-engine-hkp.c index a6c22f8..2d1240b 100644 --- a/dirmngr/ks-engine-hkp.c +++ b/dirmngr/ks-engine-hkp.c @@ -209,25 +209,24 @@ host_in_pool_p (int *pool, int tblidx) static int select_random_host (int *table) { - int *tbl; - size_t tblsize; + int *tbl = NULL; + size_t tblsize = 0; int pidx, idx; /* We create a new table so that we randomly select only from currently alive hosts. */ - for (idx=0, tblsize=0; (pidx = table[idx]) != -1; idx++) + for (idx=0; (pidx = table[idx]) != -1; idx++) if (hosttable[pidx] && !hosttable[pidx]->dead) - tblsize++; + { + 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; (pidx = table[idx]) != -1; idx++) - if (hosttable[pidx] && !hosttable[pidx]->dead) - tbl[tblsize++] = pidx; - if (tblsize == 1) /* Save a get_uint_nonce. */ pidx = tbl[0]; else -- 2.30.2