merces / libpe

The PE library used by @merces/pev
http://pev.sf.net
GNU Lesser General Public License v3.0
115 stars 40 forks source link

Fix memory leaks in hashes.c #18

Closed boddumanohar closed 7 years ago

boddumanohar commented 7 years ago

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 commented 7 years ago

Openssl wikipage has solution for this

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

jweyrich commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

Awesome! Thank you!! 🥇 👍