Skip to content

20260512-wc_init_state#10472

Merged
dgarske merged 1 commit into
wolfSSL:masterfrom
douzzer:20260512-wc_init_state
Jun 8, 2026
Merged

20260512-wc_init_state#10472
dgarske merged 1 commit into
wolfSSL:masterfrom
douzzer:20260512-wc_init_state

Conversation

@douzzer

@douzzer douzzer commented May 12, 2026

Copy link
Copy Markdown
Contributor

wolfcrypt/src/wc_port.c: fix&refactor thread safety mechanisms in wolfCrypt_Init() and wolfCrypt_Cleanup().

This PR fixes unmitigated races in the incumbent code whereby three or more threads racing each other are subject to UB, with one or more of the threads returning success with the library not actually initialized.

The new algorithm is airtight, always succeeding unless API abuse or memory corruption.

tested with

wolfssl-multi-test.sh ...
check-source-text all-gcc-c99 all-c89-clang-tidy clang-tidy-all-intelasm clang-tidy-all-async-quic single-threaded clang-tidy-all-sp-all

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #10472

Scan targets checked: wolfcrypt-bugs, wolfcrypt-src

No new issues found in the changed files. ✅

@douzzer douzzer force-pushed the 20260512-wc_init_state branch 2 times, most recently from 17b7825 to 74aea5b Compare May 13, 2026 04:06
@github-actions

github-actions Bot commented May 13, 2026

Copy link
Copy Markdown

MemBrowse Memory Report

gcc-arm-cortex-m0plus

  • FLASH: .text +236 B (+0.4%, 63,467 B / 262,144 B, total: 24% used)

gcc-arm-cortex-m3

  • FLASH: .text +236 B (+0.2%, 121,141 B / 262,144 B, total: 46% used)

gcc-arm-cortex-m4

  • FLASH: .text +256 B (+0.1%, 198,446 B / 262,144 B, total: 76% used)

gcc-arm-cortex-m4-baremetal

  • FLASH: .text +256 B (+0.4%, 66,059 B / 262,144 B, total: 25% used)

gcc-arm-cortex-m4-crypto-only

  • FLASH: .text +256 B (+0.1%, 173,550 B / 262,144 B, total: 66% used)

gcc-arm-cortex-m4-dtls13

  • FLASH: .text +256 B (+0.1%, 179,544 B / 1,048,576 B, total: 17% used)

gcc-arm-cortex-m4-min-ecc

  • FLASH: .text +256 B (+0.4%, 61,037 B / 262,144 B, total: 23% used)

gcc-arm-cortex-m4-openssl-compat

  • FLASH: .text +256 B (+0.0%, 765,932 B / 1,048,576 B, total: 73% used)

gcc-arm-cortex-m4-pkcs7

  • FLASH: .text +256 B (+0.1%, 210,929 B / 262,144 B, total: 80% used)

gcc-arm-cortex-m4-pq

  • FLASH: .text +256 B (+0.1%, 277,144 B / 1,048,576 B, total: 26% used)

gcc-arm-cortex-m4-rsa-only

  • FLASH: .text +256 B (+0.1%, 322,360 B / 1,048,576 B, total: 31% used)

gcc-arm-cortex-m4-sp-math

  • FLASH: .text +256 B (+0.4%, 61,037 B / 262,144 B, total: 23% used)

gcc-arm-cortex-m4-tls12

  • FLASH: .text +192 B (+0.2%, 121,869 B / 262,144 B, total: 46% used)

gcc-arm-cortex-m4-tls13

  • FLASH: .text +256 B (+0.1%, 234,400 B / 262,144 B, total: 89% used)

gcc-arm-cortex-m7

  • FLASH: .text +256 B (+0.1%, 198,446 B / 262,144 B, total: 76% used)

gcc-arm-cortex-m7-pq

  • FLASH: .text +256 B (+0.1%, 277,720 B / 1,048,576 B, total: 26% used)

gcc-arm-cortex-m7-tls13

  • FLASH: .text +256 B (+0.1%, 234,464 B / 262,144 B, total: 89% used)

linuxkm-pie

  • Data: __patchable_function_entries +32 B (+0.1%, 24,208 B)

linuxkm-standard

  • Data: __patchable_function_entries +32 B (+0.1%, 45,904 B)
    No memory changes detected for:
  • stm32-sim-stm32h753

@douzzer

douzzer commented May 13, 2026

Copy link
Copy Markdown
Contributor Author

retest this please

@julek-wolfssl julek-wolfssl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This solution is quite complex. Can't we use the same approach of two vars (atomic + volatile) from wolfSSL_Init? The main improvement I can see in the relation between wolfSSL_Init and wolfSSL_Cleanup is that wolfSSL_Cleanup should not do anything if inits_count_mutex_valid is not 1.

@julek-wolfssl julek-wolfssl assigned douzzer and unassigned wolfSSL-Bot May 13, 2026
@douzzer

douzzer commented May 13, 2026

Copy link
Copy Markdown
Contributor Author

This solution uses a single 32 bit atomic. That's the simplest implementation supported by the hardware.

We're only doing this because code review uncovered races in the previous implementation.

And the refactored implementation only looks complex because I added a bunch of error-checking.

@julek-wolfssl julek-wolfssl self-requested a review May 13, 2026 16:25
@douzzer douzzer force-pushed the 20260512-wc_init_state branch from 74aea5b to df7d3da Compare May 14, 2026 21:13

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #10472

Scan targets checked: wolfcrypt-bugs, wolfcrypt-src

No new issues found in the changed files. ✅

@douzzer douzzer force-pushed the 20260512-wc_init_state branch 2 times, most recently from 2339d63 to f1c0935 Compare May 15, 2026 03:18

@julek-wolfssl julek-wolfssl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

All these new assumptions and limitations should be documented in doc/. At least the change to a busy loop, BAD_STATE blocking indefinitely, and compiler differences.

Comment thread wolfssl/wolfcrypt/wc_port.h
Comment thread wolfcrypt/src/wc_port.c
* wc_local_InitUp()
* wc_local_InitUpDone()
* wc_local_InitDown()
* wc_local_InitDownDone()
* wc_init_state_t
* WC_DECLARE_INIT_STATE()
* WC_INIT_STATE_*
* union wc_init_state_bitfields
* WC_INIT_STATE_RAISE_BAD_STATE()
* WC_ATOMIC_INT_ARG and WC_ATOMIC_UINT_ARG, pivoting on WC_16BIT_CPU, used to assure operands to atomic operators are 32 bits, and that wc_init_state_t is 32 bits, even on 16 bit targets like Arduino.

fix&refactor thread safety mechanisms in wolfCrypt_Init() and wolfCrypt_Cleanup(), and fix a few preexisting error-handling flubs in wolfCrypt_Init().

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #10472

Scan targets checked: none
Failed targets: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src

⚠️ Review incomplete — one or more scan targets failed before findings could be produced. See the Fenrir PR review detail page for logs.

@dgarske dgarske left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Very nice cleanup

@dgarske dgarske self-assigned this Jun 8, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors wolfCrypt_Init() / wolfCrypt_Cleanup() to use an explicit atomic state machine for initialization depth tracking, aiming to eliminate previously unmitigated multi-threaded races and ensure consistent initialization/cleanup behavior under contention.

Changes:

  • Introduces wc_local_InitUp/Down and wc_init_state_t-based state machine to coordinate init/cleanup transitions.
  • Adjusts atomic API argument/return types via WC_ATOMIC_{INT,UINT}_ARG (notably for WC_16BIT_CPU) and propagates those through atomic function signatures.
  • Adds WC_INIT_ERROR_WHEN_CONTENDED as a configurable behavior knob for contention handling.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.

File Description
wolfssl/wolfcrypt/wc_port.h Adds init-state types/macros and widens atomic argument typedefs for 16-bit targets.
wolfcrypt/src/wc_port.c Implements the init/cleanup state machine and rewires wolfCrypt_Init/Cleanup to use it; updates atomic backends’ signatures.
.wolfssl_known_macro_extras Registers WC_INIT_ERROR_WHEN_CONTENDED for tooling/macro awareness.
Comments suppressed due to low confidence (2)

wolfcrypt/src/wc_port.c:1718

  • wolfSSL_Atomic_Int_CompareExchange() assigns the observed value back to expected_i using an (int) cast, but expected_i is now WC_ATOMIC_INT_ARG. This can truncate when WC_ATOMIC_INT_ARG is wider than int.
int wolfSSL_Atomic_Int_CompareExchange(wolfSSL_Atomic_Int* c,
                                       WC_ATOMIC_INT_ARG *expected_i,
                                       WC_ATOMIC_INT_ARG new_i)
{
    u_int exp = (u_int) *expected_i;
    int ret = atomic_fcmpset_int(c, &exp, new_i);
    *expected_i = (int)exp;
    return ret;

wolfcrypt/src/wc_port.c:1728

  • wolfSSL_Atomic_Uint_CompareExchange() assigns the observed value back to expected_i using an (unsigned int) cast, but expected_i is now WC_ATOMIC_UINT_ARG. This can truncate when WC_ATOMIC_UINT_ARG is wider than unsigned int.
int wolfSSL_Atomic_Uint_CompareExchange(
    wolfSSL_Atomic_Uint* c, WC_ATOMIC_UINT_ARG *expected_i,
    WC_ATOMIC_UINT_ARG new_i)
{
    u_int exp = (u_int)*expected_i;
    int ret = atomic_fcmpset_int(c, &exp, new_i);
    *expected_i = (unsigned int)exp;
    return ret;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread wolfcrypt/src/wc_port.c
Comment thread wolfcrypt/src/wc_port.c
Comment thread wolfcrypt/src/wc_port.c
Comment thread wolfcrypt/src/wc_port.c
Comment thread wolfcrypt/src/wc_port.c
Comment thread wolfcrypt/src/wc_port.c
Comment thread wolfcrypt/src/wc_port.c
Comment thread wolfcrypt/src/wc_port.c
@dgarske dgarske merged commit da1de8a into wolfSSL:master Jun 8, 2026
481 of 482 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants