From c248081601d91beb11c3312ba1b6c3ea36e03e51 Mon Sep 17 00:00:00 2001
From: Niels Dossche <7771979+nielsdos@users.noreply.github.com>
Date: Fri, 5 Apr 2024 21:46:07 +0200
Subject: [PATCH 1/4] Fix GH-13891: memleak and segfault when using ini_set
with session.trans_sid_hosts
The hash tables used are allocated via the persistent allocator.
When using ini_set, the allocation happens via the non-persistent
allocator. When the table is then freed in GSHUTDOWN, we get a crash
because the allocators are mismatched.
As a side note, it is strange that this is designed this way, because it
means that ini_sets persist between requests...
Test credits go to Kamil Tekiela.
---
ext/session/tests/gh13891.phpt | 26 ++++++++++++++++++++++++++
ext/standard/url_scanner_ex.re | 8 ++++++--
2 files changed, 32 insertions(+), 2 deletions(-)
create mode 100644 ext/session/tests/gh13891.phpt
diff --git a/ext/session/tests/gh13891.phpt b/ext/session/tests/gh13891.phpt
new file mode 100644
index 0000000000000..ba2267cf3a842
--- /dev/null
+++ b/ext/session/tests/gh13891.phpt
@@ -0,0 +1,26 @@
+--TEST--
+GH-13891 (memleak and segfault when using ini_set with session.trans_sid_hosts)
+--INI--
+session.use_cookies=0
+session.use_only_cookies=0
+session.use_trans_sid=1
+--EXTENSIONS--
+session
+--SKIPIF--
+
+--FILE--
+
+Click This Anchor Tag!
+External link with anchor
+External link with anchor 2
+Internal link
+--EXPECT--
+Click This Anchor Tag!
+External link with anchor
+External link with anchor 2
+Internal link
diff --git a/ext/standard/url_scanner_ex.re b/ext/standard/url_scanner_ex.re
index 8e5b43467fc88..f35990636794f 100644
--- a/ext/standard/url_scanner_ex.re
+++ b/ext/standard/url_scanner_ex.re
@@ -138,9 +138,13 @@ static int php_ini_on_update_hosts(zend_ini_entry *entry, zend_string *new_value
}
keylen = q - key;
if (keylen > 0) {
- tmp_key = zend_string_init(key, keylen, 0);
+ /* Note: the hash table is persistently allocated, so the strings must be too!
+ * FIXME: the reason this is persistent is because the tables are allocated in GINIT,
+ * so it survives the request. This also means that the values set by `init_set` carry
+ * over from request to request, which is strange... */
+ tmp_key = zend_string_init(key, keylen, true);
zend_hash_add_empty_element(hosts, tmp_key);
- zend_string_release_ex(tmp_key, 0);
+ zend_string_release_ex(tmp_key, true);
}
}
efree(tmp);
From cce8ccbc07ab681dd78c2dda661d2ce609d68cbb Mon Sep 17 00:00:00 2001
From: Niels Dossche <7771979+nielsdos@users.noreply.github.com>
Date: Fri, 5 Apr 2024 22:53:45 +0200
Subject: [PATCH 2/4] Update ext/standard/url_scanner_ex.re
Co-authored-by: Kamil Tekiela
---
ext/standard/url_scanner_ex.re | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ext/standard/url_scanner_ex.re b/ext/standard/url_scanner_ex.re
index f35990636794f..da0d4c52a3e19 100644
--- a/ext/standard/url_scanner_ex.re
+++ b/ext/standard/url_scanner_ex.re
@@ -140,7 +140,7 @@ static int php_ini_on_update_hosts(zend_ini_entry *entry, zend_string *new_value
if (keylen > 0) {
/* Note: the hash table is persistently allocated, so the strings must be too!
* FIXME: the reason this is persistent is because the tables are allocated in GINIT,
- * so it survives the request. This also means that the values set by `init_set` carry
+ * so it survives the request. This also means that the values set by `ini_set` carry
* over from request to request, which is strange... */
tmp_key = zend_string_init(key, keylen, true);
zend_hash_add_empty_element(hosts, tmp_key);
From 03166644d9e0f272738df29f0ffa32336a702a7c Mon Sep 17 00:00:00 2001
From: Niels Dossche <7771979+nielsdos@users.noreply.github.com>
Date: Fri, 5 Apr 2024 22:57:06 +0200
Subject: [PATCH 3/4] Further review comment fixes
---
ext/session/tests/gh13891.phpt | 10 ----------
ext/standard/url_scanner_ex.re | 5 +----
2 files changed, 1 insertion(+), 14 deletions(-)
diff --git a/ext/session/tests/gh13891.phpt b/ext/session/tests/gh13891.phpt
index ba2267cf3a842..51e0a15677fd5 100644
--- a/ext/session/tests/gh13891.phpt
+++ b/ext/session/tests/gh13891.phpt
@@ -12,15 +12,5 @@ session
-Click This Anchor Tag!
-External link with anchor
-External link with anchor 2
-Internal link
--EXPECT--
-Click This Anchor Tag!
-External link with anchor
-External link with anchor 2
-Internal link
diff --git a/ext/standard/url_scanner_ex.re b/ext/standard/url_scanner_ex.re
index da0d4c52a3e19..a4a48b6bf60ee 100644
--- a/ext/standard/url_scanner_ex.re
+++ b/ext/standard/url_scanner_ex.re
@@ -138,10 +138,7 @@ static int php_ini_on_update_hosts(zend_ini_entry *entry, zend_string *new_value
}
keylen = q - key;
if (keylen > 0) {
- /* Note: the hash table is persistently allocated, so the strings must be too!
- * FIXME: the reason this is persistent is because the tables are allocated in GINIT,
- * so it survives the request. This also means that the values set by `ini_set` carry
- * over from request to request, which is strange... */
+ /* Note: the hash table is persistently allocated, so the strings must be too! */
tmp_key = zend_string_init(key, keylen, true);
zend_hash_add_empty_element(hosts, tmp_key);
zend_string_release_ex(tmp_key, true);
From c6f3138faf2e2c9407616dfe401fc12dd1bbd714 Mon Sep 17 00:00:00 2001
From: Niels Dossche <7771979+nielsdos@users.noreply.github.com>
Date: Fri, 5 Apr 2024 23:10:31 +0200
Subject: [PATCH 4/4] Add it also in the ini section for better reproducibility
---
ext/session/tests/gh13891.phpt | 1 +
1 file changed, 1 insertion(+)
diff --git a/ext/session/tests/gh13891.phpt b/ext/session/tests/gh13891.phpt
index 51e0a15677fd5..7df9bffa770bc 100644
--- a/ext/session/tests/gh13891.phpt
+++ b/ext/session/tests/gh13891.phpt
@@ -4,6 +4,7 @@ GH-13891 (memleak and segfault when using ini_set with session.trans_sid_hosts)
session.use_cookies=0
session.use_only_cookies=0
session.use_trans_sid=1
+session.trans_sid_hosts=php.net
--EXTENSIONS--
session
--SKIPIF--