6 files changed, 48 insertions, 7 deletions

diff --git a/src/lib/lock/compat_mutex.c b/src/lib/lock/compat_mutex.c
index 4ad5929715..b0084a3484 100644
--- a/src/lib/lock/compat_mutex.c
+++ b/src/lib/lock/compat_mutex.c
@@ -1,6 +1,6 @@
 /* Copyright (c) 2003-2004, Roger Dingledine
  * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson.
- * Copyright (c) 2007-2019, The Tor Project, Inc. */
+ * Copyright (c) 2007-2020, The Tor Project, Inc. */
 /* See LICENSE for licensing information */
 
 /**
@@ -29,7 +29,15 @@ tor_mutex_new_nonrecursive(void)
   tor_mutex_init_nonrecursive(m);
   return m;
 }
-/** Release all storage and system resources held by <b>m</b>. */
+/** Release all storage and system resources held by <b>m</b>.
+ *
+ * Destroying a locked mutex is undefined behaviour. Global mutexes may be
+ * locked when they are passed to this function, because multiple threads can
+ * still access them. So we can either:
+ *  - destroy on shutdown, and re-initialise when tor re-initialises, or
+ *  - skip destroying and re-initialisation, using a sentinel variable.
+ * See #31735 for details.
+ */
 void
 tor_mutex_free_(tor_mutex_t *m)
 {
diff --git a/src/lib/lock/compat_mutex.h b/src/lib/lock/compat_mutex.h
index b63ce24024..5631993cc4 100644
--- a/src/lib/lock/compat_mutex.h
+++ b/src/lib/lock/compat_mutex.h
@@ -1,6 +1,6 @@
 /* Copyright (c) 2003-2004, Roger Dingledine
  * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson.
- * Copyright (c) 2007-2019, The Tor Project, Inc. */
+ * Copyright (c) 2007-2020, The Tor Project, Inc. */
 /* See LICENSE for licensing information */
 
 /**
@@ -48,7 +48,7 @@ typedef struct tor_mutex_t {
 #else
   /** No-threads only: Dummy variable so that tor_mutex_t takes up space. */
   int _unused;
-#endif /* defined(USE_WIN32_MUTEX) || ... */
+#endif /* defined(USE_WIN32_THREADS) || ... */
 } tor_mutex_t;
 
 tor_mutex_t *tor_mutex_new(void);
@@ -58,6 +58,11 @@ void tor_mutex_init_nonrecursive(tor_mutex_t *m);
 void tor_mutex_acquire(tor_mutex_t *m);
 void tor_mutex_release(tor_mutex_t *m);
 void tor_mutex_free_(tor_mutex_t *m);
+/**
+ * @copydoc tor_mutex_free_
+ *
+ * Additionally, set the pointer <b>m</b> to NULL.
+ **/
 #define tor_mutex_free(m) FREE_AND_NULL(tor_mutex_t, tor_mutex_free_, (m))
 void tor_mutex_uninit(tor_mutex_t *m);
 
diff --git a/src/lib/lock/compat_mutex_pthreads.c b/src/lib/lock/compat_mutex_pthreads.c
index ee5f520cd0..ac83c42a47 100644
--- a/src/lib/lock/compat_mutex_pthreads.c
+++ b/src/lib/lock/compat_mutex_pthreads.c
@@ -1,6 +1,6 @@
 /* Copyright (c) 2003-2004, Roger Dingledine
  * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson.
- * Copyright (c) 2007-2019, The Tor Project, Inc. */
+ * Copyright (c) 2007-2020, The Tor Project, Inc. */
 /* See LICENSE for licensing information */
 
 /**
@@ -17,8 +17,14 @@
  * "recursive" mutexes (i.e., once we can re-lock if we're already holding
  * them.) */
 static pthread_mutexattr_t attr_recursive;
+/**
+ * True iff <b>attr_recursive</b> has been initialized.
+ **/
 static int attr_initialized = 0;
 
+/**
+ * Initialize the locking module, if it is not already initialized.
+ **/
 void
 tor_locking_init(void)
 {
@@ -88,12 +94,26 @@ tor_mutex_release(tor_mutex_t *m)
 }
 /** Clean up the mutex <b>m</b> so that it no longer uses any system
  * resources.  Does not free <b>m</b>.  This function must only be called on
- * mutexes from tor_mutex_init(). */
+ * mutexes from tor_mutex_init().
+ *
+ * Destroying a locked mutex is undefined behaviour. Global mutexes may be
+ * locked when they are passed to this function, because multiple threads can
+ * still access them. So we can either:
+ *  - destroy on shutdown, and re-initialise when tor re-initialises, or
+ *  - skip destroying and re-initialisation, using a sentinel variable.
+ * See #31735 for details.
+ */
 void
 tor_mutex_uninit(tor_mutex_t *m)
 {
   int err;
   raw_assert(m);
+  /* If the mutex is already locked, wait until after it is unlocked to destroy
+   * it. Locking and releasing the mutex makes undefined behaviour less likely,
+   * but does not prevent it. Another thread can lock the mutex between release
+   * and destroy. */
+  tor_mutex_acquire(m);
+  tor_mutex_release(m);
   err = pthread_mutex_destroy(&m->mutex);
   if (PREDICT_UNLIKELY(err)) {
     // LCOV_EXCL_START
diff --git a/src/lib/lock/compat_mutex_winthreads.c b/src/lib/lock/compat_mutex_winthreads.c
index b0f5999e42..5fe6870a93 100644
--- a/src/lib/lock/compat_mutex_winthreads.c
+++ b/src/lib/lock/compat_mutex_winthreads.c
@@ -1,6 +1,6 @@
 /* Copyright (c) 2003-2004, Roger Dingledine
  * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson.
- * Copyright (c) 2007-2019, The Tor Project, Inc. */
+ * Copyright (c) 2007-2020, The Tor Project, Inc. */
 /* See LICENSE for licensing information */
 
 /**
diff --git a/src/lib/lock/include.am b/src/lib/lock/include.am
index 4e6f444347..1475b9911b 100644
--- a/src/lib/lock/include.am
+++ b/src/lib/lock/include.am
@@ -5,6 +5,7 @@ if UNITTESTS_ENABLED
 noinst_LIBRARIES += src/lib/libtor-lock-testing.a
 endif
 
+# ADD_C_FILE: INSERT SOURCES HERE.
 src_lib_libtor_lock_a_SOURCES =			\
 	src/lib/lock/compat_mutex.c
 
@@ -20,5 +21,6 @@ src_lib_libtor_lock_testing_a_SOURCES = \
 src_lib_libtor_lock_testing_a_CPPFLAGS = $(AM_CPPFLAGS) $(TEST_CPPFLAGS)
 src_lib_libtor_lock_testing_a_CFLAGS = $(AM_CFLAGS) $(TEST_CFLAGS)
 
+# ADD_C_FILE: INSERT HEADERS HERE.
 noinst_HEADERS +=					\
 	src/lib/lock/compat_mutex.h
diff --git a/src/lib/lock/lib_lock.md b/src/lib/lock/lib_lock.md
new file mode 100644
index 0000000000..6f6727bfc2
--- /dev/null
+++ b/src/lib/lock/lib_lock.md
@@ -0,0 +1,6 @@
+@dir /lib/lock
+@brief lib/lock: Simple locking support.
+
+This module is more low-level than the rest of the threading code, since it
+is needed by more intermediate-level modules.
+