16 files changed, 69 insertions, 100 deletions

diff --git a/Makefile.am b/Makefile.am
index 8f6b38aa0f..3e0ae3a227 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -140,8 +140,7 @@ check-spaces:
 		$(top_srcdir)/src/common/*.[ch] \
 		$(top_srcdir)/src/or/*.[ch] \
 		$(top_srcdir)/src/test/*.[ch] \
-		$(top_srcdir)/src/tools/*.[ch] \
-		$(top_srcdir)/src/tools/tor-fw-helper/*.[ch]
+		$(top_srcdir)/src/tools/*.[ch]
 
 check-docs: all
 	$(PERL) $(top_builddir)/scripts/maint/checkOptionDocs.pl
diff --git a/changes/workqueue_reply_t b/changes/workqueue_reply_t
new file mode 100644
index 0000000000..c2d3f4ad65
--- /dev/null
+++ b/changes/workqueue_reply_t
@@ -0,0 +1,6 @@
+  o Minor bugfixes:
+    - Ensure that worker threads actually exit when a fatal error or
+      shutdown is indicated. This doesn't currently affect the behaviour
+      of Tor, because Tor never indicates fatal error or shutdown except
+      in its unit tests. Fixes bug 16868; bugfix on 0.2.6.3-alpha.
+
diff --git a/doc/include.am b/doc/include.am
index af99501502..47f1ab3433 100644
--- a/doc/include.am
+++ b/doc/include.am
@@ -13,7 +13,7 @@
 # just use the .1 and .html files.
 
 base_mans = doc/tor doc/tor-gencert doc/tor-resolve doc/torify
-all_mans = $(base_mans) doc/tor-fw-helper
+all_mans = $(base_mans)
 if USE_FW_HELPER
 install_mans = $(all_mans)
 else
@@ -57,13 +57,11 @@ doc/tor.1.in: doc/tor.1.txt
 doc/torify.1.in: doc/torify.1.txt
 doc/tor-gencert.1.in: doc/tor-gencert.1.txt
 doc/tor-resolve.1.in: doc/tor-resolve.1.txt
-doc/tor-fw-helper.1.in: doc/tor-fw-helper.1.txt
 
 doc/tor.html.in: doc/tor.1.txt
 doc/torify.html.in: doc/torify.1.txt
 doc/tor-gencert.html.in: doc/tor-gencert.1.txt
 doc/tor-resolve.html.in: doc/tor-resolve.1.txt
-doc/tor-fw-helper.html.in: doc/tor-fw-helper.1.txt
 
 # use config.status to swap all machine-specific magic strings
 # in the asciidoc with their replacements.
@@ -78,13 +76,11 @@ doc/tor.html: doc/tor.html.in
 doc/tor-gencert.html: doc/tor-gencert.html.in
 doc/tor-resolve.html: doc/tor-resolve.html.in
 doc/torify.html: doc/torify.html.in
-doc/tor-fw-helper.html: doc/tor-fw-helper.html.in
 
 doc/tor.1: doc/tor.1.in
 doc/tor-gencert.1: doc/tor-gencert.1.in
 doc/tor-resolve.1: doc/tor-resolve.1.in
 doc/torify.1: doc/torify.1.in
-doc/tor-fw-helper.1: doc/tor-fw-helper.1.in
 
 CLEANFILES+= $(asciidoc_product) config.log
 DISTCLEANFILES+= $(html_in) $(man_in)
diff --git a/doc/tor-fw-helper.1.txt b/doc/tor-fw-helper.1.txt
deleted file mode 100644
index 1c103d9250..0000000000
--- a/doc/tor-fw-helper.1.txt
+++ /dev/null
@@ -1,60 +0,0 @@
-// Copyright (c) The Tor Project, Inc.
-// See LICENSE for licensing information
-// This is an asciidoc file used to generate the manpage/html reference.
-// Learn asciidoc on http://www.methods.co.nz/asciidoc/userguide.html
-:man source:   Tor
-:man manual:   Tor Manual
-tor-fw-helper(1)
-================
-Jacob Appelbaum
-
-NAME
-----
-tor-fw-helper - Manage upstream firewall/NAT devices
-
-SYNOPSIS
---------
-**tor-fw-helper** [-h|--help] [-T|--test-commandline] [-v|--verbose] [-g|--fetch-public-ip]
- [-p __external port__:__internal_port__]
-
-DESCRIPTION
------------
-**tor-fw-helper** currently supports Apple's NAT-PMP protocol and the UPnP
-standard for TCP port mapping. It is written as the reference implementation of
-tor-fw-helper-spec.txt and conforms to that loose plugin API.  If your network
-supports either NAT-PMP or UPnP, tor-fw-helper will attempt to automatically
-map the required TCP ports for Tor's Or and Dir ports. +
-
-OPTIONS
--------
-**-h** or **--help**::
-    Display help text and exit.
-
-**-v** or **--verbose**::
-    Display verbose output.
-
-**-T** or **--test-commandline**::
-    Display test information and print the test information in
-    tor-fw-helper.log
-
-**-g** or **--fetch-public-ip**::
-    Fetch the the public ip address for each supported NAT helper method.
-
-**-p** or **--port** __external_port__:__internal_port__::
-    Forward external_port to internal_port.  This option can appear
-    more than once.
-
-BUGS
-----
-This probably doesn't run on Windows. That's not a big issue, since we don't
-really want to deal with Windows before October 2010 anyway.
-
-SEE ALSO
---------
-**tor**(1) +
-
-See also the "tor-fw-helper-spec.txt" file, distributed with Tor.
-
-AUTHORS
--------
-    Jacob Appelbaum <jacob@torproject.org>, Steven J. Murdoch <Steven.Murdoch@cl.cam.ac.uk>
diff --git a/scripts/maint/checkSpace.pl b/scripts/maint/checkSpace.pl
index c785d89567..906281112d 100755
--- a/scripts/maint/checkSpace.pl
+++ b/scripts/maint/checkSpace.pl
@@ -129,7 +129,8 @@ for $fn (@ARGV) {
                     $1 ne "switch" and $1 ne "return" and $1 ne "int" and
                     $1 ne "elsif" and $1 ne "WINAPI" and $2 ne "WINAPI" and
                     $1 ne "void" and $1 ne "__attribute__" and $1 ne "op" and
-                    $1 ne "size_t" and $1 ne "double") {
+                    $1 ne "size_t" and $1 ne "double" and
+                    $1 ne "workqueue_reply_t") {
                     print "     fn ():$fn:$.\n";
                 }
             }
diff --git a/scripts/maint/format_changelog.py b/scripts/maint/format_changelog.py
index a557fcaf40..5e4c8cac9a 100755
--- a/scripts/maint/format_changelog.py
+++ b/scripts/maint/format_changelog.py
@@ -36,7 +36,6 @@ NO_HYPHENATE=set("""
 pf-divert
 tor-resolve
 tor-gencert
-tor-fw-helper
 """.split())
 
 LASTLINE_UNDERFLOW_EXPONENT = 1
diff --git a/src/common/compat.c b/src/common/compat.c
index 76f9bcb97e..7d72b4b7fd 100644
--- a/src/common/compat.c
+++ b/src/common/compat.c
@@ -3424,3 +3424,4 @@ tor_get_avail_disk_space(const char *path)
   return -1;
 #endif
 }
+
diff --git a/src/common/container.h b/src/common/container.h
index 5abd8b48d9..bf4f04762c 100644
--- a/src/common/container.h
+++ b/src/common/container.h
@@ -110,7 +110,8 @@ void smartlist_sort_digests256(smartlist_t *sl);
 void smartlist_sort_pointers(smartlist_t *sl);
 
 const char *smartlist_get_most_frequent_string(smartlist_t *sl);
-const char *smartlist_get_most_frequent_string_(smartlist_t *sl, int *count_out);
+const char *smartlist_get_most_frequent_string_(smartlist_t *sl,
+                                                int *count_out);
 const uint8_t *smartlist_get_most_frequent_digest256(smartlist_t *sl);
 
 void smartlist_uniq_strings(smartlist_t *sl);
diff --git a/src/common/workqueue.c b/src/common/workqueue.c
index b0b004dc25..c467bdf43b 100644
--- a/src/common/workqueue.c
+++ b/src/common/workqueue.c
@@ -25,7 +25,7 @@ struct threadpool_s {
   unsigned generation;
 
   /** Function that should be run for updates on each thread. */
-  int (*update_fn)(void *, void *);
+  workqueue_reply_t (*update_fn)(void *, void *);
   /** Function to free update arguments if they can't be run. */
   void (*free_update_arg_fn)(void *);
   /** Array of n_threads update arguments. */
@@ -56,7 +56,7 @@ struct workqueue_entry_s {
   /** True iff this entry is waiting for a worker to start processing it. */
   uint8_t pending;
   /** Function to run in the worker thread. */
-  int (*fn)(void *state, void *arg);
+  workqueue_reply_t (*fn)(void *state, void *arg);
   /** Function to run while processing the reply queue. */
   void (*reply_fn)(void *arg);
   /** Argument for the above functions. */
@@ -96,7 +96,7 @@ static void queue_reply(replyqueue_t *queue, workqueue_entry_t *work);
  * <b>fn</b> in the worker thread, and <b>reply_fn</b> in the main
  * thread. See threadpool_queue_work() for full documentation. */
 static workqueue_entry_t *
-workqueue_entry_new(int (*fn)(void*, void*),
+workqueue_entry_new(workqueue_reply_t (*fn)(void*, void*),
                     void (*reply_fn)(void*),
                     void *arg)
 {
@@ -172,7 +172,7 @@ worker_thread_main(void *thread_)
   workerthread_t *thread = thread_;
   threadpool_t *pool = thread->in_pool;
   workqueue_entry_t *work;
-  int result;
+  workqueue_reply_t result;
 
   tor_mutex_acquire(&pool->lock);
   while (1) {
@@ -182,13 +182,14 @@ worker_thread_main(void *thread_)
       if (thread->in_pool->generation != thread->generation) {
         void *arg = thread->in_pool->update_args[thread->index];
         thread->in_pool->update_args[thread->index] = NULL;
-        int (*update_fn)(void*,void*) = thread->in_pool->update_fn;
+        workqueue_reply_t (*update_fn)(void*,void*) =
+            thread->in_pool->update_fn;
         thread->generation = thread->in_pool->generation;
         tor_mutex_release(&pool->lock);
 
-        int r = update_fn(thread->state, arg);
+        workqueue_reply_t r = update_fn(thread->state, arg);
 
-        if (r < 0) {
+        if (r != WQ_RPL_REPLY) {
           return;
         }
 
@@ -208,7 +209,7 @@ worker_thread_main(void *thread_)
       queue_reply(thread->reply_queue, work);
 
       /* We may need to exit the thread. */
-      if (result >= WQ_RPL_ERROR) {
+      if (result != WQ_RPL_REPLY) {
         return;
       }
       tor_mutex_acquire(&pool->lock);
@@ -281,7 +282,7 @@ workerthread_new(void *state, threadpool_t *pool, replyqueue_t *replyqueue)
  */
 workqueue_entry_t *
 threadpool_queue_work(threadpool_t *pool,
-                      int (*fn)(void *, void *),
+                      workqueue_reply_t (*fn)(void *, void *),
                       void (*reply_fn)(void *),
                       void *arg)
 {
@@ -318,7 +319,7 @@ threadpool_queue_work(threadpool_t *pool,
 int
 threadpool_queue_update(threadpool_t *pool,
                          void *(*dup_fn)(void *),
-                         int (*fn)(void *, void *),
+                         workqueue_reply_t (*fn)(void *, void *),
                          void (*free_fn)(void *),
                          void *arg)
 {
diff --git a/src/common/workqueue.h b/src/common/workqueue.h
index 92e82b8a48..9ce1eadafc 100644
--- a/src/common/workqueue.h
+++ b/src/common/workqueue.h
@@ -15,21 +15,22 @@ typedef struct threadpool_s threadpool_t;
  * pool. */
 typedef struct workqueue_entry_s workqueue_entry_t;
 
-/** Possible return value from a work function: indicates success. */
-#define WQ_RPL_REPLY    0
-/** Possible return value from a work function: indicates fatal error */
-#define WQ_RPL_ERROR    1
-/** Possible return value from a work function: indicates thread is shutting
- * down. */
-#define WQ_RPL_SHUTDOWN 2
+/** Possible return value from a work function: */
+typedef enum {
+  WQ_RPL_REPLY = 0, /** indicates success */
+  WQ_RPL_ERROR = 1, /** indicates fatal error */
+  WQ_RPL_SHUTDOWN = 2, /** indicates thread is shutting down */
+} workqueue_reply_t;
 
 workqueue_entry_t *threadpool_queue_work(threadpool_t *pool,
-                                         int (*fn)(void *, void *),
+                                         workqueue_reply_t (*fn)(void *,
+                                                                 void *),
                                          void (*reply_fn)(void *),
                                          void *arg);
+
 int threadpool_queue_update(threadpool_t *pool,
                             void *(*dup_fn)(void *),
-                            int (*fn)(void *, void *),
+                            workqueue_reply_t (*fn)(void *, void *),
                             void (*free_fn)(void *),
                             void *arg);
 void *workqueue_entry_cancel(workqueue_entry_t *pending_work);
diff --git a/src/or/control.c b/src/or/control.c
index ed2fedac32..220e7e514f 100644
--- a/src/or/control.c
+++ b/src/or/control.c
@@ -6478,3 +6478,4 @@ control_testing_set_global_event_mask(uint64_t mask)
   global_event_mask = mask;
 }
 #endif
+
diff --git a/src/or/cpuworker.c b/src/or/cpuworker.c
index d511ecf84c..76d97e05f2 100644
--- a/src/or/cpuworker.c
+++ b/src/or/cpuworker.c
@@ -160,7 +160,7 @@ typedef struct cpuworker_job_u {
   } u;
 } cpuworker_job_t;
 
-static int
+static workqueue_reply_t
 update_state_threadfn(void *state_, void *work_)
 {
   worker_state_t *state = state_;
@@ -387,7 +387,7 @@ cpuworker_onion_handshake_replyfn(void *work_)
 }
 
 /** Implementation function for onion handshake requests. */
-static int
+static workqueue_reply_t
 cpuworker_onion_handshake_threadfn(void *state_, void *work_)
 {
   worker_state_t *state = state_;
diff --git a/src/or/rendcache.c b/src/or/rendcache.c
index 9a33046fb6..fe9a1344f6 100644
--- a/src/or/rendcache.c
+++ b/src/or/rendcache.c
@@ -330,7 +330,7 @@ cache_failure_intro_lookup(const uint8_t *identity, const char *service_id,
     *intro_entry = intro_elem;
   }
   return 1;
-not_found:
+ not_found:
   return 0;
 }
 
diff --git a/src/or/rendclient.c b/src/or/rendclient.c
index c6f29a7707..a39e518e99 100644
--- a/src/or/rendclient.c
+++ b/src/or/rendclient.c
@@ -1021,7 +1021,7 @@ rend_client_report_intro_point_failure(extend_info_t *failed_intro,
         /* fall through */
       case INTRO_POINT_FAILURE_GENERIC:
         rend_cache_intro_failure_note(failure_type,
-                                      (uint8_t *) failed_intro->identity_digest,
+                                      (uint8_t *)failed_intro->identity_digest,
                                       rend_query->onion_address);
         rend_intro_point_free(intro);
         smartlist_del(ent->parsed->intro_nodes, i);
@@ -1038,9 +1038,10 @@ rend_client_report_intro_point_failure(extend_info_t *failed_intro,
                    intro->unreachable_count,
                    zap_intro_point ? " Removing from descriptor.": "");
           if (zap_intro_point) {
-            rend_cache_intro_failure_note(failure_type,
-                                          (uint8_t *) failed_intro->identity_digest,
-                                          rend_query->onion_address);
+            rend_cache_intro_failure_note(
+                failure_type,
+                (uint8_t *) failed_intro->identity_digest,
+                rend_query->onion_address);
             rend_intro_point_free(intro);
             smartlist_del(ent->parsed->intro_nodes, i);
           }
diff --git a/src/test/test_util.c b/src/test/test_util.c
index 8b4513d34c..0a5783e9f5 100644
--- a/src/test/test_util.c
+++ b/src/test/test_util.c
@@ -3654,7 +3654,6 @@ test_util_di_map(void *arg)
   dimap_free(dimap, tor_free_);
 }
 
-
 /**
  * Test counting high bits
  */
diff --git a/src/test/test_workqueue.c b/src/test/test_workqueue.c
index 7f20642041..0d79733cf0 100644
--- a/src/test/test_workqueue.c
+++ b/src/test/test_workqueue.c
@@ -70,7 +70,7 @@ mark_handled(int serial)
 #endif
 }
 
-static int
+static workqueue_reply_t
 workqueue_do_rsa(void *state, void *work)
 {
   rsa_work_t *rw = work;
@@ -98,7 +98,7 @@ workqueue_do_rsa(void *state, void *work)
   return WQ_RPL_REPLY;
 }
 
-static int
+static workqueue_reply_t
 workqueue_do_shutdown(void *state, void *work)
 {
   (void)state;
@@ -108,7 +108,7 @@ workqueue_do_shutdown(void *state, void *work)
   return WQ_RPL_SHUTDOWN;
 }
 
-static int
+static workqueue_reply_t
 workqueue_do_ecdh(void *state, void *work)
 {
   ecdh_work_t *ew = work;
@@ -124,6 +124,14 @@ workqueue_do_ecdh(void *state, void *work)
   return WQ_RPL_REPLY;
 }
 
+static workqueue_reply_t
+workqueue_shutdown_error(void *state, void *work)
+{
+  (void)state;
+  (void)work;
+  return WQ_RPL_REPLY;
+}
+
 static void *
 new_state(void *arg)
 {
@@ -156,6 +164,7 @@ static int n_sent = 0;
 static int rsa_sent = 0;
 static int ecdh_sent = 0;
 static int n_received = 0;
+static int no_shutdown = 0;
 
 #ifdef TRACK_RESPONSES
 bitarray_t *received;
@@ -174,6 +183,14 @@ handle_reply(void *arg)
   ++n_received;
 }
 
+/* This should never get called. */
+static void
+handle_reply_shutdown(void *arg)
+{
+  (void)arg;
+  no_shutdown = 1;
+}
+
 static workqueue_entry_t *
 add_work(threadpool_t *tp)
 {
@@ -288,6 +305,9 @@ replysock_readable_cb(tor_socket_t sock, short what, void *arg)
     shutting_down = 1;
     threadpool_queue_update(tp, NULL,
                              workqueue_do_shutdown, NULL, NULL);
+    // Anything we add after starting the shutdown must not be executed.
+    threadpool_queue_work(tp, workqueue_shutdown_error,
+                          handle_reply_shutdown, NULL);
     {
       struct timeval limit = { 2, 0 };
       tor_event_base_loopexit(tor_libevent_get_base(), &limit);
@@ -416,6 +436,9 @@ main(int argc, char **argv)
     printf("%d+%d vs %d\n", n_received, n_successful_cancel, n_sent);
     puts("FAIL");
     return 1;
+  } else if (no_shutdown) {
+    puts("Accepted work after shutdown\n");
+    puts("FAIL");
   } else {
     puts("OK");
     return 0;