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

Convert sa to ptr and memory leakage fix. #184

Closed
wants to merge 3 commits into from

Conversation

kibnakamoto
Copy link

When I compiled using make, I got the following error:

[  1%] Building C object src/CMakeFiles/Crypto.dir/src_main/sadb_routine_inmemory.template.c.o
/home/kibnakamoto/workspace/NASA/CryptoLib/src/src_main/sadb_routine_inmemory.template.c: In function ‘sadb_get_sa_from_spi’:
/home/kibnakamoto/workspace/NASA/CryptoLib/src/src_main/sadb_routine_inmemory.template.c:384:12: error: the comparison will always evaluate as ‘false’ for the address of ‘sa’ will never be NULL [-Werror=address]
  384 |     if (sa == NULL)
      |            ^~
/home/kibnakamoto/workspace/NASA/CryptoLib/src/src_main/sadb_routine_inmemory.template.c:41:30: note: ‘sa’ declared here
   41 | static SecurityAssociation_t sa[NUM_SA];
      |                              ^~
/home/kibnakamoto/workspace/NASA/CryptoLib/src/src_main/sadb_routine_inmemory.template.c: In function ‘sadb_get_operational_sa_from_gvcid’:
/home/kibnakamoto/workspace/NASA/CryptoLib/src/src_main/sadb_routine_inmemory.template.c:419:12: error: the comparison will always evaluate as ‘false’ for the address of ‘sa’ will never be NULL [-Werror=address]
  419 |     if (sa == NULL)
      |            ^~
/home/kibnakamoto/workspace/NASA/CryptoLib/src/src_main/sadb_routine_inmemory.template.c:41:30: note: ‘sa’ declared here
   41 | static SecurityAssociation_t sa[NUM_SA];
      |                              ^~

To fix it, I changed the sa array into a pointer initialized with calloc, I freed it in the sadb_close() function.

This solves it because if sa is an array, comparing to NULL always returns false because array != NULL. So I converted the sa to pointer.

The second fix is:
I ran valgrind and saw that there was 14 bytes of memory definitely lost. This was caused by sa[12].arsn definition which was completely wrong to begin with.

Line 311:

sa[12].arsn = (uint8_t*) calloc(1, sa[1].arsn_len * sizeof(uint8_t)); 

To my understanding there are a few problems with this line, sa[1].arsn_len = 2, while sa[12].arsn_len = 0
I think this line was meant to be

sa[12].arsn = (uint8_t*) calloc(1, sa[12].arsn_len * sizeof(uint8_t)); 

but either way, this line causes a memory leakage that can be solved by not setting it to anything (so that sa[12].arsn = NULL).

Therefore, considering that sa[12].arsn_len = 0, then sa[12].arsn = NULL. This was correctly applied to sa[8] but not here.

Running tests...
Test project /home/kibnakamoto/workspace/NASA/CryptoLib
    Start 1: UT_TC_APPLY
1/8 Test #1: UT_TC_APPLY ......................   Passed    0.59 sec
    Start 2: UT_TC_PROCESS
2/8 Test #2: UT_TC_PROCESS ....................   Passed    0.32 sec
    Start 3: UT_CRYPTO_CONFIG
3/8 Test #3: UT_CRYPTO_CONFIG .................   Passed    0.00 sec
    Start 4: UT_CRYPTO
4/8 Test #4: UT_CRYPTO ........................   Passed    0.13 sec
    Start 5: UT_CRYPTO_AOS
5/8 Test #5: UT_CRYPTO_AOS ....................   Passed    0.00 sec
    Start 6: UT_CRYPTO_MC
6/8 Test #6: UT_CRYPTO_MC .....................   Passed    0.04 sec
    Start 7: UT_TM_APPLY
7/8 Test #7: UT_TM_APPLY ......................   Passed    0.28 sec
    Start 8: UT_TM_PROCESS
8/8 Test #8: UT_TM_PROCESS ....................   Passed    0.27 sec

100% tests passed, 0 tests failed out of 8

Total Test time (real) =   1.64 sec

All tests passed.

@kibnakamoto
Copy link
Author

The memory leak fix solved around 30k byte leak.

@rjbrown2
Copy link
Contributor

rjbrown2 commented Oct 13, 2023

This may be a larger problem than with just this instance. The recent change to the SA from pointer to array could be causing false positives in unit tests as well. @jlucas9

@dccutrig
Copy link
Contributor

Let's discuss tomorrow 10/19

@kibnakamoto
Copy link
Author

@dccutrig
That works.
Thank you.

@rjbrown2 rjbrown2 mentioned this pull request Dec 13, 2023
@rjbrown2 rjbrown2 closed this Dec 13, 2023
@rjbrown2
Copy link
Contributor

Moved to an issue:
#211
Until licensing can occur, or this is fixed. It cannot currently be remedied through a PR with an external contributor.

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.

None yet

3 participants