Renaming a file in a Phar to an already existing filename causes a NULL pointer dereference #13836

Activity

1. YuanchengJiang

   added

   Bug

   Status: Needs Triage

   on Mar 29, 2024
2. devnexen

   added

   Extension: phar

   on Mar 29, 2024
3. ndossche commented on Mar 29, 2024

   

   ndossche

   on Mar 29, 2024

   Member

   More actions

   Tested with ASAN, UBSAN, and Valgrind with both USE_ZEND_ALLOC=0 and USE_ZEND_ALLOC=1, also tested with debug and without debug; but I can't reproduce this...
   Please give more information about how you ran this test, info about your php.ini, and what configuration options you used. I'm testing on commit f2b2b7f.
4. ndossche

   added

   Status: Needs Feedback

   and removed

   Status: Needs Triage

   on Mar 29, 2024
5. YuanchengJiang commented on Mar 30, 2024

   

   YuanchengJiang

   on Mar 30, 2024

   Author

   More actions

   scripts to reproduce:

   /php-src/sapi/cli/php  -n -c '/php-src/tmp-php.ini'   -d "opcache.cache_id=worker22" -d "output_handler=" -d "open_basedir=" -d "disable_functions=" -d "output_buffering=Off" -d "error_reporting=32767" -d "display_errors=1" -d "display_startup_errors=1" -d "log_errors=0" -d "html_errors=0" -d "track_errors=0" -d "report_memleaks=1" -d "report_zend_debug=0" -d "docref_root=" -d "docref_ext=.html" -d "error_prepend_string=" -d "error_append_string=" -d "auto_prepend_file=" -d "auto_append_file=" -d "ignore_repeated_errors=0" -d "precision=14" -d "serialize_precision=-1" -d "memory_limit=128M" -d "opcache.fast_shutdown=0" -d "opcache.file_update_protection=0" -d "opcache.revalidate_freq=0" -d "opcache.jit_hot_loop=1" -d "opcache.jit_hot_func=1" -d "opcache.jit_hot_return=1" -d "opcache.jit_hot_side_exit=1" -d "opcache.jit_max_root_traces=100000" -d "opcache.jit_max_side_traces=100000" -d "opcache.jit_max_exit_counters=100000" -d "opcache.protect_memory=1" -d "zend.assertions=1" -d "zend.exception_ignore_args=0" -d "zend.exception_string_param_max_len=15" -d "short_open_tag=0" -d "extension_dir=/php-src/modules/" -d "zend_extension=/php-src/modules/opcache.so" -d "session.auto_start=0" -d "phar.require_hash=0" -d "phar.readonly=0" -f "/php-src/ext/phar/tests/test.php"  2>&1
   
6. github-actions

   added

   Status: Needs Triage

   and removed

   Status: Needs Feedback

   on Mar 30, 2024
7. ndossche commented on Mar 30, 2024

   

   ndossche

   on Mar 30, 2024

   Member

   More actions

   Thanks, it reproduces now; what was missing was running under opcache, and running it outside run-tests.php.
8. ndossche

   added

   Status: Verified

   and removed

   Status: Needs Triage

   on Mar 30, 2024
9. ndossche commented on Mar 30, 2024

   

   ndossche

   on Mar 30, 2024

   Last edited by ndossche

   Member

   More actions

   It seems it reproduces only starting from the second execution because the clean section doesn't actually clean it outside run-tests.php.
   What's actually going on is that if you rename a file to a filename that already exists, it crashes. This is because it uses the add api to add a new entry for the new filename, but that returns NULL because the entry exists already. That's why it only works after loading the existing phar that has the y file.
   Since this never worked before, we can just error out properly, similar to what copy does. EDIT: actually Phar::copy throws while copy succeeds so we should follow the latter.
10. ndossche

    changed the title [-]Member access within null pointer of type 'phar_entry_info' (aka 'struct _phar_entry_info') in /ext/phar/stream.c[/-] [+]Renaming a file in a Phar to an already existing filename causes a NULL pointer dereference[/+] on Mar 30, 2024
11. ndossche

    linked a pull request that will close this issueFix GH-13836: Renaming a file in a Phar to an already existing filename causes a NULL pointer dereference #13840on Mar 30, 2024
12. ndossche

    closed this as completedin ed8ed71on Mar 30, 2024
13. ndossche

    added a commit that references this issue on Mar 30, 2024

    Merge branch 'PHP-8.3'

    ...

    This issue will close once commit 0c3b389 is merged into the 'master' branch.

    0c3b389