Fix phpGH-17225: NULL deref in spl_directory.c

NULL checks for the glob stream are inconsistently applied. To solve
this generally, factor it out to a helper function so it's less likely
to be forgotten in the future.

Closes phpGH-17231.

Author: ndossche

NEWS

@@ -72,6 +72,9 @@ PHP                                                                        NEWS
     on SO_SNDTIMEO/SO_RCVTIMEO for socket_set_option().
     (David Carlier)
 
+- SPL:
+  . Fixed bug GH-17225 (NULL deref in spl_directory.c). (nielsdos)
+
 - Streams:
   . Fixed bug GH-17037 (UAF in user filter when adding existing filter name due
     to incorrect error handling). (nielsdos)

ext/spl/spl_directory.c

@@ -212,10 +212,16 @@ static zend_object *spl_filesystem_object_new(zend_class_entry *class_type)
 }
 /* }}} */
 
+static inline bool spl_intern_is_glob(const spl_filesystem_object *intern)
+{
+	/* NULL check on `dirp` is necessary as destructors may interfere. */
+	return intern->u.dir.dirp && php_stream_is(intern->u.dir.dirp, &php_glob_stream_ops);
+}
+
 PHPAPI zend_string *spl_filesystem_object_get_path(spl_filesystem_object *intern) /* {{{ */
 {
 #ifdef HAVE_GLOB
-	if (intern->type == SPL_FS_DIR && php_stream_is(intern->u.dir.dirp, &php_glob_stream_ops)) {
+	if (intern->type == SPL_FS_DIR && spl_intern_is_glob(intern)) {
 		size_t len = 0;
 		char *tmp = php_glob_stream_get_path(intern->u.dir.dirp, &len);
 		if (len == 0) {
@@ -658,7 +664,7 @@ static inline HashTable *spl_filesystem_object_get_debug_info(zend_object *objec
 	if (intern->type == SPL_FS_DIR) {
 #ifdef HAVE_GLOB
 		pnstr = spl_gen_private_prop_name(spl_ce_DirectoryIterator, "glob", sizeof("glob")-1);
-		if (intern->u.dir.dirp && php_stream_is(intern->u.dir.dirp ,&php_glob_stream_ops)) {
+		if (spl_intern_is_glob(intern)) {
 			ZVAL_STR_COPY(&tmp, intern->path);
 		} else {
 			ZVAL_FALSE(&tmp);
@@ -1614,11 +1620,11 @@ PHP_METHOD(GlobIterator, count)
 		RETURN_THROWS();
 	}
 
-	if (intern->u.dir.dirp && php_stream_is(intern->u.dir.dirp ,&php_glob_stream_ops)) {
+	if (spl_intern_is_glob(intern)) {
 		RETURN_LONG(php_glob_stream_get_count(intern->u.dir.dirp, NULL));
 	} else {
-		/* should not happen */
-		// TODO ZEND_ASSERT ?
+		/* This can happen by abusing destructors. */
+		/* TODO: relax this from E_ERROR to an exception */
 		php_error_docref(NULL, E_ERROR, "GlobIterator lost glob state");
 	}
 }

ext/spl/tests/gh17225.phpt

@@ -0,0 +1,52 @@
+--TEST--
+GH-17225 (NULL deref in spl_directory.c)
+--CREDITS--
+YuanchengJiang
+--EXTENSIONS--
+phar
+--INI--
+phar.readonly=0
+--FILE--
+<?php
+$fname = __DIR__ . '/gh17225.phar.zip';
+$phar = new Phar($fname);
+class HasDestructor {
+    public function __destruct() {
+        var_dump($GLOBALS['s']);
+    }
+}
+$s = new SplObjectStorage();
+$s[$phar] = new HasDestructor();
+register_shutdown_function(function() {
+    global $s;
+});
+var_dump($phar->isLink());
+?>
+--CLEAN--
+<?php
+@unlink(__DIR__ . '/gh17225.phar.zip');
+?>
+--EXPECTF--
+bool(false)
+object(SplObjectStorage)#%d (1) {
+  ["storage":"SplObjectStorage":private]=>
+  array(1) {
+    [0]=>
+    array(2) {
+      ["obj"]=>
+      object(Phar)#%d (4) {
+        ["pathName":"SplFileInfo":private]=>
+        string(0) ""
+        ["fileName":"SplFileInfo":private]=>
+        string(0) ""
+        ["glob":"DirectoryIterator":private]=>
+        bool(false)
+        ["subPathName":"RecursiveDirectoryIterator":private]=>
+        string(0) ""
+      }
+      ["inf"]=>
+      object(HasDestructor)#%d (0) {
+      }
+    }
+  }
+}