SplFileObject::fputcsv() via reflection causes segfault

[DanielEScherzer]

Description

The following code:

<?php

$obj = (new ReflectionClass( "SplFileObject" ))->newInstanceWithoutConstructor();
(new ReflectionMethod( "SplFileObject", "fputcsv" ))->invoke( $obj, [] );
echo "Done\n";

Resulted in this output:

[seg fault]

But I expected this output instead:

Done

See https://3v4l.org/uqjqf for confirmation.

Valgrind locally for 8.4 reports

==9459== Invalid read of size 8
==9459==    at 0x7939BD: _php_stream_write (streams.c:1297)
==9459==    by 0x6A1A6E: php_fputcsv (file.c:1795)
==9459==    by 0x61B03F: zim_SplFileObject_fputcsv (spl_directory.c:2369)
==9459==    by 0x87719D: zend_call_function (zend_execute_API.c:1009)
==9459==    by 0x87757D: zend_call_known_function (zend_execute_API.c:1090)
==9459==    by 0x5D6433: reflection_method_invoke (php_reflection.c:3483)
==9459==    by 0x5D6575: zim_ReflectionMethod_invoke (php_reflection.c:3501)
==9459==    by 0x88E5E3: ZEND_DO_FCALL_SPEC_RETVAL_UNUSED_HANDLER (zend_vm_execute.h:1919)
==9459==    by 0x909846: execute_ex (zend_vm_execute.h:58830)
==9459==    by 0x90E0F8: zend_execute (zend_vm_execute.h:64217)
==9459==    by 0x9A4C45: zend_execute_script (zend.c:1928)
==9459==    by 0x76EEF7: php_execute_script_ex (main.c:2574)

PHP Version

PHP 8.2+

Operating System

No response

[devnexen]

ubsan

main/streams/streams.c:1297:14: runtime error: member access within null pointer of type 'php_stream' (aka 'struct _php_stream')

[iluuu1994]

Honestly, I'd be surprised if there weren't many other cases of newInstanceWithoutConstructor() causing segfaults. This method is inherently risky, and users are responsible for ensuring proper object initialization. Handling them would just pollute and slow down the code. I remember having discussed this with @arnaud-lb on GitHub in the past, but I can't find the reference.

[cmb69]

I think we should do our best to avoid segfaults and undefined behavior. Instead of doing is_initialized checks in all methods, maybe it would be sufficient to explicitly disallow Reflection::newInstanceWithoutConstructor() for more classes. Currently we do:

php-src/ext/reflection/php_reflection.c

Lines 5144 to 5148 in 2501cad

	if (ce->type == ZEND_INTERNAL_CLASS
	&& ce->create_object != NULL && (ce->ce_flags & ZEND_ACC_FINAL)) {
	zend_throw_exception_ex(reflection_exception_ptr, 0, "Class %s is an internal class marked as final that cannot be instantiated without invoking its constructor", ZSTR_VAL(ce->name));
	RETURN_THROWS();
	}

Maybe we could introduce another flag (I guess we're running out of these, though); or maybe we can disallow the reflection method for all internal classes which are not serializable.

[iluuu1994]

I don't object to that, although I'm not sure how much it will help, as we don't use @not-serializable that often. I would object to all internal classes having to handle very unexpected cases to be "correct". Ofc I agree that we should prevent segfaults as much as possible, but I would say that newInstanceWithoutConstructor() is a "power user" tool, and it should be used with caution.

Strictly speaking, it might not be sound to call newInstanceWithoutConstructor() on any internal class with a constructor, as the constructor may initialize internal state that is impossible for the user to set (without calling the constructor later), as this initialization logic doesn't necessarily need to happen inside create_object. It looks like this is how it was initially implemented:

b04a052

This was later changed to allowing internal classes with non-final constructors.

d586441

I'm guessing the rationale is that a child class might override __construct() without calling parent::__construct(), which would lead to similar issues.

[arnaud-lb]

Found the earlier discussion: #11416 (comment)

I agree that we should do our best to avoid segfaults.

Most internal classes need to cast EX(This) from zend_object* to a more specific type, and have a macro to do it. This macro is a good place to also check that the object is fully initialized. I think that most objects have a macro that does the cast + initialization check at the same time like this:

php-src/ext/reflection/php_reflection.c

Lines 113 to 127 in 2501cad

	#define GET_REFLECTION_OBJECT() do { \
	intern = Z_REFLECTION_P(ZEND_THIS); \
	if (intern->ptr == NULL) { \
	if (EG(exception) && EG(exception)->ce == reflection_exception_ptr) { \
	RETURN_THROWS(); \
	} \
	zend_throw_error(NULL, "Internal error: Failed to retrieve the reflection object"); \
	RETURN_THROWS(); \
	} \
	} while (0)
	#define GET_REFLECTION_OBJECT_PTR(target) do { \
	GET_REFLECTION_OBJECT(); \
	target = intern->ptr; \
	} while (0)

SplFileObject uses a different approach by doing this check in the get_method handler:

php-src/ext/spl/spl_directory.c

Lines 664 to 669 in 2501cad

	static zend_function *spl_filesystem_object_get_method_check(zend_object **object, zend_string *method, const zval *key) /* {{{ */
	{
	spl_filesystem_object *fsobj = spl_filesystem_from_obj(*object);
	if (fsobj->u.dir.dirp == NULL && fsobj->orig_path == NULL) {
	zend_throw_error(NULL, "The parent constructor was not called: the object is in an invalid state");

That's why calling $obj->fputcsv() throws, but calling it via ReflectionMethod does not. Unfortunately this approach also breaks with inline caches: https://3v4l.org/XMvIQ

Most of these cases should be discovered by this test. Maybe we can extend it to also call the methods with ReflectionClass?

Unfortunately, denying newInstanceWithoutConstructor() is not enough to prevent this entirely as there are other ways to instantiate without calling the constructor, such as sub-classing and unserialize(), and preventing these may break some use-cases (e.g. test mocking).

[iluuu1994]

Maybe we can extend it to also call the methods with ReflectionClass?

That sounds like a risky test. 😅

Edit: Oh, we're already calling functions. It seems we have already thrown caution to the wind 😄

[cmb69]

The problem with arginfo_zpp_mismatch.phpt is that it only checks already loaded classes; this is why it does not segfault when com_safearray_proxy is instantiated this way. Maybe we should add support for * in --EXTENSIONS-- to load all available extensions (to my knowledge, there are 4 such tests which would need that).

[Girgias]

May I propose an idea?

Currently, get_constructor object handler is required.
This leads to us needing to manually specify one for each class that are not meant to be instantiated, moreover it may return NULL in two contradicting cases:

• It cannot be instantiated due to visibility, and an exception has been thrown
• No method __construct() exists, so the object is not initialized.

It might be better to make this object handler optional, and if this is NULL just ban newInstanceWithoutConstructor() outright, as only internal classes would be able to set this to NULL.
This might also address the issues with unserialize() as it would know when it cannot be instantiated just by looking at the default object handlers of the CE.

Don't have an idea how to address the internal classes that have a constructor which must be called. But tbf what is even the point of using newInstanceWithoutConstructor()?

Also note that PDO may also instantiate a class without calling the constructor.

[arnaud-lb]

Unfortunately this doesn't solve the issue for classes that have a constructor and are supposed to be instantiated with new

[DanielEScherzer]

Honestly, I'd be surprised if there weren't many other cases of newInstanceWithoutConstructor() causing segfaults. This method is inherently risky, and users are responsible for ensuring proper object initialization. Handling them would just pollute and slow down the code. I remember having discussed this with @arnaud-lb on GitHub in the past, but I can't find the reference.

For the record, I tried running (for the built in classes and extensions enabled by default, i.e. only from ./configure --enable-debug) essentially every built in class with every built in method via reflection, and this was the only seg fault that I found. You can see my testing script at https://gist.github.com/DanielEScherzer/1ca7856f2c25dbf30ffda1f9f529ffdd - lots of classes threw exceptions with messages like "Object of type Date... has not been correctly initialized" or "The parent constructor was not called"

I'm planning to do this for all of the bundled extensions at some point soon though

[cmb69]

I'm planning to do this for all of the bundled extensions at some point soon though

There is already Zend/tests/arginfo_zpp_mismatch.phpt and friends, which instantiate all classes via ::newInstanceWithoutConstructor() and call all methods on it. It may make sense to build on these tests, and only add passing somewhat reasonable arguments.

Maybe we should add support for * in --EXTENSIONS-- to load all available extensions

See PR #16251 (shot myself in the foot when removing the dl() check ;)

[nielsdos]

Another one: #16763