Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new API to cleanup OpenSSL threads. #1959

Merged
merged 4 commits into from
Oct 30, 2024
Merged

Conversation

ashman-p
Copy link
Contributor

@ashman-p ashman-p commented Oct 23, 2024

  • Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
  • Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider will also need to be ready for review and merge by the time this is merged.)

'run_tests' memory leak tests fails when build options enables OQS_USE_PTHREADS and OQS_USE_OPENSSL.

valgrind tests/test_sig SPHINCS+-SHA2-256s-simple
==158054== Memcheck, a memory error detector
==158054== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==158054== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
==158054== Command: tests/test_sig SPHINCS+-SHA2-256s-simple
==158054== 
Testing signature algorithms using liboqs version 0.10.2-dev
Configuration info
==================
Target platform:  x86_64-Linux-5.4.0-126-generic
Compiler:         gcc (9.4.0)
Compile options:  [-Wa,--noexecstack;-O3;-fomit-frame-pointer;-fdata-sections;-ffunction-sections;-Wl,--gc-sections;-Wbad-function-cast]
OQS version:      0.10.2-dev
Git commit:       f47817d8cb0cd621dada5276f30f99934f3132a9
OpenSSL enabled:  Yes (OpenSSL 3.3.3.8.3.0-dev )
AES:              NI
SHA-2:            OpenSSL
SHA-3:            OpenSSL
OQS build flags:  OQS_DIST_BUILD OQS_LIBJADE_BUILD OQS_OPT_TARGET=generic CMAKE_BUILD_TYPE=Release 
CPU exts active:  AES AVX AVX2 BMI1 BMI2 PCLMULQDQ POPCNT SSE SSE2 SSE3
================================================================================
Sample computation for signature SPHINCS+-SHA2-256s-simple
================================================================================
verification passes as expected
==158054== 
==158054== HEAP SUMMARY:
==158054==     in use at exit: 240 bytes in 1 blocks
==158054==   total heap usage: 7,654 allocs, 7,653 frees, 844,712 bytes allocated
==158054== 
==158054== LEAK SUMMARY:
**==158054==    definitely lost: 240 bytes in 1 blocks**
==158054==    indirectly lost: 0 bytes in 0 blocks
==158054==      possibly lost: 0 bytes in 0 blocks
==158054==    still reachable: 0 bytes in 0 blocks
==158054==         suppressed: 0 bytes in 0 blocks
==158054== Rerun with --leak-check=full to see details of leaked memory
==158054== 
==158054== For lists of detected and suppressed errors, rerun with: -s
==158054== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Fixes #1941

Previous PR attempt is ... PR1942

@ashman-p ashman-p self-assigned this Oct 23, 2024
@ashman-p ashman-p force-pushed the na_mem_leak_fix_w_ossl branch from 57e21ad to 98bd132 Compare October 23, 2024 19:48
@ashman-p ashman-p requested a review from SWilson4 October 23, 2024 20:04
@ashman-p ashman-p marked this pull request as ready for review October 23, 2024 20:04
tests/test_kem.c Outdated Show resolved Hide resolved
tests/test_sig.c Outdated Show resolved Hide resolved
tests/test_sig_stfl.c Outdated Show resolved Hide resolved
src/common/ossl_helpers.c Outdated Show resolved Hide resolved
src/common/common.h Outdated Show resolved Hide resolved
Signed-off-by: Norman Ashley <[email protected]>
Signed-off-by: Norman Ashley <[email protected]>
src/common/common.h Outdated Show resolved Hide resolved
Copy link
Member

@SWilson4 SWilson4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added one suggestion for some additional documentation. Code-wise, this looks good to me; thanks @ashman-p for the fix.

I also confirmed that the leak no longer appears in my setup when this branch is used.

Co-authored-by: Spencer Wilson <[email protected]>
Signed-off-by: Norman Ashley <[email protected]>
@ashman-p
Copy link
Contributor Author

Please, 1 more reviewer? @cothan ?

@ashman-p ashman-p merged commit 3c8bde1 into main Oct 30, 2024
83 checks passed
@ashman-p ashman-p deleted the na_mem_leak_fix_w_ossl branch October 30, 2024 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'ninja run_tests' fails due to memory leak
3 participants