Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

20241108-WOLFSSL_CLEANUP_THREADSAFE #8169

Merged
merged 3 commits into from
Nov 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/internal.c
Original file line number Diff line number Diff line change
Expand Up @@ -25672,7 +25672,9 @@ const char* wolfSSL_ERR_reason_error_string(unsigned long e)
}

/* pass to wolfCrypt */
if (error <= WC_FIRST_E && error >= WC_LAST_E) {
if ((error <= WC_SPAN1_FIRST_E && error >= WC_SPAN1_MIN_CODE_E) ||
(error <= WC_SPAN2_FIRST_E && error >= WC_SPAN2_MIN_CODE_E))
{
return wc_GetErrorString(error);
}

Expand All @@ -25684,7 +25686,7 @@ const char* wolfSSL_ERR_reason_error_string(unsigned long e)
#endif
}

switch ((enum wolfSSL_ErrorCodes)error) {
switch ((enum wolfSSL_ErrorCodes)error) { /* // NOLINT(clang-analyzer-optin.core.EnumCastOutOfRange) */

case UNSUPPORTED_SUITE :
return "unsupported cipher suite";
Expand Down
72 changes: 66 additions & 6 deletions src/ssl.c
Original file line number Diff line number Diff line change
Expand Up @@ -1037,7 +1037,7 @@ static volatile WOLFSSL_GLOBAL int initRefCount = 0;
static WOLFSSL_GLOBAL wolfSSL_Mutex inits_count_mutex
WOLFSSL_MUTEX_INITIALIZER_CLAUSE(inits_count_mutex);
#ifndef WOLFSSL_MUTEX_INITIALIZER
static WOLFSSL_GLOBAL int inits_count_mutex_valid = 0;
static WOLFSSL_GLOBAL volatile int inits_count_mutex_valid = 0;
#endif

/* Create a new WOLFSSL_CTX struct and return the pointer to created struct.
Expand Down Expand Up @@ -5641,12 +5641,48 @@ int AddCA(WOLFSSL_CERT_MANAGER* cm, DerBuffer** pDer, int type, int verify)
static int wolfSSL_RAND_InitMutex(void);
#endif

/* If we don't have static mutex initializers, but we do have static atomic
* initializers, activate WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS to leverage
* the latter.
*
* See further explanation below in wolfSSL_Init().
*/
#ifndef WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS
#if !defined(WOLFSSL_MUTEX_INITIALIZER) && !defined(SINGLE_THREADED) && \
defined(WOLFSSL_ATOMIC_OPS) && defined(WOLFSSL_ATOMIC_INITIALIZER)
#define WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS 1
#else
#define WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS 0
#endif
#elif defined(WOLFSSL_MUTEX_INITIALIZER) || defined(SINGLE_THREADED)
#undef WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS
#define WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS 0
#endif
Comment on lines +5650 to +5660
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why undef & define and just define in the other?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to force the feature off we define WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS to 0. the user can also force it off by defining to 0.

the top half is followed if WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS has no definition either way, in which case the default setup is used, following the config settings.

the bottom half is followed if the user config activated WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS, but it needs to be forced off because it's not compatible with other config settings.


#if WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS
#ifndef WOLFSSL_ATOMIC_OPS
#error WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS requires WOLFSSL_ATOMIC_OPS
#endif
#ifndef WOLFSSL_ATOMIC_INITIALIZER
#error WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS requires WOLFSSL_ATOMIC_INITIALIZER
#endif
static wolfSSL_Atomic_Int inits_count_mutex_atomic_initing_flag =
WOLFSSL_ATOMIC_INITIALIZER(0);
#endif /* WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS && !WOLFSSL_MUTEX_INITIALIZER */

#if defined(OPENSSL_EXTRA) && defined(HAVE_ATEXIT)
static void AtExitCleanup(void)
{
if (initRefCount > 0) {
initRefCount = 1;
(void)wolfSSL_Cleanup();
#if WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS
if (inits_count_mutex_valid == 1) {
(void)wc_FreeMutex(&inits_count_mutex);
inits_count_mutex_valid = 0;
inits_count_mutex_atomic_initing_flag = 0;
}
#endif
}
}
#endif
Expand All @@ -5663,8 +5699,31 @@ int wolfSSL_Init(void)

#ifndef WOLFSSL_MUTEX_INITIALIZER
if (inits_count_mutex_valid == 0) {
#if WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS

/* Without this mitigation, if two threads enter wolfSSL_Init() at the
* same time, and both see zero inits_count_mutex_valid, then both will
* run wc_InitMutex(&inits_count_mutex), leading to process corruption
* or (best case) a resource leak.
*
* When WOLFSSL_ATOMIC_INITIALIZER() is available, we can mitigate this
* by use an atomic counting int as a mutex.
*/

if (wolfSSL_Atomic_Int_FetchAdd(&inits_count_mutex_atomic_initing_flag,
1) != 0)
{
(void)wolfSSL_Atomic_Int_FetchSub(
&inits_count_mutex_atomic_initing_flag, 1);
return DEADLOCK_AVERTED_E;
}
#endif /* WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS */
if (wc_InitMutex(&inits_count_mutex) != 0) {
WOLFSSL_MSG("Bad Init Mutex count");
#if WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS
(void)wolfSSL_Atomic_Int_FetchSub(
&inits_count_mutex_atomic_initing_flag, 1);
#endif
return BAD_MUTEX_E;
}
else {
Expand Down Expand Up @@ -10422,7 +10481,8 @@ int wolfSSL_Cleanup(void)
#endif
#endif /* !NO_SESSION_CACHE */

#ifndef WOLFSSL_MUTEX_INITIALIZER
#if !defined(WOLFSSL_MUTEX_INITIALIZER) && \
!WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS
if ((inits_count_mutex_valid == 1) &&
(wc_FreeMutex(&inits_count_mutex) != 0)) {
if (ret == WOLFSSL_SUCCESS)
Expand Down Expand Up @@ -15729,11 +15789,11 @@ int wolfSSL_ERR_GET_REASON(unsigned long err)
return ASN1_R_HEADER_TOO_LONG;
#endif

/* check if error value is in range of wolfSSL errors */
/* check if error value is in range of wolfCrypt or wolfSSL errors */
ret = 0 - ret; /* setting as negative value */
/* wolfCrypt range is less than MAX (-100)
wolfSSL range is MIN (-300) and lower */
if ((ret <= WC_FIRST_E && ret >= WC_LAST_E) ||

if ((ret <= WC_SPAN1_FIRST_E && ret >= WC_SPAN1_LAST_E) ||
(ret <= WC_SPAN2_FIRST_E && ret >= WC_SPAN2_LAST_E) ||
(ret <= WOLFSSL_FIRST_E && ret >= WOLFSSL_LAST_E))
{
return ret;
Expand Down
10 changes: 6 additions & 4 deletions support/gen-debug-trace-error-codes.sh
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,12 @@ BEGIN {
if ((errcode_a[1] == "MIN_CODE_E") ||
(errcode_a[1] == "MAX_CODE_E") ||
(errcode_a[1] == "WC_FIRST_E") ||
(errcode_a[1] == "WC_LAST_E") ||
(errcode_a[1] == "WOLFSSL_FIRST_E") ||
(errcode_a[1] == "WOLFSSL_LAST_E"))
(errcode_a[1] ~ "WC.*MIN_CODE_E") ||
(errcode_a[1] ~ "WC.*MAX_CODE_E") ||
(errcode_a[1] ~ "WC.*_FIRST_E") ||
(errcode_a[1] ~ "WC.*_LAST_E") ||
(errcode_a[1] ~ "WOLFSSL.*_FIRST_E") ||
(errcode_a[1] ~ "WOLFSSL.*_LAST_E"))
{
next;
}
Expand Down
12 changes: 6 additions & 6 deletions tests/api.c
Original file line number Diff line number Diff line change
Expand Up @@ -89866,9 +89866,9 @@ static int error_test(void)
{ -15, -17 },
{ -19, -19 },
{ -26, -27 },
{ -30, WC_FIRST_E+1 },
{ -30, WC_SPAN1_FIRST_E + 1 },
#else
{ -9, WC_FIRST_E+1 },
{ -9, WC_SPAN1_FIRST_E + 1 },
#endif
{ -124, -124 },
{ -166, -169 },
Expand All @@ -89879,14 +89879,15 @@ static int error_test(void)
{ -358, -358 },
{ -384, -384 },
{ -466, -499 },
{ WOLFSSL_LAST_E-1, WOLFSSL_LAST_E-1 }
{ WOLFSSL_LAST_E - 1, WC_SPAN2_FIRST_E + 1 },
{ WC_SPAN2_LAST_E - 1, MIN_CODE_E }
};

/* Check that all errors have a string and it's the same through the two
* APIs. Check that the values that are not errors map to the unknown
* string.
*/
for (i = 0; i >= WOLFSSL_LAST_E-1; i--) {
for (i = 0; i >= MIN_CODE_E; i--) {
int this_missing = 0;
for (j = 0; j < (int)XELEM_CNT(missing); ++j) {
if ((i <= missing[j].first) && (i >= missing[j].last)) {
Expand Down Expand Up @@ -89948,8 +89949,7 @@ static int test_wolfSSL_ERR_strings(void)
ExpectNotNull(err = wolfSSL_ERR_func_error_string(WC_NO_ERR_TRACE((word32)UNSUPPORTED_SUITE)));
ExpectIntEQ((*err == '\0'), 1);

/* The value -MIN_CODE_E+2 is PEM_R_PROBLEMS_GETTING_PASSWORD. */
ExpectNotNull(err = wolfSSL_ERR_lib_error_string(-MIN_CODE_E+2));
ExpectNotNull(err = wolfSSL_ERR_lib_error_string(-WOLFSSL_PEM_R_PROBLEMS_GETTING_PASSWORD_E));
ExpectIntEQ((*err == '\0'), 1);
#endif
#endif
Expand Down
6 changes: 4 additions & 2 deletions wolfcrypt/src/error.c
Original file line number Diff line number Diff line change
Expand Up @@ -642,11 +642,14 @@ const char* wc_GetErrorString(int error)
case PBKDF2_KAT_FIPS_E:
return "wolfCrypt FIPS PBKDF2 Known Answer Test Failure";

case DEADLOCK_AVERTED_E:
return "Deadlock averted -- retry the call";

case MAX_CODE_E:
case WC_SPAN1_MIN_CODE_E:
case MIN_CODE_E:
default:
return "unknown error number";

}
}

Expand All @@ -660,4 +663,3 @@ void wc_ErrorString(int error, char* buffer)
buffer[WOLFSSL_MAX_ERROR_SZ-1] = 0;
}
#endif /* !NO_ERROR_STRINGS */

6 changes: 4 additions & 2 deletions wolfcrypt/test/test.c
Original file line number Diff line number Diff line change
Expand Up @@ -2777,14 +2777,16 @@ WOLFSSL_TEST_SUBROUTINE wc_test_ret_t error_test(void)
int last;
} missing[] = {
{ -124, -124 },
{ -166, -169 }
{ -166, -169 },
{ WC_SPAN1_LAST_E - 1, WC_SPAN2_FIRST_E + 1 },
{ WC_SPAN2_LAST_E - 1, WC_SPAN2_MIN_CODE_E }
};

/* Check that all errors have a string and it's the same through the two
* APIs. Check that the values that are not errors map to the unknown
* string.
*/
for (i = WC_FIRST_E; i >= WC_LAST_E; i--) {
for (i = WC_SPAN1_FIRST_E; i >= WC_SPAN2_MIN_CODE_E; i--) {
int this_missing = 0;
for (j = 0; j < (int)XELEM_CNT(missing); ++j) {
if ((i <= missing[j].first) && (i >= missing[j].last)) {
Expand Down
4 changes: 4 additions & 0 deletions wolfssl/error-ssl.h
Original file line number Diff line number Diff line change
Expand Up @@ -233,8 +233,12 @@ enum wolfSSL_ErrorCodes {
WOLFSSL_EVP_R_PRIVATE_KEY_DECODE_ERROR = -515,

WOLFSSL_LAST_E = -515

/* codes -1000 to -1999 are reserved for wolfCrypt. */
};

wc_static_assert((int)WC_LAST_E <= (int)WOLFSSL_LAST_E);

/* I/O Callback default errors */
enum IOerrors {
WOLFSSL_CBIO_ERR_GENERAL = -1, /* general unexpected err */
Expand Down
29 changes: 25 additions & 4 deletions wolfssl/wolfcrypt/error-crypt.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,10 @@ enum wolfCrypt_ErrorCodes {
* reasons of backward compatibility.
*/

MAX_CODE_E = -96, /* errors -97 - -299 */
WC_FIRST_E = -97, /* errors -97 - -299 */
MAX_CODE_E = -96, /* WC_FIRST_E + 1, for backward compat. */
WC_FIRST_E = -97, /* First code used for wolfCrypt */

WC_SPAN1_FIRST_E = -97, /* errors -97 - -300 */

MP_MEM = -97, /* MP dynamic memory allocation failed. */
MP_VAL = -98, /* MP value passed is not able to be used. */
Expand Down Expand Up @@ -290,13 +292,32 @@ enum wolfCrypt_ErrorCodes {
SM4_GCM_AUTH_E = -298, /* SM4-GCM Authentication check failure */
SM4_CCM_AUTH_E = -299, /* SM4-CCM Authentication check failure */

WC_LAST_E = -299, /* Update this to indicate last error */
MIN_CODE_E = -300 /* errors -2 - -299 */
WC_SPAN1_LAST_E = -299, /* Last used code in span 1 */
WC_SPAN1_MIN_CODE_E = -300, /* Last usable code in span 1 */

WC_SPAN2_FIRST_E = -1000,

DEADLOCK_AVERTED_E = -1000, /* Deadlock averted -- retry the call */

WC_SPAN2_LAST_E = -1000, /* Update to indicate last used error code */
WC_SPAN2_MIN_CODE_E = -1999, /* Last usable code in span 2 */

WC_LAST_E = -1000, /* the last code used either here or in
* error-ssl.h
*/

MIN_CODE_E = -1999 /* the last code allocated either here or in
* error-ssl.h
*/

/* add new companion error id strings for any new error codes
wolfcrypt/src/error.c !!! */
};

wc_static_assert((int)WC_LAST_E <= (int)WC_SPAN2_LAST_E);
wc_static_assert((int)MIN_CODE_E <= (int)WC_LAST_E);
wc_static_assert((int)MIN_CODE_E <= (int)WC_SPAN2_MIN_CODE_E);

#ifdef NO_ERROR_STRINGS
#define wc_GetErrorString(error) "no support for error strings built in"
#define wc_ErrorString(err, buf) \
Expand Down
7 changes: 7 additions & 0 deletions wolfssl/wolfcrypt/wc_port.h
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,10 @@

#endif /* SINGLE_THREADED */

#ifdef WOLFSSL_TEST_NO_MUTEX_INITIALIZER
#undef WOLFSSL_MUTEX_INITIALIZER
#endif

#ifdef WOLFSSL_MUTEX_INITIALIZER
#define WOLFSSL_MUTEX_INITIALIZER_CLAUSE(lockname) = WOLFSSL_MUTEX_INITIALIZER(lockname)
#else
Expand All @@ -336,13 +340,15 @@
#if defined(__GNUC__) && defined(__ATOMIC_RELAXED)
/* C++ using direct calls to compiler built-in functions */
typedef volatile int wolfSSL_Atomic_Int;
#define WOLFSSL_ATOMIC_INITIALIZER(x) (x)
#define WOLFSSL_ATOMIC_OPS
#endif
#else
#ifdef WOLFSSL_HAVE_ATOMIC_H
/* Default C Implementation */
#include <stdatomic.h>
typedef atomic_int wolfSSL_Atomic_Int;
#define WOLFSSL_ATOMIC_INITIALIZER(x) (x)
#define WOLFSSL_ATOMIC_OPS
#endif /* WOLFSSL_HAVE_ATOMIC_H */
#endif
Expand All @@ -354,6 +360,7 @@
#include <intrin.h>
#endif
typedef volatile long wolfSSL_Atomic_Int;
#define WOLFSSL_ATOMIC_INITIALIZER(x) (x)
#define WOLFSSL_ATOMIC_OPS
#endif
#endif /* WOLFSSL_NO_ATOMICS */
Expand Down
Loading