Skip to content
This repository has been archived by the owner on Apr 20, 2023. It is now read-only.

Fix memory leaks in hashes.c #18

Closed
boddumanohar opened this issue Aug 14, 2017 · 7 comments
Closed

Fix memory leaks in hashes.c #18

boddumanohar opened this issue Aug 14, 2017 · 7 comments
Assignees

Comments

@boddumanohar
Copy link
Contributor

Valgrind shows memory leaks at 5 places in file:

==1369== 17 bytes in 1 blocks are definitely lost in loss record 1 of 27
==1369==    at 0x4C2DBCD: malloc (vg_replace_malloc.c:299)
==1369==    by 0x586E799: strdup (strdup.c:42)
==1369==    by 0x4E503AB: get_hashes (in /usr/local/lib/libpe.so.1.0)
==1369==    by 0x4E505D2: get_headers_dos_hash (in /usr/local/lib/libpe.so.1.0)
==1369==    by 0x4E5078F: get_headers_hash (in /usr/local/lib/libpe.so.1.0)
==1369==    by 0x109106: main (in /home/boddu/github/boddumanohar/exe-check)
==1369==
==1369== 18 bytes in 1 blocks are definitely lost in loss record 2 of 27
==1369==    at 0x4C2DBCD: malloc (vg_replace_malloc.c:299)
==1369==    by 0x586E799: strdup (strdup.c:42)
==1369==    by 0x4E503AB: get_hashes (in /usr/local/lib/libpe.so.1.0)
==1369==    by 0x4E50646: get_headers_coff_hash (in /usr/local/lib/libpe.so.1.0)
==1369==    by 0x4E508BC: get_headers_hash (in /usr/local/lib/libpe.so.1.0)
==1369==    by 0x109106: main (in /home/boddu/github/boddumanohar/exe-check)
==1369==
==1369== 25 bytes in 1 blocks are definitely lost in loss record 3 of 27
==1369==    at 0x4C2DBCD: malloc (vg_replace_malloc.c:299)
==1369==    by 0x586E799: strdup (strdup.c:42)
==1369==    by 0x4E503AB: get_hashes (in /usr/local/lib/libpe.so.1.0)
==1369==    by 0x4E506E0: get_headers_optional_hash (in /usr/local/lib/libpe.so.1.0)
==1369==    by 0x4E5082D: get_headers_hash (in /usr/local/lib/libpe.so.1.0)
==1369==    by 0x109106: main (in /home/boddu/github/boddumanohar/exe-check)
==1369==
==1369== 51 bytes in 8 blocks are definitely lost in loss record 5 of 27
==1369==    at 0x4C2DBCD: malloc (vg_replace_malloc.c:299)
==1369==    by 0x586E799: strdup (strdup.c:42)
==1369==    by 0x4E503AB: get_hashes (in /usr/local/lib/libpe.so.1.0)
==1369==    by 0x4E50B17: get_sections_hash (in /usr/local/lib/libpe.so.1.0)
==1369==    by 0x1092BB: main (in /home/boddu/github/boddumanohar/exe-check)

==1369== 129,173 (24 direct, 129,149 indirect) bytes in 1 blocks are definitely lost in loss record 27 of 27
==1369==    at 0x4C2DBCD: malloc (vg_replace_malloc.c:299)
==1369==    by 0x4E511DA: imphash_load_imported_functions (in /usr/local/lib/libpe.so.1.0)
==1369==    by 0x4E517E7: pe_imphash (in /usr/local/lib/libpe.so.1.0)
==1369==    by 0x109E64: main (in /home/boddu/github/boddumanohar/exe-check)
==1369==

The first 4 errror are due to crypto library we are using.
EVP_cleanup(); which we used before exiting calc_hash will only clean the used memory partially.

And the last one is because, we are not freeing the linked list (where head variable is the head of the linked list). Each node is added after each call to imphash_load_imported_functions function.

@boddumanohar
Copy link
Contributor Author

Openssl wikipage has solution for this

https://wiki.openssl.org/index.php/Library_Initialization#Cleanup

@jweyrich
Copy link
Collaborator

Thanks! Would you send a PR to fix the head related leaks?

As for the crypto, I moved the crypto/ssl initialization and cleanup to pe_load_file_ex and pe_unload (pe.c) respectively. Running valgrind here doesn't reveal any related leaks, if I'm reading correctly.

@boddumanohar
Copy link
Contributor Author

I just checked with the lastest revision of our demo file with the latest commit libpe/master (99175e92). Valgrind still shows me the above errors.

I would love to send a PR to fix the head related leaks. Could you please assign this issue to me.

@jweyrich
Copy link
Collaborator

jweyrich commented Aug 14, 2017

I believe those leaks aren't related to the SSL cleanup though. At least I don't see any reference that points to it.

Btw, in the last 2 commits I did rename some types from hashes.h. I updated the demo as well -
https://gist.github.com/jweyrich/d631cc923ac5da78bfa2b266c889ed29/revisions#diff-f052edcff552b316ac7cd5f09b9c8000

During the week(end) I plan to do some renames related to resources too.

@boddumanohar
Copy link
Contributor Author

Yeah they all are gone now. :D
So now we have 2 memory leaks in discoveryNodesPeres and another inpe_imphash.
I am working on fixing these.

@boddumanohar
Copy link
Contributor Author

After the pull #19 the Valgrind's Memcheck output for our demo code shows:

==2351==
==2351== HEAP SUMMARY:
==2351==     in use at exit: 0 bytes in 0 blocks
==2351==   total heap usage: 9,191 allocs, 9,191 frees, 1,885,492 bytes allocated
==2351==
==2351== All heap blocks were freed -- no leaks are possible
==2351==
==2351== For counts of detected and suppressed errors, rerun with: -v
==2351== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

So now, after this, the entire demo code is free from memory leaks.

@jweyrich
Copy link
Collaborator

Awesome! Thank you!! 🥇 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants