From ed8ed714a86406eb8634043ac4afab577d0fd546 Mon Sep 17 00:00:00 2001
From: Niels Dossche <7771979+nielsdos@users.noreply.github.com>
Date: Sat, 30 Mar 2024 11:53:01 +0100
Subject: [PATCH] Fix GH-13836: Renaming a file in a Phar to an already
 existing filename causes a NULL pointer dereference

If the destination already exists, then the `add` function on the
manifest will return NULL, resulting in a NULL entry and therefore a
NULL deref. As `copy()` (not `Phar::copy`) chooses to succeed and
overwrite the destination if it already exists, we should do the same.
Therefore the fix is as simple as changing `add` to `update`.

Closes GH-13840.
---
 NEWS                        |  4 ++++
 ext/phar/stream.c           |  5 +++--
 ext/phar/tests/gh13833.phpt | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 40 insertions(+), 2 deletions(-)
 create mode 100644 ext/phar/tests/gh13833.phpt

diff --git a/NEWS b/NEWS
index ff9fa5a9591..946ea2911eb 100644
--- a/NEWS
+++ b/NEWS
@@ -10,6 +10,10 @@ PHP                                                                        NEWS
   . Fixed bug GH-10495 (feof on OpenSSL stream hangs indefinitely).
     (Jakub Zelenka)
 
+- Phar:
+  . Fixed bug GH-13836 (Renaming a file in a Phar to an already existing
+    filename causes a NULL pointer dereference). (nielsdos)
+
 - PHPDBG:
   . Fixed bug GH-13827 (Null pointer access of type 'zval' in phpdbg_frame).
     (nielsdos)
diff --git a/ext/phar/stream.c b/ext/phar/stream.c
index adebe5ab9bd..ceedbdae353 100644
--- a/ext/phar/stream.c
+++ b/ext/phar/stream.c
@@ -858,8 +858,9 @@ static int phar_wrapper_rename(php_stream_wrapper *wrapper, const char *url_from
 		entry->link = entry->tmp = NULL;
 		source = entry;
 
-		/* add to the manifest, and then store the pointer to the new guy in entry */
-		entry = zend_hash_str_add_mem(&(phar->manifest), ZSTR_VAL(resource_to->path)+1, ZSTR_LEN(resource_to->path)-1, (void **)&new, sizeof(phar_entry_info));
+		/* add to the manifest, and then store the pointer to the new guy in entry
+		 * if it already exists, we overwrite the destination like what copy('phar://...', 'phar://...') does. */
+		entry = zend_hash_str_update_mem(&(phar->manifest), ZSTR_VAL(resource_to->path)+1, ZSTR_LEN(resource_to->path)-1, (void **)&new, sizeof(phar_entry_info));
 
 		entry->filename = estrndup(ZSTR_VAL(resource_to->path)+1, ZSTR_LEN(resource_to->path)-1);
 		if (FAILURE == phar_copy_entry_fp(source, entry, &error)) {
diff --git a/ext/phar/tests/gh13833.phpt b/ext/phar/tests/gh13833.phpt
new file mode 100644
index 00000000000..d1b79d033e0
--- /dev/null
+++ b/ext/phar/tests/gh13833.phpt
@@ -0,0 +1,33 @@
+--TEST--
+GH-13836 (Renaming a file in a Phar to an already existing filename causes a NULL pointer dereference)
+--EXTENSIONS--
+phar
+--INI--
+phar.require_hash=0
+phar.readonly=0
+--FILE--
+<?php
+$fname = __DIR__ . '/gh13836.phar';
+
+$phar = new Phar($fname, 0, 'a.phar');
+$phar['x'] = 'hi1';
+$phar['y'] = 'hi2';
+
+var_dump(rename("phar://a.phar/x", "phar://a.phar/y"));
+
+var_dump(isset($phar['x']));
+var_dump($phar['y']);
+?>
+--CLEAN--
+<?php
+unlink(__DIR__ . '/gh13836.phar');
+?>
+--EXPECTF--
+bool(true)
+bool(false)
+object(PharFileInfo)#2 (2) {
+  ["pathName":"SplFileInfo":private]=>
+  string(%d) "phar://%sgh13836.phar/y"
+  ["fileName":"SplFileInfo":private]=>
+  string(1) "y"
+}