Skip to content

Fix GH-15833: Segmentation fault (access null pointer) in ext/spl/spl_array.c #17235

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
+88 −0
Closed

Fix GH-15833: Segmentation fault (access null pointer) in ext/spl/spl_array.c #17235

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
34f1450
Fix GH-15833: Segmentation fault (access null pointer) in ext/spl/spl…
nielsdos Dec 21, 2024
013582b
fix?
nielsdos Dec 22, 2024
7b3a1f1
update test contents too ofc
nielsdos Dec 22, 2024
456c632
review
nielsdos Dec 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions ext/spl/spl_array.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,16 @@ static inline HashTable **spl_array_get_hash_table_ptr(spl_array_object* intern)
return &Z_ARRVAL(intern->array);
} else {
zend_object *obj = Z_OBJ(intern->array);
/* Since we're directly playing with the properties table, we shall initialize the lazy object directly.
* If we don't, it's possible to continue working with the wrong object in case we're using a proxy. */
if (UNEXPECTED(zend_lazy_object_must_init(obj))) {
obj = zend_lazy_object_init(obj);
Comment on lines +78 to +81
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zend_lazy_object_init() may return NULL when initialization fails (in which case EG(exception) is set, too).

Unfortunately I don't see other solution than to return NULL from spl_array_get_hash_table_ptr and to handle that in every caller. (An alternative would be to return a ptr-ptr to an empty array, and to rely on the exception to discard the result of the operation, but then we need to manage that array somehow.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch, okay, I'll do the NULL thing then :/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I believe we can just store the empty array in intern->array. I pushed a commit that does that, so the total patch stays simple. The only downside is that any further action won't do anything on the object, which may be fine as the object failed initialization anyway. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only downside is that any further action won't do anything on the object

I would prefer if further actions on the ArrayObject was carried normally, causing a new attempt to initialize the object and probably a new exception. Otherwise the silent failure could be surprising.

Storing the empty array in a new field in spl_array_object would be fine I think.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I also added some checks to avoid unexpected modification to the sentinel

if (UNEXPECTED(!obj)) {
zval_ptr_dtor(&intern->array);
ZVAL_ARR(&intern->array, zend_new_array(0));
return &Z_ARRVAL(intern->array);
}
}
/* rebuild properties */
zend_std_get_properties_ex(obj);
if (GC_REFCOUNT(obj->properties) > 1) {
Expand Down
22 changes: 22 additions & 0 deletions ext/spl/tests/gh15833_1.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
--TEST--
GH-15833 (Segmentation fault (access null pointer) in ext/spl/spl_array.c)
--CREDITS--
YuanchengJiang
--FILE--
<?php
class C {
public int $a = 1;
}
$reflector = new ReflectionClass(C::class);
$obj = $reflector->newLazyProxy(function ($obj) {
$obj = new C();
return $obj;
});
$recursiveArrayIterator = new RecursiveArrayIterator($obj);
var_dump($recursiveArrayIterator->current());
$recursiveArrayIterator->next();
var_dump($recursiveArrayIterator->current());
?>
--EXPECT--
int(1)
NULL
27 changes: 27 additions & 0 deletions ext/spl/tests/gh15833_2.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
--TEST--
GH-15833 (Segmentation fault (access null pointer) in ext/spl/spl_array.c)
--CREDITS--
YuanchengJiang
--FILE--
<?php
class C {
public int $a = 1;
}
$reflector = new ReflectionClass(C::class);
$obj = $reflector->newLazyProxy(function ($obj) {
throw new Error('foo');
});
$recursiveArrayIterator = new RecursiveArrayIterator($obj);
try {
var_dump($recursiveArrayIterator->current());
} catch (Error $e) {
echo $e->getMessage(), "\n";
}
var_dump($recursiveArrayIterator->current());
$recursiveArrayIterator->next();
var_dump($recursiveArrayIterator->current());
?>
--EXPECT--
foo
NULL
NULL
Loading