memleak in secp256k1_ec_pubkey_serialize

[flgh]

version 0.1.3
In secp256k1_ec_pubkey_serialize and also secp256k1_ec_privkey_tweak_add (didn't check the others) emalloc is not freed:
$ ZEND_DONT_UNLOAD_MODULES=1 USE_ZEND_ALLOC=0 valgrind --leak-check=full --show-reachable=yes --track-origins=yes -v /usr/bin/php ./testec.php

==9623== 160 bytes in 5 blocks are definitely lost in loss record 39 of 52
==9623== at 0x4C2BBAF: malloc (vg_replace_malloc.c:299)
==9623== by 0x378D68: __zend_malloc (in /usr/bin/php7.2)
==9623== by 0x12CE72BC: zif_secp256k1_ec_privkey_tweak_add (secp256k1.c:950)
==9623== by 0x454595: execute_ex (in /usr/bin/php7.2)
==9623== by 0x456ABD: zend_execute (in /usr/bin/php7.2)
==9623== by 0x3A4A62: zend_execute_scripts (in /usr/bin/php7.2)
==9623== by 0x33E9A7: php_execute_script (in /usr/bin/php7.2)
==9623== by 0x458E8E: ??? (in /usr/bin/php7.2)
==9623== by 0x1F2FC1: ??? (in /usr/bin/php7.2)
==9623== by 0x69922E0: (below main) (libc-start.c:291)

==9623== 198 bytes in 6 blocks are definitely lost in loss record 42 of 52
==9623== at 0x4C2BBAF: malloc (vg_replace_malloc.c:299)
==9623== by 0x378D68: __zend_malloc (in /usr/bin/php7.2)
==9623== by 0x12CE7108: zif_secp256k1_ec_pubkey_serialize (secp256k1.c:909)
==9623== by 0x454595: execute_ex (in /usr/bin/php7.2)
==9623== by 0x456ABD: zend_execute (in /usr/bin/php7.2)
==9623== by 0x3A4A62: zend_execute_scripts (in /usr/bin/php7.2)
==9623== by 0x33E9A7: php_execute_script (in /usr/bin/php7.2)
==9623== by 0x458E8E: ??? (in /usr/bin/php7.2)
==9623== by 0x1F2FC1: ??? (in /usr/bin/php7.2)
==9623== by 0x69922E0: (below main) (libc-start.c:291)

==9623== LEAK SUMMARY:
==9623== definitely lost: 358 bytes in 11 blocks
==9623== indirectly lost: 0 bytes in 0 blocks
==9623== possibly lost: 0 bytes in 0 blocks
==9623== still reachable: 113,977 bytes in 348 blocks
==9623== suppressed: 0 bytes in 0 blocks
==9623==
==9623== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)
==9623== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)

diff -Nru secp256k1-php-0.1.3.orig/secp256k1/secp256k1.c secp256k1-php-0.1.3/secp256k1/secp256k1.c
--- secp256k1-php-0.1.3.orig/secp256k1/secp256k1.c      2018-02-17 15:24:05.000000000 +0100
+++ secp256k1-php-0.1.3/secp256k1/secp256k1.c   2019-02-23 16:06:00.029970460 +0100
@@ -911,6 +911,7 @@
 
     zval_dtor(zPubOut);
     ZVAL_STRINGL(zPubOut, pubkeyout, pubkeylen);
+    efree(pubkeyout);
     RETURN_LONG(result);
 }
 /* }}} */
@@ -953,6 +954,7 @@
 
     zval_dtor(zSecKey);
     ZVAL_STRINGL(zSecKey, newseckey, SECRETKEY_LENGTH);
+    efree(newseckey);
     RETURN_LONG(result);
 }
 /* }}} */

same command with patch:
==9758== LEAK SUMMARY:
==9758== definitely lost: 0 bytes in 0 blocks
==9758== indirectly lost: 0 bytes in 0 blocks
==9758== possibly lost: 0 bytes in 0 blocks
==9758== still reachable: 113,977 bytes in 348 blocks
==9758== suppressed: 0 bytes in 0 blocks
==9758==
==9758== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
==9758== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

[afk11]

Err you're quite right. I'll look into this today and run tests with valgrind during travis runs. Thanks for opening this issue!

[afk11]

Can you try with this branch? https://github.com/afk11/secp256k1-php/tree/0.1.3-valgrind I've reviewed and made some modifications.

I also experimented with PHP's test suite running valgrind, but it didn't report any detected leaks. Partly may be because I can't pass extra flags to valgrind, but could you share your script with me? here, or email me@thomaskerin.io (0xC740DE0BE7F08642)

[flgh]

I also ran with TESTS="-m" and didn't spot the leak.
Unfortunately the script calls some private php libs but I could reproduce the leak in secp256k1_ec_privkey_tweak_mul with:

$ctx = secp256k1_context_create(SECP256K1_CONTEXT_SIGN);

$key = pack("H*","1ab42cc412b618bdea3a599e3c9bae199ebf030895b039e9db1e30dafb12b727");
$twe = pack("H*","8bd3c9effcc25e5de72b329041f72a0067c1ea058b583b1b0d19308b06f189ba");
$result = secp256k1_ec_privkey_tweak_mul($ctx, $key, $twe);
echo $result . PHP_EOL;
echo unpack("H*", $key)[1] . PHP_EOL;

secp256k1_context_destroy($ctx);

and using:

po@g860:~/tests$ ZEND_DONT_UNLOAD_MODULES=1 USE_ZEND_ALLOC=0 valgrind --leak-check=full --show-reachable=yes --track-origins=yes /usr/bin/php ./testec2.php
==26130== Memcheck, a memory error detector
==26130== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==26130== Using Valgrind-3.12.0.SVN and LibVEX; rerun with -h for copyright info
==26130== Command: /usr/bin/php ./testec2.php
==26130==
1
70bdfa3c5f8c19ab21e3b8235677a9761a6dc2133411fcf5eba013d6afc8b62e
==26130==
==26130== HEAP SUMMARY:
==26130==     in use at exit: 114,009 bytes in 349 blocks
==26130==   total heap usage: 32,501 allocs, 32,152 frees, 3,879,884 bytes allocated
==26130==
...
==26130== 32 bytes in 1 blocks are definitely lost in loss record 23 of 51
==26130==    at 0x4C2BBAF: malloc (vg_replace_malloc.c:299)
==26130==    by 0x378D68: __zend_malloc (in /usr/bin/php7.2)
==26130==    by 0x12CE757C: zif_secp256k1_ec_privkey_tweak_mul (secp256k1.c:1027)
==26130==    by 0x450F4A: execute_ex (in /usr/bin/php7.2)
==26130==    by 0x456ABD: zend_execute (in /usr/bin/php7.2)
==26130==    by 0x3A4A62: zend_execute_scripts (in /usr/bin/php7.2)
==26130==    by 0x33E9A7: php_execute_script (in /usr/bin/php7.2)
==26130==    by 0x458E8E: ??? (in /usr/bin/php7.2)
==26130==    by 0x1F2FC1: ??? (in /usr/bin/php7.2)
==26130==    by 0x69922E0: (below main) (libc-start.c:291)
==26130==
...
==26130== LEAK SUMMARY:
==26130==    definitely lost: 32 bytes in 1 blocks
==26130==    indirectly lost: 0 bytes in 0 blocks
==26130==      possibly lost: 0 bytes in 0 blocks
==26130==    still reachable: 113,977 bytes in 348 blocks
==26130==         suppressed: 0 bytes in 0 blocks
==26130==
==26130== For counts of detected and suppressed errors, rerun with: -v
==26130== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

[flgh]

Just tested with the 0.1.3-valgrind branch (both the original secp256k1_ec_pubkey_serialize/secp256k1_ec_privkey_tweak_add script and the secp256k1_ec_privkey_tweak_mul script) same commandline and valgrind looks happy.

[afk11]

@flgh thanks, great to hear. I've added another commit to avoid using the heap in some situations, which might be a better habit to get into. I'll open a PR with it now, but could you test and see if things look ok?

[flgh]

My 2 tests are still working with your branch @ a5a8ed4 , so ok for me.

[afk11]

Awesome, I've released v0.1.4 and v0.2.2 with the fix! Thanks a lot for reporting this!