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.
There was a problem hiding this comment.
This still looks fishy. Why is typesNoImpl looking inside?
There was a problem hiding this comment.
This still looks fishy. Why is typesNoImpl looking inside?
check for: "Is the current hash algo one of those not compiled in?"
It is logic that I think was already intended to be there.
There was a problem hiding this comment.
There are three categories:
- implementation available
- implementation not compiled in
- no implementation available
Is this what you are talking about?
If so, three categories means three lists. (See #8528 test_hash.c)
There was a problem hiding this comment.
Your open PR in #8528 looks very cool & useful to test hash functions for correct results. The topic of this PR fixes only the memory leak in the function tests only the various return values. One might argue that all the tests might belong in one place.
I do not understand the difference between your (2 not available) and (3 not implemented) in our current codebase.
I do like the idea of "no implementation available": we'd define a macro that we're aware of it, but known not to be implemented. Do we do this already? Example?
Currently there are manual lists defined in wc_test_ret_t hash_test(void), copied here for reference since I cannot link to a line due to file size.
(btw- imho the list does not even belong in test.c; it should be a globally defined: a known-everywhere list. Why not use the full list in types.h? Perhaps beyond the scope of this PR)
A.) Known hash types. May or may not be implemented and/or compiled in.
enum wc_HashType typesGood[] = { WC_HASH_TYPE_MD5, WC_HASH_TYPE_SHA,
WC_HASH_TYPE_SHA224, WC_HASH_TYPE_SHA256,
WC_HASH_TYPE_SHA384, WC_HASH_TYPE_SHA512,
WC_HASH_TYPE_SHA3_224,
WC_HASH_TYPE_SHA3_256,
WC_HASH_TYPE_SHA3_384,
WC_HASH_TYPE_SHA3_512 };B.) There are two lists of "Bad hashes" also defined in the function:
enum wc_HashType typesBad[] = { WC_HASH_TYPE_NONE, WC_HASH_TYPE_MD5_SHA,
WC_HASH_TYPE_MD2, WC_HASH_TYPE_MD4 };
enum wc_HashType typesHashBad[] = { WC_HASH_TYPE_MD2, WC_HASH_TYPE_MD4,
WC_HASH_TYPE_BLAKE2B,
WC_HASH_TYPE_NONE };C.) There's a list of hashes not implemented, conditionally assembled at build time:
enum wc_HashType typesNoImpl[] = {
#ifdef NO_MD5
WC_HASH_TYPE_MD5,
#endif
#ifdef NO_SHA
WC_HASH_TYPE_SHA,
#endif
#ifndef WOLFSSL_SHA224
WC_HASH_TYPE_SHA224,
#endif
#ifdef NO_SHA256
WC_HASH_TYPE_SHA256,
#endif
#ifndef WOLFSSL_SHA384
WC_HASH_TYPE_SHA384,
#endif
#ifndef WOLFSSL_SHA512
WC_HASH_TYPE_SHA512,
#endif
#if !defined(WOLFSSL_SHA3) || defined(WOLFSSL_NOSHA3_224)
WC_HASH_TYPE_SHA3_224,
#endif
#if !defined(WOLFSSL_SHA3) || defined(WOLFSSL_NOSHA3_256)
WC_HASH_TYPE_SHA3_256,
#endif
#if !defined(WOLFSSL_SHA3) || defined(WOLFSSL_NOSHA3_384)
WC_HASH_TYPE_SHA3_384,
#endif
#if !defined(WOLFSSL_SHA3) || defined(WOLFSSL_NOSHA3_512)
WC_HASH_TYPE_SHA3_512,
#endif
WC_HASH_TYPE_NONE
};The logic currently uses the (A.) list, and at runtime determines if an item from the (A.) list is in the (C.) list (known but not compiled in). If it is, the test requires special handling to create a valid hash object, but then test with the not compiled (C.) item to ensure HASH_TYPE_E is returned.
My personal opinion: I would think that if the hash type is known not to be compiled in, why is the enum even available?
For instance, if NO_MD5 is defined, why is WC_HASH_TYPE_MD5 available and not conditionally disabled in types.h? Is there some situation where we recognize a WC_HASH_TYPE_MD5 even when it is not compiled in?
Why not just make that change to the enum wc_HashType in wolfcrypt/types.h?
I'm completely open to your advice as to how to best proceed. Thanks.
There was a problem hiding this comment.
Do not run any tests for algorithms that are disabled at build time. Please find a better way to fix this
There was a problem hiding this comment.
Got it, I'll remove references to algorithms that are disabled at build time.
In doing so, the tests will no longer check for HASH_TYPE_E return code.
There will also be no more "placeholder hash" that was needed to test HASH_TYPE_E return codes.
My plan is to remove the typesNoImpl[] and replace the typesGood[] list with only those enum values for also compiled in.
| #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.
This still looks fishy. Why is typesNoImpl looking inside?
| @@ -6036,10 +6083,15 @@ WOLFSSL_TEST_SUBROUTINE wc_test_ret_t sm3_test(void) | |||
| WOLFSSL_TEST_SUBROUTINE wc_test_ret_t hash_test(void) | |||
| { | |||
| #if defined(WOLFSSL_SMALL_STACK) && !defined(WOLFSSL_NO_MALLOC) | |||
| /* The first item in the typesGood array will be used for placeholder */ | |||
| #define PLACEHOLDER_TYPE 0 | |||
| int this_type = PLACEHOLDER_TYPE; | |||
There was a problem hiding this comment.
A "placeholder type" is arbitrarily selected value as the first item in the good (all known valid) hash list in typesGood.
When iterating through all known algos (the for i...) if one of them is determined to not be actually compiled in (the 'for j.. peek at the list in typesNoImpl) ... then some _other_ valid hash is needed to ensure theHASH_TYPE_E` can be returned. For example in this statement:
re = wc_HashUpdate(hash, SOME_TYPE_NOT_IMPLMENTED, data, sizeof(data));
where SOME_TYPE_NOT_IMPLMENTED is one of the items in typesNoImpl.
There was a problem hiding this comment.
This seems obvious right? Index 0 is always the first. Is a macro needed? Seems like noisy code
Yes, I believe so. They are all good tests, even if unlikely to be encountered. The eyebrow raising code is surrounding the hash algos that are not implemented. For instance, it is unlikely (but not impossible) that someone might try to call If you agree with the logic and scope, there's one more check that is needed that I overlooked: what if the first item in |
| @@ -6036,10 +6083,15 @@ WOLFSSL_TEST_SUBROUTINE wc_test_ret_t sm3_test(void) | |||
| WOLFSSL_TEST_SUBROUTINE wc_test_ret_t hash_test(void) | |||
| { | |||
| #if defined(WOLFSSL_SMALL_STACK) && !defined(WOLFSSL_NO_MALLOC) | |||
| /* The first item in the typesGood array will be used for placeholder */ | |||
| #define PLACEHOLDER_TYPE 0 | |||
| int this_type = PLACEHOLDER_TYPE; | |||
There was a problem hiding this comment.
This seems obvious right? Index 0 is always the first. Is a macro needed? Seems like noisy code
| #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.
Do not run any tests for algorithms that are disabled at build time. Please find a better way to fix this
I plan an update for only compiled-in algorithms and only reference the respective For future reference: the original code in question was introduced in 2016. Specifically commit It is this "
btw - I've now run thousands of iterations of the |
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