[RFC] [DON'T MERGE YET] Make crypt and crypt_gensalt use thread-local output buffers.

.gitignore

@@ -69,6 +69,7 @@
 /test-crypt-des
 /test-crypt-gost-yescrypt
 /test-crypt-md5
+/test-crypt-mt
 /test-crypt-nthash
 /test-crypt-pbkdf1-sha1
 /test-crypt-scrypt

Makefile.am

@@ -237,9 +237,9 @@ check_PROGRAMS = \
 	test-crypt-sha256 test-crypt-sha512 test-crypt-sunmd5 \
 	test-crypt-yescrypt test-byteorder test-badsalt test-badsetting \
 	test-gensalt test-gensalt-extradata test-gensalt-nthash \
-	test-preferred-method test-crypt-badargs test-short-outbuf \
-	test-compile-strong-alias test-getrandom-interface \
-	test-getrandom-fallbacks
+	test-preferred-method test-crypt-badargs test-crypt-mt \
+	test-short-outbuf test-compile-strong-alias \
+	test-getrandom-interface test-getrandom-fallbacks
 
 if ENABLE_OBSOLETE_API
 libcrypt_la_SOURCES += crypt-des-obsolete.c
@@ -300,9 +300,14 @@ test_des_obsolete_LDADD = $(COMMON_TEST_OBJECTS)
 test_des_obsolete_r_LDADD = $(COMMON_TEST_OBJECTS)
 test_fcrypt_enosys_LDADD = $(COMMON_TEST_OBJECTS)
 test_crypt_badargs_LDADD = $(COMMON_TEST_OBJECTS)
+test_crypt_mt_LDADD = $(COMMON_TEST_OBJECTS)
 test_short_outbuf_LDADD = $(COMMON_TEST_OBJECTS)
 test_preferred_method_LDADD = $(COMMON_TEST_OBJECTS)
 
+# test-crypt-mt uses threads
+test_crypt_mt_CFLAGS = $(PTHREAD_CFLAGS)
+test_crypt_mt_LIBS = $(PTHREAD_LIBS)
+
 # These tests call internal APIs that may not be accessible from the
 # fully linked shared library.
 test_alg_des_LDADD = \
@@ -372,7 +377,13 @@ endif
 # sufficient.
 include $(builddir)/Makefile.deps
 
-# Add additional targets
+# Additional targets
+
+# this target allows you to recompile all the test programs but not
+# run them
+check_PROGRAMS: $(check_PROGRAMS)
+phony_targets += check_PROGRAMS
+
 .PHONY: $(phony_targets)
 install-exec-hook: $(install_exec_hook_targets)
 uninstall-hook: $(uninstall_hook_targets)

NEWS

@@ -4,6 +4,18 @@ Please send bug reports, questions and suggestions to
 <https://github.com/besser82/libxcrypt/issues>.
 
 Version 4.4.4
+* crypt and crypt_gensalt now use per-thread storage areas for their
+  output, allocated upon the first call in each thread that uses them.
+  This makes it safe to call these functions from multiple threads
+  simultaneously (but consecutive calls will still clobber the
+  previous output).
+  This feature is a safety net against sloppy coding.  Programs are
+  still strongly encouraged to use the reentrant functions instead,
+  both because this safety net is not guaranteed by any standard
+  (although we are informed that Solaris also does this) and because
+  there is no way to arrange for the storage areas to be _deallocated_
+  at an appropriate time, so programs that call these functions from
+  many short-lived threads will have a memory leak.
 
 Version 4.4.3
 * Fix the value of SUNMD5_MAX_ROUNDS.

TODO.md

@@ -67,15 +67,6 @@ It was last updated 20 October 2018.
     * If we do, should it know how to trigger the trusted-path
       password prompt in modern GUI environments? (probably)
 
-  * Make the crypt and crypt_gensalt static state thread-specific?
-    * Solaris 11 may have done this (its `crypt(3)` manpage describes
-      it as MT-Safe and I don’t see any other way they could have
-      accomplished that).
-    * if allocated on first use, this would also shave 32kB of
-      data segment off the shared library
-    * alternatively, add a global lock and *crash the program* if we
-      detect concurrent calls
-
   * Allow access to more of yescrypt’s tunable parameters and ROM
     feature, in a way that’s generic enough that we could also use it
     for e.g. Argon2’s tunable parameters

configure.ac

@@ -70,6 +70,17 @@ AC_CACHE_CHECK([whether the preprocessor ($CPP) supports -dD],
 AM_CONDITIONAL([HAVE_CPP_dD], [test $ac_cv_prog_cc_dD = yes])
 
 # Checks for libraries.
+# some tests are multithreaded
+AX_PTHREAD([# action-if-true
+  # this could be worked around with sufficient hackery in the Makefile,
+  # maybe, but considering it only affects (old versions of?) AIX, I can't
+  # be bothered.
+  if test x"$PTHREAD_CC" != x"$CC"; then
+    AC_MSG_ERROR([Automake does not support PTHREAD_CC different from CC.])
+  fi
+  AC_DEFINE([HAVE_PTHREAD],[1],
+      [Define if you have POSIX threads libraries and header files.])
+])
 
 # Checks for header files.
 AC_CHECK_HEADERS_ONCE(
@@ -120,6 +131,7 @@ if test $ac_cv_header_sys_cdefs_THROW = yes; then
 fi
 
 # Checks for typedefs, structures, and compiler characteristics.
+AX_TLS
 zw_C_ALIGNAS
 zw_C_ALIGNOF
 zw_C_MAX_ALIGN_T

crypt-gensalt-static.c

@@ -17,19 +17,32 @@
 #include "crypt-port.h"
 #include "xcrypt.h"
 
+#include <stdlib.h>
+
 /* The functions that use global state objects are isolated in their
    own files so that a statically-linked program that doesn't use them
    will not have the state objects in its data segment.  */
 
 #if INCLUDE_crypt_gensalt
+
+static THREAD_LOCAL char *obuf = NULL;
+
 char *
 crypt_gensalt (const char *prefix, unsigned long count,
                const char *rbytes, int nrbytes)
 {
-  static char output[CRYPT_GENSALT_OUTPUT_SIZE];
+  if (!obuf) /* first call in this thread */
+    {
+      obuf = malloc (CRYPT_GENSALT_OUTPUT_SIZE);
+      if (!obuf)
+        return 0;
+#if HAVE_GLIBC_CXA_THREAD_ATEXIT_IMPL
+      __cxa_thread_atexit_impl (free, obuf, obuf);
+#endif
+    }
 
-  return crypt_gensalt_rn (prefix, count,
-                           rbytes, nrbytes, output, sizeof (output));
+  return crypt_gensalt_rn (prefix, count, rbytes, nrbytes,
+                           obuf, CRYPT_GENSALT_OUTPUT_SIZE);
 }
 SYMVER_crypt_gensalt;
 #endif

crypt-port.h

@@ -123,6 +123,19 @@ typedef union
 /* ARRAY_SIZE is used in tests.  */
 #define ARRAY_SIZE(a_)  (sizeof (a_) / sizeof ((a_)[0]))
 
+/* Thread local storage needs a way to be destructed, when the thread
+   is terminated.  Using a weak defined function from glibc >= 2.18
+   is the only way we currently know about.  */
+#if defined __GLIBC__ && ((__GLIBC__ == 2 && \
+    defined __GLIBC_MINOR__ && __GLIBC_MINOR__ >= 18) || \
+    __GLIBC__ >= 3)
+#define HAVE_GLIBC_CXA_THREAD_ATEXIT_IMPL 1
+extern int __cxa_thread_atexit_impl (void (*func) (void *), void *arg,
+                                     void *d);
+#else
+#define HAVE_GLIBC_CXA_THREAD_ATEXIT_IMPL 0
+#endif
+
 /* Provide a guaranteed way to erase sensitive memory at the best we
    can, given the possibilities of the system.  */
 #if defined HAVE_MEMSET_S

crypt-static.c

@@ -1,4 +1,4 @@
-/* Copyright (C) 2007-2017 Thorsten Kukuk
+/* Copyright (C) 2007-2018 Thorsten Kukuk
    Copyright (C) 2019 Björn Esser
 
    This library is free software; you can redistribute it and/or
@@ -17,18 +17,44 @@
 
 #include "crypt-port.h"
 #include "xcrypt.h"
+
 #include <errno.h>
+#include <stdlib.h>
 
 /* The functions that use global state objects are isolated in their
    own files so that a statically-linked program that doesn't use them
    will not have the state objects in its data segment.  */
 
 #if INCLUDE_crypt || INCLUDE_fcrypt
+
+static THREAD_LOCAL struct crypt_data *nr_crypt_ctx = NULL;
+
+#if ENABLE_FAILURE_TOKENS
+static THREAD_LOCAL char failure_token_crypt[3];
+#endif
+
 char *
 crypt (const char *key, const char *setting)
 {
-  static struct crypt_data nr_crypt_ctx;
-  return crypt_r (key, setting, &nr_crypt_ctx);
+  if (!nr_crypt_ctx) /* first call in this thread */
+    {
+      nr_crypt_ctx = malloc (sizeof (struct crypt_data));
+      if (!nr_crypt_ctx)
+        {
+          /* malloc has already set errno */
+#if ENABLE_FAILURE_TOKENS
+          make_failure_token (setting, failure_token_crypt, 3);
+          return failure_token_crypt;
+#else
+          return 0;
+#endif
+        }
+#if HAVE_GLIBC_CXA_THREAD_ATEXIT_IMPL
+      __cxa_thread_atexit_impl (free, nr_crypt_ctx, nr_crypt_ctx);
+#endif
+    }
+
+  return crypt_r (key, setting, nr_crypt_ctx);
 }
 #endif
 
@@ -38,6 +64,11 @@ SYMVER_crypt;
 
 #if INCLUDE_fcrypt
 #if ENABLE_OBSOLETE_API_ENOSYS
+
+#if ENABLE_FAILURE_TOKENS
+static THREAD_LOCAL char failure_token_fcrypt[3];
+#endif
+
 char *
 fcrypt (ARG_UNUSED (const char *key), ARG_UNUSED (const char *setting))
 {
@@ -46,9 +77,8 @@ fcrypt (ARG_UNUSED (const char *key), ARG_UNUSED (const char *setting))
 
 #if ENABLE_FAILURE_TOKENS
   /* Return static buffer filled with a failure-token.  */
-  static char retval[3];
-  make_failure_token (setting, retval, 3);
-  return retval;
+  make_failure_token (setting, failure_token_fcrypt, 3);
+  return failure_token_fcrypt;
 #else
   return NULL;
 #endif

crypt.3

@@ -218,7 +218,9 @@ for more detail on the format of hashed passphrases.
 places its result in a static storage area,
 which will be overwritten by subsequent calls to
 .Nm crypt .
-It is not safe to call
+In this version of libcrypt,
+the static storage area is allocated separately for each thread,
+but portable code should not call
 .Nm crypt
 from multiple threads simultaneously.
 .Pp
@@ -312,6 +314,15 @@ and
 .Nm crypt_ra
 are not part of any standard.
 .Pp
+POSIX permits
+.Nm crypt
+to use a single storage area for all callers,
+even in the presence of threads.
+This library uses per-thread storage areas,
+but portable code should avoid calling
+.Nm crypt
+from multiple threads simultaneously.
+.Pp
 POSIX does not specify any hashing methods,
 and does not require hashed passphrases to be portable between systems.
 In practice, hashed passphrases are portable
@@ -414,6 +425,19 @@ has no way of knowing how much memory to allocate.
 does the allocation itself,
 but can only make a single call to
 .Xr malloc 3 .
+.Pp
+The per-thread storage area used by
+.Nm crypt
+is allocated using
+.Xr malloc 3
+upon the first call in each thread.
+There is no way to arrange for it to be deallocated at an appropriate time,
+so it is never deallocated.
+Programs that call
+.Nm crypt
+from many short-lived threads will suffer a memory leak of
+.Li sizeof (struct crypt_data)
+bytes for each thread.
 .Sh ATTRIBUTES
 For an explanation of the terms used in this section, see
 .Xr attributes 7 .
@@ -424,14 +448,17 @@ l l l.
 Interface	Attribute	Value
 T{
 .Nm crypt
-T}	Thread safety	MT-Unsafe race:crypt
+T}	Thread safety	MT-Safe*
 T{
 .Nm crypt_r ,
 .Nm crypt_rn ,
 .Nm crypt_ra
 T}	Thread safety	MT-Safe
 .TE
-.sp
+.Pp
+\&* In this implementation.
+See
+.Sx PORTABILITY NOTES .
 .Sh HISTORY
 A rotor-based
 .Nm crypt

crypt_gensalt.3

@@ -130,12 +130,12 @@ to an appropriate error code.
 places its result in a static storage area,
 which will be overwritten by subsequent calls to
 .Nm crypt_gensalt .
-It is not safe to call
+In this version of libcrypt,
+the static storage area is allocated separately for each thread,
+but portable code should not call
 .Nm crypt_gensalt
 from multiple threads simultaneously.
-However, it
-.Em is
-safe to pass the string returned by
+It is safe to pass the string returned by
 .Nm crypt_gensalt
 directly to
 .Nm crypt
@@ -227,6 +227,15 @@ A function with the name
 .Nm crypt_gensalt
 also exists on Solaris 10 and newer, but its prototype and semantics differ.
 .Pp
+Some implementations of
+.Nm crypt_gensalt
+use a single storage area for all callers,
+even in the presence of threads.
+This library uses per-thread storage areas,
+but portable code should avoid calling
+.Nm crypt_gensalt
+from multiple threads simultaneously.
+.Pp
 The default prefix and auto entropy features are available since libxcrypt
 version 4.0.0.  Portable software can use feature test macros to find out
 whether null pointers can be used for the
@@ -247,13 +256,29 @@ l l l.
 Interface	Attribute	Value
 T{
 .Nm crypt_gensalt
-T}	Thread safety	MT-Unsafe race:crypt_gensalt
+T}	Thread safety	MT-Safe*
 T{
 .Nm crypt_gensalt_rn ,
 .Nm crypt_gensalt_ra
 T}	Thread safety	MT-Safe
 .TE
-.sp
+.Pp
+\&* In this implementation.
+See
+.Sx PORTABILITY NOTES .
+.Sh BUGS
+The per-thread storage area used by
+.Nm crypt_gensalt
+is allocated using
+.Xr malloc 3
+upon the first call in each thread.
+There is no way to arrange for it to be deallocated at an appropriate time,
+so it is never deallocated.
+Programs that call
+.Nm crypt_gensalt
+from many short-lived threads will suffer a memory leak of
+.Dv CRYPT_GENSALT_OUTPUT_SIZE
+bytes for each thread.
 .Sh SEE ALSO
 .Xr crypt 3 ,
 .Xr getpass 3 ,