[#1823] replace malloc/calloc/strdup/free with openssl allocator #1926

songlingatpan · 2024-09-17T01:23:39Z

Replaced malloc, calloc, strdup, and free with the OpenSSL memory allocator to enable the caller to customize memory allocator, addressing issue #1823. This PR does not change the existing behavior or algorithms.

…sl allocator Signed-off-by: Songling Han <[email protected]>

songlingatpan · 2024-09-17T04:32:57Z

I will commit another change for copy_from_upstream.

Signed-off-by: Songling Han <[email protected]>

baentsch

Conceptually LGTM, but CI needs to pass; please see https://github.com/open-quantum-safe/liboqs/blob/main/CONTRIBUTING.md#coding-style

songlingatpan · 2024-09-17T07:22:19Z

@baentsch Question: 'bike', 'frodokem', 'ntruprime' is not from upstream.
To replace malloc with OQS_MEM_malloc, where should i modify the code? Thanks
liboqs/src/kem/frodokem/kem_frodokem976shake.c:11: OQS_KEM *kem = malloc(sizeof(OQS_KEM)); liboqs/src/kem/frodokem/kem_frodokem640aes.c:11: OQS_KEM *kem = malloc(sizeof(OQS_KEM)); liboqs/src/kem/frodokem/kem_frodokem1344shake.c:11: OQS_KEM *kem = malloc(sizeof(OQS_KEM)); liboqs/src/kem/frodokem/kem_frodokem976aes.c:11: OQS_KEM *kem = malloc(sizeof(OQS_KEM)); liboqs/src/kem/frodokem/kem_frodokem1344aes.c:11: OQS_KEM *kem = malloc(sizeof(OQS_KEM)); liboqs/src/kem/frodokem/kem_frodokem640shake.c:11: OQS_KEM *kem = malloc(sizeof(OQS_KEM)); liboqs/src/kem/kem.h:264:OQS_API void OQS_KEM_free(OQS_KEM *kem); liboqs/src/kem/ntruprime/kem_ntruprime_sntrup761.c:11: OQS_KEM *kem = malloc(sizeof(OQS_KEM)); liboqs/src/kem/kem.c:493:OQS_API void OQS_KEM_free(OQS_KEM *kem) { liboqs/src/kem/bike/kem_bike.c:9: OQS_KEM *kem = malloc(sizeof(OQS_KEM)); liboqs/src/kem/bike/kem_bike.c:34: OQS_KEM *kem = malloc(sizeof(OQS_KEM)); liboqs/src/kem/bike/kem_bike.c:59: OQS_KEM *kem = malloc(sizeof(OQS_KEM));

Signed-off-by: Songling Han <[email protected]>

baentsch · 2024-09-17T07:58:51Z

Question: 'bike', 'frodokem', 'ntruprime' is not from upstream.

Good point. @dstebila : There's surely upstreams for these algorithms, but they're not captured by OQS automation/copy_from_upstream. Question: Are these projects still maintained (and where) or is it OK to change code straight in liboqs for these algs? If the former, what would it take to bring them in via "copy_from_upstream"? If the latter, should we remove them completely from OQS?

…KEM, and NTRUPrime Signed-off-by: Songling Han <[email protected]>

dstebila · 2024-09-17T16:56:52Z

Question: 'bike', 'frodokem', 'ntruprime' is not from upstream.

Good point. @dstebila : There's surely upstreams for these algorithms, but they're not captured by OQS automation/copy_from_upstream. Question: Are these projects still maintained (and where) or is it OK to change code straight in liboqs for these algs? If the former, what would it take to bring them in via "copy_from_upstream"? If the latter, should we remove them completely from OQS?

FrodoKEM is maintained at https://github.com/microsoft/PQCrypto-LWEKE/; I exported the code from there any manually added it to liboqs, as we didn't have as robust a copy_from_upstream at the time. FrodoKEM also needs to do an update from upstream, as there have been new variants introduced in the last year, but I don't have a plan for this update. So to avoid blocking on that, I would say it's fine to make the FrodoKEM changes directly here in this repository.

NTRUPrime had been coming from PQClean, but they have stopped supporting it. We only are keeping one variant of NTRUPrime because of its use in OpenSSH. I think we consider a timeline for sunsetting it. But in the interim, I think changes to NTRUPrime can be done directly here in this repository.

BIKE was contributed directly by the team at AWS. Our main contact for that has been @dkostic, but I'm not sure if he's still the right contact. Pinging @brian-jarvis-aws for some input.

songlingatpan · 2024-09-17T19:53:52Z

@dstebila @baentsch @bhess @jschanck
Code change for openssl allocator has been done.
Please review the code.

In addition, there are quite some error handling and potential memory leak.
Would you like to fix them in the current PR? or create a separate PR for review?

Thanks

songlingatpan · 2024-09-17T19:56:31Z

**baentsch ** requested changes

done

dstebila · 2024-09-17T20:02:58Z

In addition, there are quite some error handling and potential memory leak. Would you like to fix them in the current PR? or create a separate PR for review?

I'd prefer to see those as a second PR, since those changes may be less mechanical and might require a closer look.

dstebila · 2024-09-17T20:07:50Z

I suggest we defer merging this until after the 0.11.0 release, otherwise we would need to cut a new release candidate and push the release back a week.

Signed-off-by: Songling Han <[email protected]>

songlingatpan · 2024-09-21T18:09:18Z

@dstebila @baentsch @bhess @jschanck
This PR is ready for review and merge.

…ator

SWilson4 · 2024-09-23T15:39:46Z

src/kem/ntruprime/pqclean_sntrup761_avx2/params.h

@@ -32,7 +32,7 @@
 #define ppad 769
 #define endingmask _mm256_set_epi8(1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,0,0,0,0,0,0,0)
 #define crypto_core_weight PQCLEAN_SNTRUP761_AVX2_crypto_core_weightsntrup761
-#define p 761
+#define p_param 761


I'm not immediately seeing the reason for this name change—could you explain why it's necessary?

There is some conflict with openssl.
In file included from /Volumes/GIT/git/oqs/liboqs/src/kem/ntruprime/pqclean_sntrup761_clean/kem.c:5: In file included from /Volumes/GIT/git/oqs/liboqs/src/common/pqclean_shims/randombytes.h:6: In file included from /Volumes/GIT/git/oqs/liboqs/build/include/oqs/rand.h:15: In file included from /Volumes/GIT/git/oqs/liboqs/build/include/oqs/common.h:30: In file included from /usr/local/opt/openssl@3/include/openssl/crypto.h:34: In file included from /usr/local/opt/openssl@3/include/openssl/safestack.h:24: /usr/local/opt/openssl@3/include/openssl/stack.h:45:60: error: expected ')' void *OPENSSL_sk_delete_ptr(OPENSSL_STACK *st, const void *p); ^ /Volumes/GIT/git/oqs/liboqs/src/kem/ntruprime/pqclean_sntrup761_clean/params.h:31:11: note: expanded from macro 'p' #define p 761 ^ /usr/local/opt/openssl@3/include/openssl/stack.h:45:28: note: to match this '(' void *OPENSSL_sk_delete_ptr(OPENSSL_STACK *st, const void *p); ^ 1 error generated. [121/1636] Building C object src/kem/frodokem/CMakeFiles/frodokem.dir/external/frodo1344aes.c.o ninja: build stopped: subcommand failed.

Hmmmm, I see. Is there a clean way to handle this with #include guards or something similar? Renaming seems like a brittle workaround. Suppose for argument's sake that a future version of OpenSSL changes the function parameter to const void *q—then everything would break here again.

What I wouldn't give for C++ namespaces... 😜

SWilson4 · 2024-09-23T15:39:57Z

Thanks for this work, @songlingatpan!

How would you feel about dropping the OQS_MEM_free function and instead using OQS_MEM_insecure_free (presumably modifying it to use OPENSSL_free)? I think it's a good idea to have a clear separation between "insecure" and "secure" frees, and we lose that if we introduce a not-explicitly-insecure free function with the same signature as OQS_MEM_insecure_free.

SWilson4 · 2024-09-23T15:42:01Z

After this lands, we may want to add some sort of static analysis to guard against raw malloc or free calls. Seems like something related to #1868.

Signed-off-by: Songling Han <[email protected]>

songlingatpan · 2024-09-23T22:52:17Z

Thanks for this work, @songlingatpan!

How would you feel about dropping the OQS_MEM_free function and instead using OQS_MEM_insecure_free (presumably modifying it to use OPENSSL_free)? I think it's a good idea to have a clear separation between "insecure" and "secure" frees, and we lose that if we introduce a not-explicitly-insecure free function with the same signature as OQS_MEM_insecure_free.

Addressed the comment in the latest PR. Please review it.

songlingatpan · 2024-09-23T22:56:28Z

After this lands, we may want to add some sort of static analysis to guard against raw malloc or free calls. Seems like something related to #1868.

Correct. We should phase out direct calls to malloc, free, calloc, strdup, and realloc.

dstebila · 2024-09-23T23:50:54Z

After this lands, we may want to add some sort of static analysis to guard against raw malloc or free calls. Seems like something related to #1868.

Correct. We should phase out direct calls to malloc, free, calloc, strdup, and realloc.

test_free in tests/test_code_conventions.py does this for free calls using an ad hoc script. It may be better to do this with CodeQL in the long run.

Signed-off-by: Songling Han <[email protected]>

songlingatpan · 2024-09-24T06:25:05Z

test_code_conventions.py

Addressed the comment by adding code in test_code_conventions.py.

Signed-off-by: Songling Han <[email protected]>

…ator

SWilson4 · 2024-09-27T19:30:14Z

src/common/common.c

 	if (ptr == NULL) {
 		fprintf(stderr, "Memory allocation failed\n");
-		abort();
+		return NULL; //abort();


Doesn't this change effectively make OQS_checked_malloc and OQS_mem_malloc the same function (up to printing)?

True.
Many embedded systems have strict memory constraints, and encountering a malloc failure is not uncommon in such environments.

src/common/common.c

SWilson4 · 2024-09-27T19:35:31Z

src/common/common.c

@@ -289,7 +292,7 @@ void *OQS_MEM_checked_aligned_alloc(size_t alignment, size_t size) {
 	void *ptr = OQS_MEM_aligned_alloc(alignment, size);
 	if (ptr == NULL) {
 		fprintf(stderr, "Memory allocation failed\n");
-		abort();
+		return NULL; //abort();


Same comment here regarding OQS_MEM_checked_aligned_alloc and OQS_MEM_aligned_alloc.

yes. Please let me know if you agree to remove the checked functions. If so, I will go ahead and remove them.

We could—but then we'll be left with a bunch of common-code functions that rely on the "checked" functions aborting if allocation fails. If this code is left unchanged, won't it lead to more crashes and/or memory corruption? If it needs to be changed to handle unchecked functions, is it not simpler to keep the "checked" functions rather than factor out the check everywhere?

The caller must perform a NULL check before dereferencing the pointer. Additionally, in the case of a NULL check failure, the caller is responsible for releasing any allocated resources to prevent memory leaks.

The caller must perform a NULL check before dereferencing the pointer. Additionally, in the case of a NULL check failure, the caller is responsible for releasing any allocated resources to prevent memory leaks.

Sure—but those NULL checks are not currently present in the codebase, because the callers were relying on the checked_malloc functions. If we replace the checked_malloc functions with the unchecked versions in this PR, the NULL checks must also be added in this PR. For example this code will dereference NULL on malloc failure.

SWilson4 · 2024-09-27T20:04:12Z

tests/test_code_conventions.py

                    continue
-                if 'IGNORE free-check' in line:
+                # Check if we're inside a multi-line comment
+                if '/*' in content[:content.find(line)] and '*/' not in content[:content.find(line)]:


Just a note, this will produce a false positive in the following scenario:

/* comment */ /* malloc(sizeof(int)); */ OQS_MEM_malloc(sizeof(int));

I think you'd have to search backwards for the closest /*, but that seems like overkill.

@SWilson4 You are right. We can improve it later.

Signed-off-by: Songling Han <[email protected]>

songlingatpan · 2024-10-01T19:11:43Z

@dstebila @baentsch @SWilson4
Is there anything else I need to address before we proceed with merging this PR?

Thanks!

SWilson4 · 2024-10-02T18:47:52Z

@dstebila @baentsch @SWilson4 Is there anything else I need to address before we proceed with merging this PR?

Thanks!

Please take a look at my comments here and here and let me know what you think.

Signed-off-by: Songling Han <[email protected]>

songlingatpan · 2024-10-03T02:24:40Z

@dstebila @baentsch @SWilson4 Is there anything else I need to address before we proceed with merging this PR?
Thanks!

Please take a look at my comments here and here and let me know what you think.

It seems the conflicts issue only occurs for certain openssl version/platform. Didn't see conflicts any more after revert.

songlingatpan · 2024-10-03T02:45:46Z

@SWilson4 Addressed your 2 comments.
Please let me know if I need to run copy_from_upstream.py or any other scripts to regenerate code before merge.
If so, what is the detail steps.

…ator

songlingatpan requested a review from dstebila as a code owner September 17, 2024 01:23

[open-quantum-safe#1823] replace malloc/calloc/strdup/free with opens…

b61754c

…sl allocator Signed-off-by: Songling Han <[email protected]>

songlingatpan force-pushed the pr_shan_1823_openssl_allocator branch from a93e625 to b61754c Compare September 17, 2024 02:10

[open-quantum-safe#1823] update memory allocator for copy_from_upstream

3dc1284

Signed-off-by: Songling Han <[email protected]>

songlingatpan requested review from bhess, jschanck and baentsch as code owners September 17, 2024 04:57

baentsch requested changes Sep 17, 2024

View reviewed changes

[open-quantum-safe#1823] format code

04a9a73

Signed-off-by: Songling Han <[email protected]>

[open-quantum-safe#1823] Use OpenSSL Memory Allocator for BIKE, Frodo…

201771c

…KEM, and NTRUPrime Signed-off-by: Songling Han <[email protected]>

dstebila approved these changes Sep 17, 2024

View reviewed changes

dstebila added this to the 0.12.0 milestone Sep 17, 2024

songlingatpan added 10 commits September 17, 2024 20:37

[open-quantum-safe#1823] Add Comments for Doxygen

a7aef7d

Signed-off-by: Songling Han <[email protected]>

sig_stfl build fix

ac5c941

Signed-off-by: Songling Han <[email protected]>

fix ptrdiff_t failure

74c4dba

Signed-off-by: Songling Han <[email protected]>

include openssl/crypto.h and resolve conflict varible for ntru

f511ba0

Signed-off-by: Songling Han <[email protected]>

Add openssl version check to fix build error

01de31f

Signed-off-by: Songling Han <[email protected]>

fix implicit conversion for diff

a955fe5

Signed-off-by: Songling Han <[email protected]>

fix build for tests

7dfc557

Signed-off-by: Songling Han <[email protected]>

Fix build for tests

30ca6bd

Signed-off-by: Songling Han <[email protected]>

Fix build for OQS_DLOPEN_OPENSSL

9951317

Signed-off-by: Songling Han <[email protected]>

Fix build failure [full tests]

51f263b

Signed-off-by: Songling Han <[email protected]>

songlingatpan requested a review from baentsch September 21, 2024 18:01

Merge branch 'open-quantum-safe:main' into pr_shan_1823_openssl_alloc…

dd1f819

…ator

SWilson4 reviewed Sep 23, 2024

View reviewed changes

remove OQS_MEM_free

b268445

Signed-off-by: Songling Han <[email protected]>

songlingatpan marked this pull request as draft September 23, 2024 21:56

remove OQS_MEM_free

c4b647e

Signed-off-by: Songling Han <[email protected]>

songlingatpan marked this pull request as ready for review September 23, 2024 22:51

songlingatpan added 2 commits September 24, 2024 03:45

Add allocator check in tests/test_code_conventions.py

f47e341

Signed-off-by: Songling Han <[email protected]>

format code

99c198a

Signed-off-by: Songling Han <[email protected]>

songlingatpan and others added 2 commits September 24, 2024 07:42

Add IGNORE memory-check

374c1d4

Signed-off-by: Songling Han <[email protected]>

Merge branch 'open-quantum-safe:main' into pr_shan_1823_openssl_alloc…

5d4b9ee

…ator

songlingatpan requested a review from crockeea as a code owner September 27, 2024 17:15

SWilson4 reviewed Sep 27, 2024

View reviewed changes

revert back to abort() in OQS_MEM_cleanse

34f9b1b

Signed-off-by: Songling Han <[email protected]>

songlingatpan added 2 commits October 3, 2024 02:03

Delect checked allocation functions

c01c376

Signed-off-by: Songling Han <[email protected]>

Revert back p_param to p for sntrup

320c6e1

Signed-off-by: Songling Han <[email protected]>

Merge branch 'open-quantum-safe:main' into pr_shan_1823_openssl_alloc…

3027acf

…ator

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1823] replace malloc/calloc/strdup/free with openssl allocator #1926

[#1823] replace malloc/calloc/strdup/free with openssl allocator #1926

songlingatpan commented Sep 17, 2024

songlingatpan commented Sep 17, 2024

baentsch left a comment

songlingatpan commented Sep 17, 2024

baentsch commented Sep 17, 2024

dstebila commented Sep 17, 2024

songlingatpan commented Sep 17, 2024

songlingatpan commented Sep 17, 2024

dstebila commented Sep 17, 2024

dstebila commented Sep 17, 2024

songlingatpan commented Sep 21, 2024

SWilson4 Sep 23, 2024

songlingatpan Sep 23, 2024

SWilson4 Sep 25, 2024

SWilson4 Sep 25, 2024

SWilson4 commented Sep 23, 2024

SWilson4 commented Sep 23, 2024

songlingatpan commented Sep 23, 2024

songlingatpan commented Sep 23, 2024

dstebila commented Sep 23, 2024

songlingatpan commented Sep 24, 2024

SWilson4 Sep 27, 2024

songlingatpan Sep 27, 2024

SWilson4 Sep 27, 2024

songlingatpan Sep 27, 2024

SWilson4 Sep 30, 2024

songlingatpan Oct 3, 2024

SWilson4 Oct 3, 2024

SWilson4 Sep 27, 2024

songlingatpan Sep 27, 2024

songlingatpan commented Oct 1, 2024

SWilson4 commented Oct 2, 2024

songlingatpan commented Oct 3, 2024

songlingatpan commented Oct 3, 2024

[#1823] replace malloc/calloc/strdup/free with openssl allocator #1926

Are you sure you want to change the base?

[#1823] replace malloc/calloc/strdup/free with openssl allocator #1926

Conversation

songlingatpan commented Sep 17, 2024

songlingatpan commented Sep 17, 2024

baentsch left a comment

Choose a reason for hiding this comment

songlingatpan commented Sep 17, 2024

baentsch commented Sep 17, 2024

dstebila commented Sep 17, 2024

songlingatpan commented Sep 17, 2024

songlingatpan commented Sep 17, 2024

dstebila commented Sep 17, 2024

dstebila commented Sep 17, 2024

songlingatpan commented Sep 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SWilson4 commented Sep 23, 2024

SWilson4 commented Sep 23, 2024

songlingatpan commented Sep 23, 2024

songlingatpan commented Sep 23, 2024

dstebila commented Sep 23, 2024

songlingatpan commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

songlingatpan commented Oct 1, 2024

SWilson4 commented Oct 2, 2024

songlingatpan commented Oct 3, 2024

songlingatpan commented Oct 3, 2024