Fix GH-16390: dba_open() can segfault for "pathless" streams

[cmb69]

dba_open() accepts arbitrary stream wrapper paths, but unless no locking (-) is specified, we try to determine the underlying file path. If that fails, we need to error out.

---

Note that this had partially be resolved via 421c56d (i.e. as of PHP 8.3.0), but the ZEND_ASSERT(opened_path) is not correct; we cannot assert that all streams have a path, but need to catch that at runtime.

Patch for PHP 8.3 and up

From 88091f3c74eaa313aa58d1e349a3de09e909b4ea Mon Sep 17 00:00:00 2001
From: "Christoph M. Becker" <[email protected]>
Date: Fri, 18 Oct 2024 20:49:37 +0200
Subject: [PATCH] Fix GH-16390: dba_open() can segfault for "pathless" streams

`dba_open()` accepts arbitrary stream wrapper paths, but unless no
locking (`-`) is specified, we try to determine the underlying file
path.  If that fails, we need to error out.
---
 ext/dba/dba.c              | 15 +++++++++------
 ext/dba/tests/gh16390.phpt | 11 +++++++++++
 2 files changed, 20 insertions(+), 6 deletions(-)
 create mode 100644 ext/dba/tests/gh16390.phpt

diff --git a/ext/dba/dba.c b/ext/dba/dba.c
index 04cee6f385..a2f8c780ef 100644
--- a/ext/dba/dba.c
+++ b/ext/dba/dba.c
@@ -842,10 +842,13 @@ static void php_dba_open(INTERNAL_FUNCTION_PARAMETERS, bool persistent)
 			connection->info->lock.fp = php_stream_open_wrapper(lock_name, lock_file_mode, STREAM_MUST_SEEK|REPORT_ERRORS|IGNORE_PATH|persistent_flag, &opened_path);
 			if (connection->info->lock.fp) {
 				if (is_db_lock) {
-					ZEND_ASSERT(opened_path);
-					/* replace the path info with the real path of the opened file */
-					zend_string_release_ex(connection->info->path, persistent);
-					connection->info->path = php_dba_zend_string_dup_safe(opened_path, persistent);
+					if (opened_path) {
+						/* replace the path info with the real path of the opened file */
+						zend_string_release_ex(connection->info->path, persistent);
+						connection->info->path = php_dba_zend_string_dup_safe(opened_path, persistent);
+					} else {
+						error = "Unable to determine path for locking";
+					}
 				}
 			}
 			if (opened_path) {
@@ -862,10 +865,10 @@ static void php_dba_open(INTERNAL_FUNCTION_PARAMETERS, bool persistent)
 			zval_ptr_dtor(return_value);
 			RETURN_FALSE;
 		}
-		if (!php_stream_supports_lock(connection->info->lock.fp)) {
+		if (!error && !php_stream_supports_lock(connection->info->lock.fp)) {
 			error = "Stream does not support locking";
 		}
-		if (php_stream_lock(connection->info->lock.fp, lock_mode)) {
+		if (!error && php_stream_lock(connection->info->lock.fp, lock_mode)) {
 			error = "Unable to establish lock"; /* force failure exit */
 		}
 	}
diff --git a/ext/dba/tests/gh16390.phpt b/ext/dba/tests/gh16390.phpt
new file mode 100644
index 0000000000..78015d6eef
--- /dev/null
+++ b/ext/dba/tests/gh16390.phpt
@@ -0,0 +1,11 @@
+--TEST--
+GH-16390 (dba_open() can segfault for "pathless" streams)
+--EXTENSIONS--
+dba
+--FILE--
+<?php
+$file = 'data:text/plain;z=y;uri=eviluri;mediatype=wut?;mediatype2=hello,somedata';
+$db = dba_open($file, 'c', 'inifile');
+?>
+--EXPECTF--
+Warning: dba_open(): Driver initialization failed for handler: inifile: Unable to determine path for locking in %s on line %d
-- 
2.45.2.windows.1

Also note that contrary to my former suggestion, I now went with a warning, because this is more inline with the other error handling, and simplifies resource management.

[Girgias]

LGTM, I didn't really know one could not have a path be initialized, I do wonder if other parts of the codebase might not be affected by this same assumption?

[cmb69]

I do wonder if other parts of the codebase might not be affected by this same assumption?

Might be worthwhile to have a closer look, but most pass NULL to opened_path anyway, so there is no issue.

In dba.c, just a few lines above, there seems to be a superfluous usage of opened_path, which is then released right away. I'll come up with PR for master (since that is not a bug).