Skip to content

fix: guard wp_kdf_free refCnt with mutex#436

Open
MarkAtwood wants to merge 1 commit into
wolfSSL:masterfrom
MarkAtwood:fix/kdf-free-unlocked-refcnt
Open

fix: guard wp_kdf_free refCnt with mutex#436
MarkAtwood wants to merge 1 commit into
wolfSSL:masterfrom
MarkAtwood:fix/kdf-free-unlocked-refcnt

Conversation

@MarkAtwood

Copy link
Copy Markdown

Bug

In wp_kdf_free (src/wp_kdf_kmgmt.c), the reference-count decrement runs outside the mutex guard in the multi-threaded (#ifndef WP_SINGLE_THREADED) path:

rc = wc_LockMutex(&kdf->mutex);
cnt = --kdf->refCnt;        // runs unconditionally, even when the lock was NOT acquired
if (rc == 0) {
    wc_UnLockMutex(&kdf->mutex);
}

When wc_LockMutex fails, --kdf->refCnt is a non-atomic read-modify-write on shared state with no lock held. Two threads can race and both observe cnt == 0, leading to a double OPENSSL_free(kdf).

Fix

Return early when the lock cannot be acquired, and hold the mutex across the decrement — mirroring the already-correct wp_mac_free in src/wp_mac_kmgmt.c:

rc = wc_LockMutex(&kdf->mutex);
if (rc != 0) {
    return;
}
cnt = --kdf->refCnt;
wc_UnLockMutex(&kdf->mutex);

The single-threaded #else branch is unchanged.

Verification

Compiled the changed translation unit against a wolfSSL install + system OpenSSL headers (gcc -c src/wp_kdf_kmgmt.c ...) — EXIT=0, wp_kdf_free emitted as a defined text symbol. WP_SINGLE_THREADED is not defined by default, so the fixed branch is the live one.

Reported by static analysis (Fenrir finding 840).

Copilot AI review requested due to automatic review settings July 9, 2026 23:48

Copilot AI 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.

Pull request overview

Note

Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.

Fixes a race in wp_kdf_free where refCnt could be decremented without holding the mutex in the multi-threaded build, potentially causing double-free.

Changes:

  • Adds an early-return when wc_LockMutex(&kdf->mutex) fails.
  • Moves the refCnt decrement to occur only while the mutex is held and always unlocks afterward.

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

Comment thread src/wp_kdf_kmgmt.c
Comment on lines 128 to +133
rc = wc_LockMutex(&kdf->mutex);
cnt = --kdf->refCnt;
if (rc == 0) {
wc_UnLockMutex(&kdf->mutex);
if (rc != 0) {
return;
}
cnt = --kdf->refCnt;
wc_UnLockMutex(&kdf->mutex);
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.

3 participants