sergio/takana

1note: combination of previous 0210+0220
2--
3From: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
4Date: Sat, 29 Oct 2016 01:25:05 -0400
5Subject: dirmngr: hkp: Avoid potential race condition when some hosts die.
6
7* dirmngr/ks-engine-hkp.c (select_random_host): Use atomic pass
8through the host table instead of risking out-of-bounds write.
9
10--
11
12Multiple threads may write to hosttable[x]->dead while
13select_random_host() is running.  For example, a housekeeping thread
14might clear the ->dead bit on some entries, or another connection to
15dirmngr might manually mark a host as alive.
16
17If one or more hosts are resurrected between the two loops over a
18given table in select_random_host(), then the allocation of tbl might
19not be large enough, resulting in a write past the end of tbl on the
20second loop.
21
22This change collapses the two loops into a single loop to avoid this
23discrepancy: each host's "dead" bit is now only checked once.
24
25As Werner points out, this isn't currently strictly necessary, since
26npth will not switch threads unless a blocking system call is made,
27and no blocking system call is made in these two loops.
28
29However, in a subsequent change in this series, we will call a
30function in this loop, and that function may sometimes write(2), or
31call other functions, which may themselves block.  Keeping this as a
32single-pass loop avoids the need to keep track of what might block and
33what might not.
34
35Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
36---
37 dirmngr/ks-engine-hkp.c | 23 ++++++++++-------------
38 1 file changed, 10 insertions(+), 13 deletions(-)
39
40Patch-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
41
42From: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
43Date: Sat, 29 Oct 2016 02:00:50 -0400
44Subject: dirmngr: Avoid need for hkp housekeeping.
45
46* dirmngr/ks-engine-hkp.c (host_is_alive): New function.  Test whether
47host is alive and resurrects it if it has been dead long enough.
48(select_random_host, map_host, ks_hkp_mark_host): Use host_is_alive
49instead of testing hostinfo_t->dead directly.
50(ks_hkp_housekeeping): Remove function, no longer needed.
51* dirmngr/dirmngr.c (housekeeping_thread): Remove call to
52ks_hkp_housekeeping.
53
54--
55
56Rather than resurrecting hosts upon scheduled resurrection times, test
57whether hosts should be resurrected as they're inspected for being
58dead.  This removes the need for explicit housekeeping, and makes host
59resurrections happen "just in time", rather than being clustered on
60HOUSEKEEPING_INTERVAL seconds.
61
62According to 392e068e9f143d41f6350345619543cbcd47380f,
63dns_stuff_housekeeping only works on Windows, so it also isn't
64necessary in debian, but it remains in place for now.
65
66Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
67---
68 dirmngr/dirmngr.c       |  3 ---
69 dirmngr/dirmngr.h       |  1 -
70 dirmngr/ks-engine-hkp.c | 72 ++++++++++++++++++++++++-------------------------
71 3 files changed, 35 insertions(+), 41 deletions(-)
72
73Patch-Source: https://sources.debian.org/data/main/g/gnupg2/2.2.27-2/debian/patches/dirmngr-idling/dirmngr-Avoid-need-for-hkp-housekeeping.patch
74
75diff --git a/dirmngr/dirmngr.c b/dirmngr/dirmngr.c
76index 977e7dd..5017c3f 100644
77--- a/dirmngr/dirmngr.c
78+++ b/dirmngr/dirmngr.c
79@@ -2085,7 +2085,6 @@ housekeeping_thread (void *arg)
80   dirmngr_init_default_ctrl (&ctrlbuf);
81 
82   dns_stuff_housekeeping ();
83-  ks_hkp_housekeeping (curtime);
84   if (network_activity_seen)
85     {
86       network_activity_seen = 0;
87diff --git a/dirmngr/ks-engine-hkp.c b/dirmngr/ks-engine-hkp.c
88index 5292da8..690b37d 100644
89--- a/dirmngr/ks-engine-hkp.c
90+++ b/dirmngr/ks-engine-hkp.c
91@@ -221,6 +221,24 @@ host_in_pool_p (hostinfo_t hi, int tblidx)
92   return 0;
93 }
94 
95+static int
96+host_is_alive (hostinfo_t hi, time_t curtime)
97+{
98+  if (!hi)
99+    return 0;
100+  if (!hi->dead)
101+    return 1;
102+  if (!hi->died_at)
103+    return 0; /* manually marked dead */
104+  if (hi->died_at + RESURRECT_INTERVAL <= curtime
105+      || hi->died_at > curtime)
106+    {
107+      hi->dead = 0;
108+      log_info ("resurrected host '%s'", hi->name);
109+      return 1;
110+    }
111+  return 0;
112+}
113 
114 /* Select a random host.  Consult HI->pool which indices into the global
115    hosttable.  Returns index into HI->pool or -1 if no host could be
116@@ -228,32 +246,32 @@ host_in_pool_p (hostinfo_t hi, int tblidx)
117 static int
118 select_random_host (hostinfo_t hi)
119 {
120-  int *tbl;
121-  size_t tblsize;
122+  int *tbl = NULL;
123+  size_t tblsize = 0;
124   int pidx, idx;
125+  time_t curtime;
126+
127+  curtime = gnupg_get_time ();
128 
129   /* CHECKTHIS();  See */
130   /*                 https://sources.debian.org/patches/gnupg2/2.2.20-1/dirmngr-idling/dirmngr-hkp-Avoid-potential-race-condition-when-some.patch/ */
131 
132   /* We create a new table so that we randomly select only from
133      currently alive hosts.  */
134-  for (idx = 0, tblsize = 0;
135+  for (idx = 0;
136        idx < hi->pool_len && (pidx = hi->pool[idx]) != -1;
137        idx++)
138-    if (hosttable[pidx] && !hosttable[pidx]->dead)
139-      tblsize++;
140+    if (hosttable[pidx] && host_is_alive (hosttable[pidx], curtime))
141+      {
142+        tblsize++;
143+        tbl = xtryrealloc(tbl, tblsize * sizeof *tbl);
144+        if (!tbl)
145+          return -1; /* memory allocation failed! */
146+        tbl[tblsize-1] = pidx;
147+      }
148   if (!tblsize)
149     return -1; /* No hosts.  */
150 
151-  tbl = xtrymalloc (tblsize * sizeof *tbl);
152-  if (!tbl)
153-    return -1;
154-  for (idx = 0, tblsize = 0;
155-       idx < hi->pool_len && (pidx = hi->pool[idx]) != -1;
156-       idx++)
157-    if (hosttable[pidx] && !hosttable[pidx]->dead)
158-      tbl[tblsize++] = pidx;
159-
160   if (tblsize == 1)  /* Save a get_uint_nonce.  */
161     pidx = tbl[0];
162   else
163@@ -471,6 +489,7 @@ map_host (ctrl_t ctrl, const char *name, const char *srvtag, int force_reselect,
164   int is_pool;
165   int new_hosts = 0;
166   char *cname;
167+  time_t curtime;
168 
169   *r_host = NULL;
170   if (r_httpflags)
171@@ -510,6 +529,7 @@ map_host (ctrl_t ctrl, const char *name, const char *srvtag, int force_reselect,
172     }
173   else
174     hi = hosttable[idx];
175+  curtime = gnupg_get_time ();
176 
177   is_pool = hi->pool != NULL;
178 
179@@ -616,7 +636,7 @@ map_host (ctrl_t ctrl, const char *name, const char *srvtag, int force_reselect,
180       if (force_reselect)
181         hi->poolidx = -1;
182       else if (hi->poolidx >= 0 && hi->poolidx < hosttable_size
183-               && hosttable[hi->poolidx] && hosttable[hi->poolidx]->dead)
184+               && hosttable[hi->poolidx] && !host_is_alive (hosttable[hi->poolidx], curtime))
185         hi->poolidx = -1;
186 
187       /* Select a host if needed.  */
188@@ -675,7 +695,7 @@ map_host (ctrl_t ctrl, const char *name, const char *srvtag, int force_reselect,
189         return gpg_error_from_syserror ();
190     }
191 
192-  if (hi->dead)
193+  if (!host_is_alive (hi, curtime))
194     {
195       log_error ("host '%s' marked as dead\n", hi->name);
196       if (r_httphost)
197@@ -781,7 +801,8 @@ ks_hkp_mark_host (ctrl_t ctrl, const char *name, int alive)
198 {
199   gpg_error_t err = 0;
200   hostinfo_t hi, hi2;
201-  int idx, idx2, idx3, n;
202+  int idx, idx2, idx3, n, is_alive;
203+  time_t curtime;
204 
205   if (!name || !*name || !strcmp (name, "localhost"))
206     return 0;
207@@ -796,13 +817,15 @@ ks_hkp_mark_host (ctrl_t ctrl, const char *name, int alive)
208       goto leave;
209     }
210 
211+  curtime = gnupg_get_time ();
212   hi = hosttable[idx];
213-  if (alive && hi->dead)
214+  is_alive = host_is_alive (hi, curtime);
215+  if (alive && !is_alive)
216     {
217       hi->dead = 0;
218       err = ks_printf_help (ctrl, "marking '%s' as alive", name);
219     }
220-  else if (!alive && !hi->dead)
221+  else if (!alive && is_alive)
222     {
223       hi->dead = 1;
224       hi->died_at = 0; /* Manually set dead.  */
225@@ -836,14 +859,15 @@ ks_hkp_mark_host (ctrl_t ctrl, const char *name, int alive)
226 
227           hi2 = hosttable[n];
228           if (!hi2)
229-            ;
230-          else if (alive && hi2->dead)
231+            continue;
232+          is_alive = host_is_alive (hi2, curtime);
233+          if (alive && !is_alive)
234             {
235               hi2->dead = 0;
236               err = ks_printf_help (ctrl, "marking '%s' as alive",
237                                     hi2->name);
238             }
239-          else if (!alive && !hi2->dead)
240+          else if (!alive && is_alive)
241             {
242               hi2->dead = 1;
243               hi2->died_at = 0; /* Manually set dead. */
244@@ -1149,40 +1173,6 @@ ks_hkp_resolve (ctrl_t ctrl, parsed_uri_t uri)
245 }
246 
247 
248-/* Housekeeping function called from the housekeeping thread.  It is
249-   used to mark dead hosts alive so that they may be tried again after
250-   some time.  */
251-void
252-ks_hkp_housekeeping (time_t curtime)
253-{
254-  int idx;
255-  hostinfo_t hi;
256-
257-  if (npth_mutex_lock (&hosttable_lock))
258-    log_fatal ("failed to acquire mutex\n");
259-
260-  for (idx=0; idx < hosttable_size; idx++)
261-    {
262-      hi = hosttable[idx];
263-      if (!hi)
264-        continue;
265-      if (!hi->dead)
266-        continue;
267-      if (!hi->died_at)
268-        continue; /* Do not resurrect manually shot hosts.  */
269-      if (hi->died_at + RESURRECT_INTERVAL <= curtime
270-          || hi->died_at > curtime)
271-        {
272-          hi->dead = 0;
273-          log_info ("resurrected host '%s'", hi->name);
274-        }
275-    }
276-
277-  if (npth_mutex_unlock (&hosttable_lock))
278-    log_fatal ("failed to release mutex\n");
279-}
280-
281-
282 /* Reload (SIGHUP) action for this module.  We mark all host alive
283  * even those which have been manually shot.  */
284 void