ext/dba: Use persistent flatfile iterator memory

NEWS

@@ -70,6 +70,8 @@ PHP                                                                        NEWS
 - DBA:
   . Fixed bug GH-24641 (ndbm backend never checks dbm_open() return value,
     NULL deref on first use after a failed open). (Girgias)
+  . Fixed a crash with persistent flatfile, inifile, and cdb_make handles.
+    (Ilia Alshanetsky)
 
 - DOM:
   . Fixed a use-after-free when reflected attribute setters replace an

ext/dba/dba_flatfile.c

@@ -46,9 +46,6 @@ DBA_CLOSE_FUNC(flatfile)
 {
 	flatfile *dba = info->dbf;
 
-	if (dba->nextkey.dptr) {
-		efree(dba->nextkey.dptr);
-	}
 	pefree(dba, info->flags&DBA_PERSISTENT);
 }
 
@@ -123,36 +120,33 @@ DBA_DELETE_FUNC(flatfile)
 	return(flatfile_delete(dba, gkey) == -1 ? FAILURE : SUCCESS);
 }
 
+static zend_string *flatfile_key_to_string(datum key)
+{
+	if (!key.dptr) {
+		return NULL;
+	}
+
+	zend_string *str = zend_string_init(key.dptr, key.dsize, false);
+	efree(key.dptr);
+	return str;
+}
+
 DBA_FIRSTKEY_FUNC(flatfile)
 {
 	flatfile *dba = info->dbf;
 
-	if (dba->nextkey.dptr) {
-		efree(dba->nextkey.dptr);
-	}
-	dba->nextkey = flatfile_firstkey(dba);
-	if (dba->nextkey.dptr) {
-		return zend_string_init(dba->nextkey.dptr, dba->nextkey.dsize, /* persistent */ false);
-	}
-	return NULL;
+	return flatfile_key_to_string(flatfile_firstkey(dba));
 }
 
 DBA_NEXTKEY_FUNC(flatfile)
 {
 	flatfile *dba = info->dbf;
 
-	if (!dba->nextkey.dptr) {
+	if (!dba->CurrentFlatFilePos) {
 		return NULL;
 	}
 
-	if (dba->nextkey.dptr) {
-		efree(dba->nextkey.dptr);
-	}
-	dba->nextkey = flatfile_nextkey(dba);
-	if (dba->nextkey.dptr) {
-		return zend_string_init(dba->nextkey.dptr, dba->nextkey.dsize, /* persistent */ false);
-	}
-	return NULL;
+	return flatfile_key_to_string(flatfile_nextkey(dba));
 }
 
 DBA_OPTIMIZE_FUNC(flatfile)

ext/dba/libcdb/cdb_make.c

@@ -77,7 +77,7 @@ int cdb_make_addend(struct cdb_make *c, unsigned int keylen, unsigned int datale
 
 	head = c->head;
 	if (!head || (head->num >= CDB_HPLIST)) {
-		head = (struct cdb_hplist *) emalloc(sizeof(struct cdb_hplist));
+		head = pemalloc(sizeof(struct cdb_hplist), php_stream_is_persistent(c->fp));
 		if (!head)
 			return -1;
 		head->num = 0;
@@ -221,7 +221,7 @@ int cdb_make_finish(struct cdb_make *c)
 
 	for (x = c->head; x; c->head = x) {
 		x = x->next;
-		efree(c->head);
+		pefree(c->head, php_stream_is_persistent(c->fp));
 	}
 
 	if (php_stream_flush(c->fp) != 0)

ext/dba/libflatfile/flatfile.c

@@ -226,6 +226,7 @@ datum flatfile_firstkey(flatfile *dba) {
 		num = php_stream_read(dba->fp, buf, num);
 	}
 	efree(buf);
+	dba->CurrentFlatFilePos = 0;
 	res.dptr = NULL;
 	res.dsize = 0;
 	return res;
@@ -265,6 +266,7 @@ datum flatfile_nextkey(flatfile *dba) {
 		}
 	}
 	efree(buf);
+	dba->CurrentFlatFilePos = 0;
 	res.dptr = NULL;
 	res.dsize = 0;
 	return res;

ext/dba/libflatfile/flatfile.h

@@ -27,7 +27,6 @@ typedef struct {
 	int lockfd;
 	php_stream *fp;
 	size_t CurrentFlatFilePos;
-	datum nextkey;
 } flatfile;
 
 #define FLATFILE_INSERT 1

ext/dba/libinifile/inifile.c

@@ -76,6 +76,40 @@ void inifile_line_free(line_type *ln)
 }
 /* }}} */
 
+static void inifile_line_release(line_type *ln, bool persistent)
+{
+	if (ln->key.group) {
+		pefree(ln->key.group, persistent);
+	}
+	if (ln->key.name) {
+		pefree(ln->key.name, persistent);
+	}
+	if (ln->val.value) {
+		pefree(ln->val.value, persistent);
+	}
+	memset(ln, 0, sizeof(line_type));
+}
+
+static void inifile_line_store(line_type *dst, line_type *src, bool persistent)
+{
+	inifile_line_release(dst, persistent);
+	if (!persistent) {
+		*dst = *src;
+		return;
+	}
+	if (src->key.group) {
+		dst->key.group = pestrdup(src->key.group, true);
+	}
+	if (src->key.name) {
+		dst->key.name = pestrdup(src->key.name, true);
+	}
+	if (src->val.value) {
+		dst->val.value = pestrdup(src->val.value, true);
+	}
+	dst->pos = src->pos;
+	inifile_line_free(src);
+}
+
 /* {{{ inifile_alloc */
 inifile * inifile_alloc(php_stream *fp, int readonly, int persistent)
 {
@@ -100,8 +134,8 @@ inifile * inifile_alloc(php_stream *fp, int readonly, int persistent)
 void inifile_free(inifile *dba, int persistent)
 {
 	if (dba) {
-		inifile_line_free(&dba->curr);
-		inifile_line_free(&dba->next);
+		inifile_line_release(&dba->curr, persistent);
+		inifile_line_release(&dba->next, persistent);
 		pefree(dba, persistent);
 	}
 }
@@ -251,7 +285,7 @@ val_type inifile_fetch(inifile *dba, const key_type *key, int skip) {
 		/* specific instance or not same key -> restart search */
 		/* the slow way: restart and seacrch */
 		php_stream_rewind(dba->fp);
-		inifile_line_free(&dba->next);
+		inifile_line_release(&dba->next, php_stream_is_persistent(dba->fp));
 	}
 	if (skip == -1) {
 		skip = 0;
@@ -261,9 +295,8 @@ val_type inifile_fetch(inifile *dba, const key_type *key, int skip) {
 			if (!skip) {
 				val.value = estrdup(ln.val.value ? ln.val.value : "");
 				/* allow faster access by updating key read into next */
-				inifile_line_free(&dba->next);
-				dba->next = ln;
-				dba->next.pos = php_stream_tell(dba->fp);
+				ln.pos = php_stream_tell(dba->fp);
+				inifile_line_store(&dba->next, &ln, php_stream_is_persistent(dba->fp));
 				return val;
 			}
 			skip--;
@@ -282,7 +315,7 @@ val_type inifile_fetch(inifile *dba, const key_type *key, int skip) {
 
 /* {{{ inifile_firstkey */
 int inifile_firstkey(inifile *dba) {
-	inifile_line_free(&dba->curr);
+	inifile_line_release(&dba->curr, php_stream_is_persistent(dba->fp));
 	dba->curr.pos = 0;
 	return inifile_nextkey(dba);
 }
@@ -296,9 +329,8 @@ int inifile_nextkey(inifile *dba) {
 	php_stream_seek(dba->fp, dba->curr.pos, SEEK_SET);
 	ln.key.group = estrdup(dba->curr.key.group ? dba->curr.key.group : "");
 	inifile_read(dba, &ln);
-	inifile_line_free(&dba->curr);
-	dba->curr = ln;
-	return ln.key.group || ln.key.name;
+	inifile_line_store(&dba->curr, &ln, php_stream_is_persistent(dba->fp));
+	return dba->curr.key.group || dba->curr.key.name;
 }
 /* }}} */
 
@@ -325,8 +357,8 @@ static int inifile_find_group(inifile *dba, const key_type *key, size_t *pos_grp
 
 	php_stream_flush(dba->fp);
 	php_stream_seek(dba->fp, 0, SEEK_SET);
-	inifile_line_free(&dba->curr);
-	inifile_line_free(&dba->next);
+	inifile_line_release(&dba->curr, php_stream_is_persistent(dba->fp));
+	inifile_line_release(&dba->next, php_stream_is_persistent(dba->fp));
 
 	if (key->group && strlen(key->group)) {
 		int res;

ext/dba/tests/dba_cdb_make_persistent_insert.phpt

@@ -0,0 +1,60 @@
+--TEST--
+dba_popen() cdb_make handle inserted into in consecutive requests
+--EXTENSIONS--
+dba
+--SKIPIF--
+<?php
+require_once __DIR__ . '/setup/setup_dba_tests.inc';
+check_skip('cdb_make');
+if (PHP_OS_FAMILY === 'Windows') {
+    die('skip SIGINT cannot shut down the CLI server on Windows');
+}
+if (!file_exists(__DIR__ . "/../../../sapi/cli/tests/php_cli_server.inc")) {
+    echo "skip sapi/cli/tests/php_cli_server.inc required but not found";
+}
+?>
+--CONFLICTS--
+server
+--ENV--
+USE_ZEND_ALLOC=0
+USE_TRACKED_ALLOC=1
+--FILE--
+<?php
+include __DIR__ . "/../../../sapi/cli/tests/php_cli_server.inc";
+
+$db_file = __DIR__ . '/dba_cdb_make_persistent_insert.db';
+
+$args = ["-d", "extension_dir=" . ini_get("extension_dir"), "-d", "extension=" . (substr(PHP_OS, 0, 3) == "WIN" ? "php_" : "") . "dba." . PHP_SHLIB_SUFFIX];
+if (php_ini_loaded_file()) {
+    $args[] = "-c";
+    $args[] = php_ini_loaded_file();
+}
+$info = php_cli_server_start('$db = dba_popen(' . var_export($db_file, true) . ', "n", "cdb_make"); var_dump(dba_insert("key" . $_GET["n"], "value" . $_GET["n"], $db));', 'router.php', $args);
+
+for ($i = 1; $i <= 2; $i++) {
+    echo "Request $i:\n";
+    echo file_get_contents('http://' . PHP_CLI_SERVER_ADDRESS . '/?n=' . $i);
+}
+
+/* The persistent handle writes the cdb index when the server shuts down. */
+proc_terminate($info->processHandle, 2);
+for ($i = 0; $i < 100 && proc_get_status($info->processHandle)['running']; $i++) {
+    usleep(50000);
+}
+
+$db = dba_open($db_file, 'r-', 'cdb');
+var_dump(dba_fetch('key1', $db), dba_fetch('key2', $db));
+dba_close($db);
+?>
+--CLEAN--
+<?php
+require_once __DIR__ . '/setup/setup_dba_tests.inc';
+cleanup_standard_db(__DIR__ . '/dba_cdb_make_persistent_insert.db');
+?>
+--EXPECT--
+Request 1:
+bool(true)
+Request 2:
+bool(true)
+string(6) "value1"
+string(6) "value2"

ext/dba/tests/dba_flatfile_persistent_iteration.phpt

@@ -0,0 +1,51 @@
+--TEST--
+dba_popen() flatfile handle iterated in consecutive requests
+--EXTENSIONS--
+dba
+--SKIPIF--
+<?php
+require_once __DIR__ . '/setup/setup_dba_tests.inc';
+check_skip('flatfile');
+if (!file_exists(__DIR__ . "/../../../sapi/cli/tests/php_cli_server.inc")) {
+    echo "skip sapi/cli/tests/php_cli_server.inc required but not found";
+}
+?>
+--CONFLICTS--
+server
+--ENV--
+USE_ZEND_ALLOC=0
+USE_TRACKED_ALLOC=1
+--FILE--
+<?php
+include __DIR__ . "/../../../sapi/cli/tests/php_cli_server.inc";
+
+$db_file = __DIR__ . '/dba_flatfile_persistent_iteration.db';
+$db = dba_open($db_file, 'n', 'flatfile');
+dba_insert('key1', 'value1', $db);
+dba_insert('key2', 'value2', $db);
+dba_close($db);
+
+$args = ["-d", "extension_dir=" . ini_get("extension_dir"), "-d", "extension=" . (substr(PHP_OS, 0, 3) == "WIN" ? "php_" : "") . "dba." . PHP_SHLIB_SUFFIX];
+if (php_ini_loaded_file()) {
+    $args[] = "-c";
+    $args[] = php_ini_loaded_file();
+}
+php_cli_server_start('$db = dba_popen(' . var_export($db_file, true) . ', "r", "flatfile"); var_dump(dba_firstkey($db), dba_nextkey($db));', 'router.php', $args);
+
+for ($i = 1; $i <= 2; $i++) {
+    echo "Request $i:\n";
+    echo file_get_contents('http://' . PHP_CLI_SERVER_ADDRESS . '/');
+}
+?>
+--CLEAN--
+<?php
+require_once __DIR__ . '/setup/setup_dba_tests.inc';
+cleanup_standard_db(__DIR__ . '/dba_flatfile_persistent_iteration.db');
+?>
+--EXPECT--
+Request 1:
+string(4) "key1"
+string(4) "key2"
+Request 2:
+string(4) "key1"
+string(4) "key2"

ext/dba/tests/dba_inifile_persistent_iteration.phpt

@@ -0,0 +1,53 @@
+--TEST--
+dba_popen() inifile handle iterated in consecutive requests
+--EXTENSIONS--
+dba
+--SKIPIF--
+<?php
+require_once __DIR__ . '/setup/setup_dba_tests.inc';
+check_skip('inifile');
+if (!file_exists(__DIR__ . "/../../../sapi/cli/tests/php_cli_server.inc")) {
+    echo "skip sapi/cli/tests/php_cli_server.inc required but not found";
+}
+?>
+--CONFLICTS--
+server
+--ENV--
+USE_ZEND_ALLOC=0
+USE_TRACKED_ALLOC=1
+--FILE--
+<?php
+include __DIR__ . "/../../../sapi/cli/tests/php_cli_server.inc";
+
+$db_file = __DIR__ . '/dba_inifile_persistent_iteration.db';
+$db = dba_open($db_file, 'n', 'inifile');
+dba_insert('key1', 'value1', $db);
+dba_insert('key2', 'value2', $db);
+dba_close($db);
+
+$args = ["-d", "extension_dir=" . ini_get("extension_dir"), "-d", "extension=" . (substr(PHP_OS, 0, 3) == "WIN" ? "php_" : "") . "dba." . PHP_SHLIB_SUFFIX];
+if (php_ini_loaded_file()) {
+    $args[] = "-c";
+    $args[] = php_ini_loaded_file();
+}
+php_cli_server_start('$db = dba_popen(' . var_export($db_file, true) . ', "r", "inifile"); var_dump(dba_fetch("key2", $db), dba_firstkey($db), dba_nextkey($db));', 'router.php', $args);
+
+for ($i = 1; $i <= 2; $i++) {
+    echo "Request $i:\n";
+    echo file_get_contents('http://' . PHP_CLI_SERVER_ADDRESS . '/');
+}
+?>
+--CLEAN--
+<?php
+require_once __DIR__ . '/setup/setup_dba_tests.inc';
+cleanup_standard_db(__DIR__ . '/dba_inifile_persistent_iteration.db');
+?>
+--EXPECT--
+Request 1:
+string(6) "value2"
+string(4) "key1"
+string(4) "key2"
+Request 2:
+string(6) "value2"
+string(4) "key1"
+string(4) "key2"