Fix GH-10755: Memory leak in phar_rename_archive()

[stkeke]

In phar_renmae_archive() context, added one reference but immediately
destroyed another, so do not need to increase refcount. With removal of
refcount++ line, PHP/Zend no longer reports memory leak.
Updated bug69958.phpt test file accordingly.

Testing (PASS on both Debug and Release build)
Debug: ./configure --enable-debug
Release: ./configure

1) Running selected tests.
PASS Phar: bug #69958: Segfault in Phar::convertToData on invalid file [bug69958.phpt]

2) All tests under ext/phar/tests PASSED except skipped.
=====================================================================
Number of tests :   530               515
Tests skipped   :    15 (  2.8%) --------
Tests warned    :     0 (  0.0%) (  0.0%)
Tests failed    :     0 (  0.0%) (  0.0%)
Tests passed    :   515 ( 97.2%) (100.0%)
---------------------------------------------------------------------
Time taken      :    26 seconds
=====================================================================

[iluuu1994]

ASAN reveals an incorrect free here for phar->alias. Duplicating apparently causes leaks in other places. I don't really know the solution. Do you want to take a closer look or should I?

[stkeke]

@iluuu1994 If you can take a closer look, that would be wonderful.
I am not able to reproduce the ASAN failure on my local machine with --enable-address-sanitizer, suggested by @nielsdos from #10848. In the meantime, I can only do some further code reading and understand if my simple fix trigger it.

[iluuu1994]

@stkeke Did you also pass the --asan flag when running tests? That will actually set the USE_ZEND_ALLOC=0 environment variable.

[stkeke]

@iluuu1994 I now can reproduce and debug this issue with --asan flag. Thanks for instructing.
Still, if you have some bandwidth, please help to take a look or give some hint. It would surely be quicker with four hands than two.

[stkeke]

This is what I got today... @nielsdos told me pemalloc/pefree should be paired.
Here I saw from phar module, the pair is pestrndup()/pefree(). Anything wrong?

1. At ext/phar/tar.c:692, myphar->alias is allocated via pestrndup() function.
   myphar->alias = pestrndup(myphar->fname, fname_len, myphar->is_persistent);

stack trace:
#1 0x55acf3fdb702 in __zend_malloc /home/tony/php-src/Zend/zend_alloc.c:3113
#2 0x55acf3fd8c0f in _malloc_custom /home/tony/php-src/Zend/zend_alloc.c:2471
#3 0x55acf3fd8e7c in _emalloc /home/tony/php-src/Zend/zend_alloc.c:2590
#4 0x55acf3fd945e in _estrndup /home/tony/php-src/Zend/zend_alloc.c:2688
#5 0x55acf3a89111 in phar_parse_tarfile /home/tony/php-src/ext/phar/tar.c:692

2. At ext/phar/phar.c:193, phar->alias is released.
   pefree(phar->alias, phar->is_persistent);

stack trace:
#0 0x7f2339b16517 in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:127
#1 0x55acf3fd8cf9 in _efree_custom /home/tony/php-src/Zend/zend_alloc.c:2480
#2 0x55acf3fd8f2b in _efree /home/tony/php-src/Zend/zend_alloc.c:2600
#3 0x55acf3acd021 in phar_destroy_phar_data /home/tony/php-src/ext/phar/phar.c:193

[iluuu1994]

@stkeke

diff --git a/ext/phar/phar_object.c b/ext/phar/phar_object.c
index ba47cd8843..fbf454e499 100644
--- a/ext/phar/phar_object.c
+++ b/ext/phar/phar_object.c
@@ -2113,10 +2113,12 @@ static zend_object *phar_rename_archive(phar_archive_data **sphar, char *ext) /*
 				pphar->flags = phar->flags;
 				pphar->fp = phar->fp;
 				phar->fp = NULL;
+				phar->alias = NULL;
 				phar_destroy_phar_data(phar);
 				*sphar = NULL;
 				phar = pphar;
-				phar->refcount++;
+				/* FIX: GH-10755 Memory leak in phar_rename_archive() */
+				/* phar->refcount++; */
 				newpath = oldpath;
 				goto its_ok;
 			}

I think this might do it. Setting alias to NULL before passing it to phar_destroy_phar_data avoids the double free.

[stkeke]

@iluuu1994 Thanks for the hint. Let me check and test it tomorrow.

[stkeke]

@iluuu1994 I still need one more day to understand the code logic
+ phar->alias = NULL;
This does fix the double-free caught by ASAN check.
ASAN no longer reports double-free and zend also does not report memory leak.

Only one thing I am not sure, someone (include me) might have a concern that
if phar->alias is previously pointing to (char*) a string before we set it to NULL,
there might be memory leak of a string if no other guy points to and release it later?

I am wondering, could we do?

 				pphar->fp = phar->fp;
 				phar->fp = NULL;
+                             pphar->alias = phar->alias; /* transfer alias to pphar just as fp */
+				phar->alias = NULL; /* fix double-free issue caught by ASAN */

[iluuu1994]

@stkeke Of course, sorry, that line was missing in my example. I really don't know how this code works (I've looked at it only superficially) but from looking at how the other properties are handled here that seems to make sense.

[stkeke]

@iluuu1994 I traced the alias lifetime and found the root cause of double-free.
If we don't set alias to NULL, then it will be free'd first time in phar_rename_archive(), and second time in phar object destructor during php_request_shutdown() call stack.

So your fixation is quite correct. Let's use it to avoid the first-time release and let it go in destructor.

How alias double-free issue happened

1. alias free'd first time call stack (from main() to latest phar_rename_archive())

1  cli/php_cli.c:1367  main()
2    cli/php_cli.c:965  do_cli()
3      main/main.c:2542  php_execute_script()
4        Zend/zend.c:1845  zend_execute_scripts()
5          Zend/zend_vm_execute.h:60151  zend_execute()
6            Zend/zend_vm_execute.h:55811  execute_ex()
7              Zend/zend_vm_execute.h:1870  ZEND_DO_FCALL_SPEC_RETVAL_USED_HANDLER()
8                phar/phar_object.c:2526  zim_Phar_convertToData()
9                  phar/phar_object.c:2298  phar_convert_to_other()
10                    phar/phar_object.c:2115  phar_rename_archive()
                                pphar->fp = phar->fp;
				phar->fp = NULL;
                                phar_destroy_phar_data(phar); // alias released here

2. alias free'd second time call stack (from main() to destroy_phar_data())

1  cli/php_cli.c:1367  main()
2    cli/php_cli.c:1135  do_cli()
3      main/main.c:1810  php_request_shutdown()
4        Zend/zend.c:1262  zend_call_destructors()
5          Zend/zend_execute_API.c:256  shutdown_destructors()
6            Zend/zend_hash.c:1959  zend_hash_reverse_apply()
7              Zend/zend_hash.c:1388  _zend_hash_del_el()
8                Zend/zend_hash.c:1365  _zend_hash_del_el_ex()
9                  Zend/zend_variables.c:84  zval_ptr_dtor()
10                    Zend/zend_variables.h:44  i_zval_ptr_dtor()
11                      Zend/zend_variables.c:57  rc_dtor_func()
12                        Zend/zend_objects_API.c:200  zend_objects_store_del()
13                          spl/spl_directory.c:118  spl_filesystem_object_free_storage()
14                            phar/phar_object.c:1064  phar_spl_foreign_dtor()
15                              phar/phar.c:272  phar_archive_delref()
16                                Zend/zend_hash.c:1541  zend_hash_str_del()
17                                  Zend/zend_hash.c:1365  _zend_hash_del_el_ex()
18                                    phar/phar.c:342  destroy_phar_data()
	                                        if (EG(exception) || --phar_data->refcount < 0) {
		                                        phar_destroy_phar_data(phar_data); // alias free'd again
	                                        }

[iluuu1994]

Thank you @stkeke! Since I'm not familiar with phar at all (but also few people left seem to be) I'd be more comfortable merging this into master. A leak is better than memory corruption. Does that work for you?

[iluuu1994]

Just remove the code.

[stkeke]

Sure, no problem. You can simple change it.