fix: guard RSA refCnt decrement with held lock#435
Open
MarkAtwood wants to merge 1 commit into
Open
Conversation
There was a problem hiding this comment.
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 potential double-free race in wp_rsa_free() by ensuring the RSA reference count decrement occurs only while the mutex is successfully held, matching the locking pattern used by wp_rsa_up_ref().
Changes:
- Initialize
cntto a non-zero default to avoid freeing on mutex lock failure (leak-safe fallback). - Move
--rsa->refCntinside therc == 0(lock acquired) branch. - Preserve unlock behavior only for the successful lock path.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
In
wp_rsa_free()(src/wp_rsa_kmgmt.c) the reference-count decrementcnt = --rsa->refCnt;runs unconditionally, outside theif (rc == 0)mutex-lock-success guard. Ifwc_LockMutex()fails, two threads can race the unlocked decrement, both observecnt == 0, and double-free the object.This is an asymmetry with the sibling
wp_rsa_up_ref()in the same file, which correctly guards its increment behind a successfully-held lock.Fix
Move the decrement inside the lock-success branch and default
cnt = 1. On the rare lock failure the object is leaked rather than double-freed (leak-safe), and in the normal path the decrement is atomic with the held lock.Verification
--enable-singlethreadeddefaults to no, soWP_SINGLE_THREADEDis undefined):gcc -c src/wp_rsa_kmgmt.c-> exit 0, no errors.if (rc == 0).Reported by static analysis (Fenrir finding 515). Severity is low: wolfSSL's pthread
wc_LockMutexessentially never fails for a valid normal-type mutex, so this is a latent defect.