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.

Sign up for GitHub

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

Jump to bottom

Improve Error Handling to Prevent Exit, Crashes and Memory Leaks #1929

Draft
wants to merge 40 commits into
base: main
Choose a base branch
from
Draft

Improve Error Handling to Prevent Exit, Crashes and Memory Leaks #1929

wants to merge 40 commits into from

Conversation

songlingatpan
Copy link
Contributor

This PR focuses on improving the robustness and stability of the code by enhancing error handling. The main objectives of this update include:

  • Preventing the program from exiting or crashing in case of errors
  • Preventing memory leaks by checking for NULL pointers after memory allocations and ensuring proper memory cleanup in failure cases.
  • Adding safeguards for potential memory allocation issues, ensuring the program doesn't crash or exit unexpectedly.

Note: This PR does not introduce any new functionality but aims to stabilize the existing code and make it more resilient to errors. Future test cases will be added to simulate scenarios such as out-of-memory conditions and ensure these improvements work as intended.

@songlingatpan
Copy link
Contributor Author

This PR depends on #1823. Since the changes from #1823 have not been merged yet, I have cherry-picked those commits into this PR. Once #1823 is merged, I will rebase this PR on top of the main branch to ensure a clean merge.

@baentsch
Copy link
Member

@songlingatpan Thanks for the various enhancements. I now triggered a more complete CI run. Do you expect to do further commits or is this now indeed Ready for Review from your perspective?

@songlingatpan
Copy link
Contributor Author

@songlingatpan Thanks for the various enhancements. I now triggered a more complete CI run. Do you expect to do further commits or is this now indeed Ready for Review from your perspective?

I will post more commits. Thanks

@songlingatpan
Copy link
Contributor Author

@dstebila @baentsch Could you please trigger the complete CI run again? Thanks

@songlingatpan
Copy link
Contributor Author

@baentsch CI run again? Thanks

@baentsch
Copy link
Member

I will post more commits.

Then please move this PR status to "Draft".

CI run again?

Done. Other failures. Could you please try to get CI to pass locally first so that I'm not required to manually trigger re-runs any more @songlingatpan ? If all seems to be OK, please revert to Ready for Review stage. Thanks.

@songlingatpan
[open-quantum-safe#1823] replace malloc/calloc/strdup/free with opens…
e4d0560 
…sl allocator

Signed-off-by: Songling Han <[email protected]>
@songlingatpan
[open-quantum-safe#1823] update memory allocator for copy_from_upstream
125027e 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan
[open-quantum-safe#1823] format code
0d35a14 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan
[open-quantum-safe#1823] Use OpenSSL Memory Allocator for BIKE, Frodo…
7f3fe9c 
…KEM, and NTRUPrime

Signed-off-by: Songling Han <[email protected]>
@songlingatpan
[open-quantum-safe#1823] Add Comments for Doxygen
d544770 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan
sig_stfl build fix
e177d16 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan
fix ptrdiff_t failure
d73b469 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan
include openssl/crypto.h and resolve conflict varible for ntru
9e15107 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan
Add openssl version check to fix build error
a83958b 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan
fix implicit conversion for diff
369a268 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan
fix build for tests
e24b16a 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan
Fix build for tests
fdacd10 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan
Fix build for OQS_DLOPEN_OPENSSL
0c99451 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan
Fix build failure
a90603f 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan
Error Handling
40586f8 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan
Add NULL check for AES
b8236b1 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan
remove io for better performance during failure. add log later.
fec2cdf 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan
Add NULL check for AES
5e0d03b 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan
fix OQS_OPENSSL_GUARD failure
0c2345b 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan
Fix potential memory leak for do_hash
7f6ea1b 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan
Fix potential memory leak for sha2_256_init
c6ace19 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan
Fix potential memory leak for SHA2_sha512_inc
3fa49f1 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan
format code
8ab2e63 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan
Fix NULL check for ossl_sha3.c
1089458 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan
Add NULL check for rand
2780fe9 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan
format code
2e6facb 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan
Add NULL check for common
148b11b 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan
fix build failure
9bb0db9 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan songlingatpan force-pushed the pr_shan_error_handling branch 2 times, most recently from 09cb543 to d248429 Compare September 23, 2024 00:03
@songlingatpan
Copy link
Contributor Author

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

@songlingatpan
remove OQS_MEM_free
00a9689 
Signed-off-by: Songling Han <[email protected]>
@songlingatpan
remove OQS_MEM_free
c02c48c 
Signed-off-by: Songling Han <[email protected]>
@dstebila dstebila added this to the 0.12.0 milestone Sep 23, 2024
@songlingatpan songlingatpan marked this pull request as draft October 19, 2024 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baentsch baentsch baentsch left review comments

@dstebila dstebila Awaiting requested review from dstebila dstebila is a code owner

@bhess bhess Awaiting requested review from bhess bhess is a code owner

@jschanck jschanck Awaiting requested review from jschanck

@crockeea crockeea Awaiting requested review from crockeea

@ashman-p ashman-p Awaiting requested review from ashman-p ashman-p will be requested when the pull request is marked ready for review ashman-p is a code owner

@cothan cothan Awaiting requested review from cothan cothan will be requested when the pull request is marked ready for review cothan is a code owner

@alexrow alexrow Awaiting requested review from alexrow alexrow will be requested when the pull request is marked ready for review alexrow is a code owner

@brian-jarvis-aws brian-jarvis-aws Awaiting requested review from brian-jarvis-aws brian-jarvis-aws will be requested when the pull request is marked ready for review brian-jarvis-aws is a code owner

@Martyrshot Martyrshot Awaiting requested review from Martyrshot Martyrshot will be requested when the pull request is marked ready for review Martyrshot is a code owner

@praveksharma praveksharma Awaiting requested review from praveksharma praveksharma will be requested when the pull request is marked ready for review praveksharma is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.12.0
Development

Successfully merging this pull request may close these issues.
5 participants
@songlingatpan @baentsch @dstebila @praveksharma @SWilson4