Fix hash_test() memory leak in wolfcrypt/test/test.c #8506
Conversation
|
Jenkins retest this please |
|
Jenkins retest this please. to retry external tests |
|
Retest this please: "ERROR: Cannot delete workspace :Unable to delete" |
| #if defined(WOLFSSL_SMALL_STACK) && !defined(WOLFSSL_NO_MALLOC) | ||
| this_type = i; | ||
| #endif | ||
| for(j = 0; j < (int)(sizeof(typesNoImpl) / sizeof(*typesNoImpl)); j++) { |
There was a problem hiding this comment.
We don't want to test if the algorithm isn't supported.
No point in retesting the first algorithm over and over again.
There was a problem hiding this comment.
Hi @SparkiDev thanks for taking a look at this.
This isn't a matter of unsupported algorithm, nor testing the same algo repeatedly.
We're also not testing the type of the placeholder hash assigned with this_hash. We just need any non-null value.
The tesdt.c loop on line 6238 iterates though all known good hash types from the typesGood list., testing each one time. However, it may be the case that 1 or more of those may not be implemented and/or (more likely) not compiled in. That list is assembled in typesNoImpl; see line 6111.
The tricky thing in this section is that we are looking for a "expected return value" in exp_ret to be HASH_TYPE_E.
The interesting thing here though, is that in order to do that testing, the hash object parameter created needs to have some valid type, otherwise for example the wc_HashUpdate falls through the case statement and ends up instead returning BAD_FUNC_ARG.
It's admittedly a bit eyebrow raising: as the hash type assigned for these tests is arbitrary (and fixed). But this is just so the subsequent wc_HashInit, wc_HashUpdate, wc_HashFinal for the actual hash being tested returns the expected error codes, instead of complaining about a bad hash object.
Otherwise without that placeholder valid hash type, the wc_HashNew won't create a hash object to test with if it is one of the typesNoImpl items.
It is a good test to ensure that all of the known-good, but not-compiled-in algorithms properly return the HASH_TYPE_E value, and we should be going through the entire list for all the functions.
Perhaps the change here is to have the code better documented?
What do you think of a comment just before the 'for j = 0 ... to be something like:
Ensure we create a valid object for all known hash algorithms, including those that may not be compiled in..
Functions not compiled in are expected to return HASH_TYPE_E and need a value hash object to test with.
Description
This PR fixes a memory leak in the
hash_testfunction ofwolfcrypt/test/test.cand adds some Espressif-specific heap checking features.Background
The memory leak was first brought to my attention by the Devin automated #8471.
I've reviewed the Espressif code. I was unable to find a memory leak.
I also had ChatGPT review the code. There were some false positives, but after I explained the multi-threaded nature of the library, ChatGPT was also unable to find any memory leak candidates.
From what I can see, there is no memory leak detected in any of the core wolfcrypt hash functions, only the test code around current line 6082 (no line as the file is too big to view online):
The problem is manifested when
#define WOLFSSL_SMALL_STACKis found in theuser_settings.h.Generally the test has been used as a "run once and exit" app, so it is unlikely anyone noticed there was a memory leak.
In my case, I typically run the tests in a loop for hours and days on embedded devices. See the #define TEST_LOOP setting.
After about 1,500+ loops, the ESP32 devices would run out of memory. I incorrectly assumed the problem was Espressif-specific.
I've run the benchmark in loops on 9 different devices for days now. Between ~5,000 and ~8,000 loops have completed successfully:
I've started a similar loop of the wolfcrypt test again with changes in this PR.
Changes
There's a new
DEBUG_WOLFSSL_ESP32_HEAPnow documented inesp32-crypt.h.The
PRINT_HEAP_CHECKPOINT(b, i)has been modified to accept two parameters: a breadcrumb string (b) and an integer index (i).The existing implementation of
PRINT_HEAP_CHECKPOINTwas not otherwise changed. A newPRINT_HEAP_CHECKPOINTwas added for the Espressif ESP-IDF environment.There's a new
PRINT_HEAP_ADDRESSthat prints the address of a newly allocated memory segment. In this PR, the address of the hash object.There's a new "placeholder" hash type created for known-but-not-implemented hash types. Previously the hash
freewould quietly fail due to type mismatch, contributing to the memory leak.The default placeholder type is introduced in a new macro:
PLACEHOLDER_TYPE. An arbitirary value of0is used; the first validtypesGood[]item is used when checking bad parameters.More return codes are checked (e.g. on
wc_HashFree) that were not previously checked.The logic for "is this a valid hash?" when checking the
typesNoImpllist has been revised. It is no longer sensitive to the order of items. It was also probably not fully working as intended anyhow.New hash objects are created and destroyed as appropriate in the "Good type and implemented" tests.
Still outstanding
There's still another
test.cmemory leak in or around the PKCS12 test. See message from full log, below:I will look at this one at a later date unless otherwise advised.
It would be nice to have the hash checks for other platforms to give similar warnings. For now the feature is Espressif-specific.
Full ESP32 Output
For reference:
Fixes zd# n/a
Testing
How did you test?
ESP32 with and without HW encryption, with and without WOLFSSL_SMALL_STACK on ESP-IDF v5.2.
./autogen.sh ./configure --enable-smallstack --enable-all make clean make -j$(nproc) ./wolfcrypt/test/testwolfcryptAnd
./autogen.sh ./configure --enable-all make clean make -j$(nproc) ./wolfcrypt/test/testwolfcryptUpdate:
Tests have been running on my 9-device jig since yesterday. Screen snip of memory shows no memory leak for the default tests running on the 8x ESP32 + ESP8266
Checklist