From 7353c7ce17e920fe4248a056a8f89cede696c0c1 Mon Sep 17 00:00:00 2001
From: Niels Dossche <7771979+nielsdos@users.noreply.github.com>
Date: Wed, 8 Nov 2023 20:06:18 +0100
Subject: [PATCH] Fix GH-12621: browscap segmentation fault when configured in
 the vhost

The temporary HashTable has a destructor that releases the string held
by the entry's value. However, browscap_intern_str(_ci) only incremented
the refcount for the reference created by the return value. As the
HashTable is only used during parsing, we don't need to manage the
reference count of the value anyway, so get rid of the destructor.

This is triggerable in two cases:
 - When using php_admin_value to set the ini at the activation stage
 - When running out of space for the opcache-interned strings

Closes GH-12634.
---
 NEWS                        |  2 ++
 ext/standard/browscap.c     | 10 ++++----
 sapi/fpm/tests/gh12621.phpt | 46 +++++++++++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+), 6 deletions(-)
 create mode 100644 sapi/fpm/tests/gh12621.phpt

diff --git a/NEWS b/NEWS
index a1a33700ed0..22d900c51eb 100644
--- a/NEWS
+++ b/NEWS
@@ -10,6 +10,8 @@ PHP                                                                        NEWS
 
 - Standard:
   . Fix memory leak in syslog device handling. (danog)
+  . Fixed bug GH-12621 (browscap segmentation fault when configured in the
+    vhost). (nielsdos)
 
 - SQLite3:
   . Fixed bug GH-12633 (sqlite3_defensive.phpt fails with sqlite 3.44.0).
diff --git a/ext/standard/browscap.c b/ext/standard/browscap.c
index 4fa766e3df8..d7b53e65993 100644
--- a/ext/standard/browscap.c
+++ b/ext/standard/browscap.c
@@ -228,7 +228,7 @@ static zend_string *browscap_intern_str(
 	} else {
 		interned = zend_string_copy(str);
 		if (persistent) {
-			interned = zend_new_interned_string(str);
+			interned = zend_new_interned_string(interned);
 		}
 		zend_hash_add_new_ptr(&ctx->str_interned, interned, interned);
 	}
@@ -397,10 +397,6 @@ static void php_browscap_parser_cb(zval *arg1, zval *arg2, zval *arg3, int callb
 }
 /* }}} */
 
-static void str_interned_dtor(zval *zv) {
-	zend_string_release(Z_STR_P(zv));
-}
-
 static int browscap_read_file(char *filename, browser_data *browdata, int persistent) /* {{{ */
 {
 	zend_file_handle fh;
@@ -430,7 +426,9 @@ static int browscap_read_file(char *filename, browser_data *browdata, int persis
 	ctx.bdata = browdata;
 	ctx.current_entry = NULL;
 	ctx.current_section_name = NULL;
-	zend_hash_init(&ctx.str_interned, 8, NULL, str_interned_dtor, persistent);
+	/* No dtor because we don't inc the refcount for the reference stored within the hash table's entry value
+	 * as the hash table is only temporary anyway. */
+	zend_hash_init(&ctx.str_interned, 8, NULL, NULL, persistent);
 
 	zend_parse_ini_file(&fh, persistent, ZEND_INI_SCANNER_RAW,
 			(zend_ini_parser_cb_t) php_browscap_parser_cb, &ctx);
diff --git a/sapi/fpm/tests/gh12621.phpt b/sapi/fpm/tests/gh12621.phpt
new file mode 100644
index 00000000000..0ee64fbe415
--- /dev/null
+++ b/sapi/fpm/tests/gh12621.phpt
@@ -0,0 +1,46 @@
+--TEST--
+GH-12621 (browscap segmentation fault when configured with php_admin_value)
+--SKIPIF--
+<?php include "skipif.inc"; ?>
+--FILE--
+<?php
+
+require_once "tester.inc";
+
+$cfg = <<<EOT
+[global]
+error_log = {{FILE:LOG}}
+[unconfined]
+listen = {{ADDR}}
+pm = dynamic
+pm.max_children = 5
+pm.start_servers = 1
+pm.min_spare_servers = 1
+pm.max_spare_servers = 3
+
+EOT;
+$cfg .= 'php_admin_value[browscap] = ' . __DIR__ . '/../../../ext/standard/tests/misc/browscap.ini';
+
+$code = <<<EOT
+<?php
+\$cv = get_browser("Konqueror/2.0")->browser_name_pattern;
+var_dump(\$cv);
+EOT;
+
+$tester = new FPM\Tester($cfg, $code);
+$tester->start();
+$tester->expectLogStartNotices();
+echo $tester
+    ->request()
+    ->getBody();
+$tester->terminate();
+$tester->close();
+
+?>
+--EXPECT--
+string(14) "*Konqueror/2.*"
+--CLEAN--
+<?php
+require_once "tester.inc";
+FPM\Tester::clean();
+?>