Regression testing fixes - memory allocation failure testing#10652
Regression testing fixes - memory allocation failure testing#10652SparkiDev wants to merge 1 commit into
Conversation
|
Jenkins: retest this please |
16bc473 to
6289c62
Compare
|
|
Jenkins: retest this please generic |
There was a problem hiding this comment.
Pull request overview
This PR hardens several wolfSSL/wolfCrypt code paths and API tests against memory-allocation-failure scenarios by fixing leaks, correcting ownership/freeing behavior, and adding additional test guards/initialization to prevent crashes during fault-injection regression testing.
Changes:
- Fix/adjust cleanup behavior in TLS/DTLS/TLS 1.3 ECH, OCSP, CRL handling, and EVP key loading to avoid leaks/double-frees on error paths.
- Add stricter NULL validation in ML-DSA verification helpers.
- Update multiple API tests to better tolerate injected allocation failures (guarding operations, initializing structs, and asserting expected errors).
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
wolfssl/internal.h |
Reorders CRL_Entry fields to avoid failure-path free issues in CRL duplication logic. |
src/crl.c |
Frees CRL signature-parameter buffers during CRL re-signing. |
src/dtls13.c |
Ensures DTLS 1.3 fragment buffer is freed when FSM resources are released. |
src/ocsp.c |
Avoids freeing caller-owned OCSP CERTID when decoding into an existing object. |
src/tls.c |
Refactors hybrid KeyShare allocation sequence to ensure safe cleanup/zeroization on partial alloc failures. |
src/tls13.c |
Adjusts ECH hash state assignment timing to survive failure paths. |
wolfcrypt/src/evp_pk.c |
Frees EVP_PKEY on BIO “write extra data back” error path. |
wolfcrypt/src/wc_mldsa.c |
Adds key->params NULL checks to verification helpers and VerifyMu API. |
tests/api.c |
Guards allocation-dependent negative tests and adds a WANT_READ assertion for an ECH handshake round. |
tests/api/test_dtls.c |
Initializes/guards variables to avoid use of uninitialized values under failure injection. |
tests/api/test_dtls13.c |
Adds EXPECT_SUCCESS guards to avoid unsafe operations under failure injection. |
tests/api/test_lms_xmss.c |
Zero-initializes key structs before use to prevent invalid frees on partial init. |
tests/api/test_ossl_p7p12.c |
Adjusts ownership handling in PKCS7 cert stack test under failure injection. |
tests/api/test_ossl_x509_ext.c |
Adjusts ownership handling in SAN extension test under failure injection. |
tests/api/test_ossl_x509_str.c |
Adds store cleanup attempts when EXPECT state indicates earlier failures. |
tests/api/test_pkcs7.c |
Guards DER-builder helper call under failure injection. |
tests/api/test_tls13.c |
Guards session-ticket direct manipulation under failure injection. |
tests/api/test_tls_ext.c |
Guards CH2 mismatch assertions under failure injection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 4 total — 4 posted, 0 skipped
3 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [High] CRL_Entry reorder moves sigParamsSz out of DupCRL_Entry memcpy region, breaking PSS param deep-copy —
wolfssl/internal.h:2522-2525 (manifests in src/crl.c:1355-1380, DupCRL_Entry) - [Medium] Test cleanup double-frees X509_STORE when set_cert_store took ownership but an earlier expectation failed —
tests/api/test_ossl_x509_str.c:2022-2025, 2072-2075 - [Medium] Conditional gn=NULL leaves GENERAL_NAME double-freed when push succeeds but an earlier expectation failed —
tests/api/test_ossl_x509_ext.c:629-632 - [Low] wolfSSL_X509_CRL_sign frees sigParams but leaves sigParamsSz stale —
src/crl.c:3017-3028
Review generated by Skoll
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10652
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
6289c62 to
6fbc964
Compare
crl.c, internal.h: leak of sigParams requiring reorder the struct fields to that it is above memcpy part. dtl13.c: free the DRLS fragments buffer in Dtls13FreeFsmResources in case fragment is never sent. ocsp.c: only free cid if locally allocated. tls.c: make sure ecc_kse is zeroized and can be freed. tls13.c: set hsHashesEch after init so isn't lost on failure. evp_pk.c: free key on the BIO error path Fixed various tests to not leak or crash on memory allocation failure.
6fbc964 to
b04f573
Compare
Description
crl.c, internal.h: leak of sigParams requiring reorder the struct fields to that it is above memcpy part.
dtl13.c: free the DRLS fragments buffer in Dtls13FreeFsmResources in case fragment is never sent.
ocsp.c: only free cid if locally allocated.
tls.c: make sure ecc_kse is zeroized and can be freed.
tls13.c: set hsHashesEch after init so isn't lost on failure.
evp_pk.c: free key on the BIO error path
Fixed various tests to not leak or crash on memory allocation failure.
Testing
Memory allocation failure testing.