Fix multiple reported issues#10965
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple reported issues (zd#22133) by tightening memory-management around X509 decoding and improving safety in shared-context initialization and key allocation logic.
Changes:
- Prevent orphaned allocations when re-decoding certificates into existing
WOLFSSL_X509objects. - Add CTX-level locking around lazy
ctx->suitesinitialization to avoid races/leaks in multi-threadedwolfSSL_new()usage. - Zero newly allocated key structures in
AllocKey()to make partial-init failure paths safe to free.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/ssl.c | Frees existing X509 contents before reinitializing/decoding to avoid orphaned allocations. |
| src/internal.c | Guards lazy ctx->suites init with CTX mutex and zeroes AllocKey() allocations pre-init. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
Retest this please |
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 3 total — 1 posted, 2 skipped
Posted findings
- [Medium] suites race fix not applied to the parallel lazy-alloc path in wolfSSL_set_SSL_CTX —
src/internal.c:8274-8299
Skipped findings
- [Medium] No regression test added for the concurrent suites-init and peerCert re-decode fixes
- [Low] Redundant FreeX509/InitX509 in wolfSSL_get_peer_certificate now that DecodeToX509 frees internally
Review generated by Skoll via Claude/Codex
|
Jenkins retest this please |
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 6 total — 3 posted, 3 skipped
Posted findings
- [Medium] Lazy ctx->suites init in wolfSSL_set_SSL_CTX left unguarded, defeating the mutex added to InitSSL —
src/ssl.c:8486-8491 - [Medium] CTX refcount leaked when suites allocation fails in wolfSSL_set_SSL_CTX —
src/ssl.c:8475-8491 - [Low] Redundant FreeX509/InitX509 now duplicated inside DecodeToX509 —
src/ssl_api_cert.c:2765-2766
Skipped findings
- [Medium] CTX reference leaked when AllocateCtxSuites fails after reorder
- [Medium] Lazy suites init in wolfSSL_set_SSL_CTX left unguarded while InitSSL now locks
- [Low] Suites-init race the PR fixes in InitSSL remains open in wolfSSL_set_SSL_CTX
Review generated by Skoll via Claude/Codex
…olfSSL_get_peer_certificate.
…on failure and free.
Description
Fixes zd#22133
Testing
Built in tests, provided reproducers
Checklist