Fixes for static memory builds with WOLFSSL_NO_MALLOC - #11432
Conversation
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11432
Scan targets checked: none
Failed targets: wolfcrypt-src, wolfcrypt-bugs
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11432
Error: CalledProcessError
6c11cd5 to
5a03400
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11432
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
5a03400 to
667312f
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11432
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
667312f to
8b7f6a8
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11432
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
|
Jenkins retest this please |
8b7f6a8 to
e364887
Compare
|
Jenkins retest this please |
|
|
||
| for (i = 0; i < WOLFMEM_MAX_BUCKETS; i++) { | ||
| if (heap->ava[i] != NULL) { | ||
| (void)wolfSSL_SetGlobalHeapHint(hint); |
There was a problem hiding this comment.
I'm not a fan of the automatic setting and unsetting of the global heap hint. The setting of the global heap hint itself is not mutex protected so there could be obscure edge race cases here on which heap hint gets set. It's meant for an application to manage setting the global heap hint if wanted otherwise the build should fail on malloc when not set and no heap hint has been used.
There was a problem hiding this comment.
I do like the WC_ASN_NO_HEAP fix in asn.c in this PR though!
There was a problem hiding this comment.
Fair enough - dropped it. wolfcrypt/src/memory.c is back to master; the applications set the hint now.
One thing: the server example couldn't set it at all. wolfSSL_CTX_load_static_memory(&ctx, method, ...) with a NULL ctx loads the pool and creates the CTX in one call, so wolfSSL_Init() runs before the caller ever sees the hint. Restructured it like the client. Any app using that form hits the same wall.
Only the standalone examples claim it, and only when unset - testsuite and unit.test run the server on a thread beside the client with function-local pools, so there the wolfCrypt test's long-lived pool is the right holder.
Also pinned the WC_ASN_NO_HEAP fix with a cert_no_malloc_test() assertion on the copy-out side, and moved the MC/DC white-box guard for that block off WOLFSSL_NO_MALLOC.
--enable-staticmemory --enable-opensslextra -DWOLFSSL_NO_MALLOC goes from 0/5 to 3 pass / 2 fail, the two remaining failing identically without these changes. Plain --enable-staticmemory stays 5/5.
ParseCert() guarded its RSA public-key copy with !WOLFSSL_NO_MALLOC, while StoreKey() guards the equivalent copy for every non-RSA key with !WC_ASN_NO_HEAP. Those are not the same condition: WC_ASN_NO_HEAP is auto-defined only when WOLFSSL_NO_MALLOC and NO_WOLFSSL_MEMORY are set without XMALLOC_USER or WOLFSSL_STATIC_MEMORY, so a static-memory build defines WOLFSSL_NO_MALLOC yet still has a working allocator. In such a build the copy was skipped, cert->pubKeyStored stayed 0, and FillSigner() therefore never populated signer->publicKey/pubKeySize. ParseCertRelative() then passed a NULL key and a zero key size to ConfirmSignature(), which rejects them with BAD_FUNC_ARG before its WOLFSSL_ENTER. The effect was that no certificate issued by an RSA CA could be verified against it - wolfSSL_CertManagerVerifyBuffer() and TLS peer validation alike - while ECC, Ed25519, Ed448 and ML-DSA CAs worked, because those keys travel through StoreKey(). Use WC_ASN_NO_HEAP in all three guards, including the one on the ptr declaration. FreeDecodedCert() and FreeSigner() already key off pubKeyStored, so ownership and freeing are unchanged. Point the MC/DC white-box guard for this block at WC_ASN_NO_HEAP too. It still keyed off WOLFSSL_NO_MALLOC, so in a static-memory build the copy is now compiled and executed while the section covering it fell back to its stub, and the coverage claim was inaccurate for the one configuration this fixes.
With WOLFSSL_NO_MALLOC there is no system heap to fall back on, so an allocation the compatibility layer makes with a NULL heap has to come out of the static pool the test loads. The test only nominated that pool as the global heap hint when OPENSSL_EXTRA was off, so in the combination of the two those allocations had no source at all and openssl_test() failed. Set the hint in that combination too, and give it a 1 MB pool. gTestMemory was sized for the wolfCrypt tests alone and the compatibility layer's allocations exhausted it - first in wolfSSL_CRYPTO_malloc(), then, as the pool was enlarged, further along in wolfSSL_X509_load_certificate_file(). Every other configuration keeps the size it had. The matching teardown stays restricted to !OPENSSL_EXTRA on purpose. gTestMemory has static storage duration, so the hint cannot dangle, and testsuite runs the echo server after wolfcrypt_test() returns: the echo server has no pool of its own, and this is the one that outlives it. testsuite/testsuite.test now passes with --enable-staticmemory -DWOLFSSL_NO_MALLOC. scripts/unit.test still fails there, at seven API tests this change does not address. Exclude BENCH_EMBEDDED from the new arm so a target that has declared itself embedded is not silently handed a 1 MB array; WOLFSSL_STATIC_MEMORY_TEST_SZ remains the override. Pin the behaviour the asn.c fix restores while it is here: cert_no_malloc_test() only asserted the in-place layout under WC_ASN_NO_HEAP, so nothing covered the copy-out side. Add the mirror assertion - pubKeyStored set, publicKey outside the source DER. It fails on the pre-fix guard and passes after it, and unlike fill_signer_twice_test() it is not gated on NO_FILESYSTEM, which a real static-memory target turns off.
With WOLFSSL_NO_MALLOC there is no allocator behind a NULL-heap XMALLOC, so an allocation made outside any CTX or SSL object can only be served from the global heap hint. wolfSSL_Init() makes such an allocation: under OPENSSL_EXTRA it seeds the compatibility-layer RNG, whose _InitRng() call allocates with a NULL heap. That returned NULL, wolfSSL_Init() reported WC_INIT_E, and every wolfSSL_CTX_new_ex() that triggered the lazy init failed, so neither example could establish a connection even though the pool it had loaded was large enough. Have each example nominate its own pool, which is what the hint is for. The server could not do that before: it created its CTX through wolfSSL_CTX_load_static_memory(), which loads the pool and creates the CTX in one call, so wolfSSL_Init() ran before the caller ever saw the hint. Load the pool with wc_LoadStaticMemory() and create the CTX afterwards, as the client already does. Claim the hint only when none is set, and drop it again on the way out. Both pools are local to the example's own function, and testsuite runs the wolfCrypt test, both examples and the echo server in one process, so an example that overwrote the hint would leave it pointing at a pool that dies the moment the example returns. scripts/resume.test, scripts/tls13.test and testsuite/testsuite.test go from failing to passing with --enable-staticmemory -DWOLFSSL_NO_MALLOC. Restrict the claim to the standalone programs (!NO_MAIN_DRIVER). testsuite and unit.test compile both examples with NO_MAIN_DRIVER and run server_test on a spawned thread beside client_test, so an in-harness claim would publish one thread's automatic-storage pool as the process allocator, let the other thread allocate from it, and then revoke it when the owning frame unwound. In those builds the harness's own long-lived pool is the one that belongs in the hint. Check ctx after wolfSSL_CTX_new_ex() rather than letting the following IO-pool load report a CTX allocation failure as "unable to load static memory".
e364887 to
cc50474
Compare
4 participants
WOLFSSL_NO_MALLOCremoves only the stdio malloc fall-back, so a build that also has--enable-staticmemorystill has a working allocator. Two places treat that configuration as having no allocator at all: one makes every RSA CA useless for chain verification, the other stops any TLS connection being established. They are independent and sit in different files. A third commit sizes the wolfCrypt test pool somake checkexercises the configuration.Found while running wolfCert's CI matrix, which builds it.
1. RSA public key never reaches the CA Signer
ParseCert()guarded its RSA public-key copy with!defined(WOLFSSL_NO_MALLOC), whileStoreKey()guards the equivalent copy for every non-RSA key with!defined(WC_ASN_NO_HEAP). Only the latter means "genuinely no allocator", which is what the copy depends on, so a static memory build skipped it despite having somewhere to copy to.cert->pubKeyStoredtherefore stayed 0,FillSigner()never populatedsigner->publicKey/pubKeySize, andConfirmSignature()was handed a NULL key and zero size, which it rejects withBAD_FUNC_ARG. No certificate issued by an RSA CA could be verified against it, throughwolfSSL_CertManagerVerifyBuffer()or TLS peer validation alike. ECC, Ed25519, Ed448 and ML-DSA CAs were unaffected, because those keys travel throughStoreKey().The fix uses
WC_ASN_NO_HEAPin all three guards inParseCert(), including the one on thechar* ptrdeclaration.FreeDecodedCert()andFreeSigner()already key offpubKeyStored, so ownership is unchanged.2. Loading a static pool never set the global heap hint
An allocation made outside any
WOLFSSL_CTXorWOLFSSLobject can only be served from the global heap hint, and loading a pool never set it.wolfSSL_Init()makes exactly such an allocation: underOPENSSL_EXTRAit seeds the compatibility layer RNG with a NULL heap. That returned NULL,wolfSSL_Init()reportedWC_INIT_E, and everywolfSSL_CTX_new_ex()that triggered the lazy init failed, so no TLS connection could be established at all regardless of pool size.The two bundled examples reach this from different entry points, the server through
wolfSSL_CTX_load_static_memory()and the client through a directwc_LoadStaticMemory(), so the fix goes inwc_LoadStaticMemory_ex(), which both funnel through. It applies only when no hint has been set yet, so an application managing the hint itself keeps control.Pools loaded with
WOLFMEM_IO_POOLorWOLFMEM_IO_POOL_FIXEDare skipped.wc_partition_static_memory()puts every chunk of such a pool on the heap's io list and leaves the general buckets empty, so adopting one would give a global allocator that fails every general request, and would then stop a general pool loaded afterwards from taking the hint. Load order does not matter.wc_UnloadStaticMemory()now clears the hint when it refers to the heap being unloaded. The hint lives inside the caller's pool buffer, which the caller may reuse once the pool is gone, and the mutex it reaches through has just been destroyed. That part is not gated onWOLFSSL_NO_MALLOC: an application setting the global hint by hand has always been able to leave it dangling the same way.Reviewers may recall 98a19f9, which removed commented-out
wolfSSL_SetGlobalHeapHint()calls from both examples because the pool "does not handle all memory used on default build". That concern does not apply here: the change is confined toWOLFSSL_NO_MALLOC, where the alternative to serving those allocations from the pool is failing them outright. Builds with a system heap are unaffected.3. wolfCrypt test pool sized for the configuration
Once the pool serves the compatibility layer's NULL-heap allocations,
gTestMemorywas too small andopenssl_test()failed on exhaustion. TheWOLFSSL_NO_MALLOCandOPENSSL_EXTRAcombination now gets 1 MB; every other configuration keeps the size it had.Testing
With
--enable-staticmemory -DWOLFSSL_NO_MALLOC,make checkgoes from 0 of 5 passing to 3 of 5:resume.test,tls13.testandtestsuiteall move from fail to pass. A standalonewolfSSL_CertManagerVerifyBuffer()on an RSA CA in that build goes from -173 toWOLFSSL_SUCCESS, and wolfCert's integration suite passes against it with its local workaround for defect 1 removed.--enable-staticmemorywithoutWOLFSSL_NO_MALLOC- the configuration the unload change newly touches - passesmake check5 of 5. The default configuration passesmake checkandtestwolfcrypt, and a true no-allocator build (-DWOLFSSL_NO_MALLOC -DNO_WOLFSSL_MEMORY) compiles with behaviour unchanged by design. Both predicates in 1 and 2 are unchanged for those two cases, so onlyWOLFSSL_NO_MALLOCbuilds that do have an allocator change behaviour.Two failures deliberately left in place
unit.testreaches a structural limit rather than a sizing one:test_wc_ChaCha20Poly1305_Encrypt_Decrypt_Big()asks for a single 16384 byte buffer whileLARGEST_MEM_BUCKETis 16128 for this feature set. A static pool is partitioned into fixed size buckets, so no pool size satisfies that request, and raising the ceiling changes the layout for every static memory user.pem.testfails becauseexamples/pem/pem.cnever loads a pool at all, so itsXMALLOC(..., NULL, ...)has no source in a build with no system heap.Neither is a regression.