ReflectionClass::isInstantiable() is wrong about internal classes that prevent instantiation

[DanielEScherzer]

Test script:

<?php

$classes = get_declared_classes();

foreach ( $classes as $clazz ) {
    $r = new ReflectionClass( $clazz );
    if ( !$r->isInstantiable() ) {
        continue;
    }
    try {
        new $clazz();
    } catch ( ArgumentCountError|TypeError $e ) {
        // Nothing
    } catch ( Throwable $e ) {
        echo $clazz . ": ";
        echo get_class( $e ) . ': ' . $e->getMessage() . "\n";
    }
}

Failures

For each of the following classes, ReflectionClass::isInstantiable() returns true, but trying to create an instance of the class directly fails:

Generator: Error: The "Generator" class is reserved for internal use and cannot be manually instantiated
WeakReference: Error: Direct instantiation of WeakReference is not allowed, use WeakReference::create instead
FiberError: Error: The "FiberError" class is reserved for internal use and cannot be manually instantiated
InflateContext: Error: Cannot directly construct InflateContext, use inflate_init() instead
DeflateContext: Error: Cannot directly construct DeflateContext, use deflate_init() instead
CurlHandle: Error: Cannot directly construct CurlHandle, use curl_init() instead
CurlMultiHandle: Error: Cannot directly construct CurlMultiHandle, use curl_multi_init() instead
CurlShareHandle: Error: Cannot directly construct CurlShareHandle, use curl_share_init() instead
CurlSharePersistentHandle: Error: Cannot directly construct CurlSharePersistentHandle, use curl_share_init_persistent() instead
Dba\Connection: Error: Cannot directly construct Dba\Connection, use dba_open() or dba_popen() instead
FFI: Error: Instantiation of FFI is not allowed
FFI\CData: Error: Instantiation of FFI\CData is not allowed
FFI\CType: Error: Instantiation of FFI\CType is not allowed
FTP\Connection: Error: Cannot directly construct FTP\Connection, use ftp_connect() or ftp_ssl_connect() instead
LDAP\Connection: Error: Cannot directly construct LDAP\Connection, use ldap_connect() instead
LDAP\Result: Error: Cannot directly construct LDAP\Result, use the dedicated functions instead
LDAP\ResultEntry: Error: Cannot directly construct LDAP\ResultEntry, use the dedicated functions instead
Directory: Error: Cannot directly construct Directory, use dir() instead
Odbc\Connection: Error: Cannot directly construct Odbc\Connection, use odbc_connect() or odbc_pconnect() instead
Odbc\Result: Error: Cannot directly construct Odbc\Result, use an appropriate odbc_* function instead
PDORow: PDOException: You may not create a PDORow manually
PgSql\Connection: Error: Cannot directly construct PgSql\Connection, use pg_connect() or pg_pconnect() instead
PgSql\Result: Error: Cannot directly construct PgSql\Result, use a dedicated function instead
PgSql\Lob: Error: Cannot directly construct PgSql\Lob, use pg_lo_open() instead
Shmop: Error: Cannot directly construct Shmop, use shmop_open() instead
Soap\Url: Error: Cannot directly construct Soap\Url
Soap\Sdl: Error: Cannot directly construct Soap\Sdl
SysvMessageQueue: Error: Cannot directly construct SysvMessageQueue, use msg_get_queue() instead
SysvSemaphore: Error: Cannot directly construct SysvSemaphore, use sem_get() instead
SysvSharedMemory: Error: Cannot directly construct SysvSharedMemory, use shm_attach() instead
XMLParser: Error: Cannot directly construct XMLParser, use xml_parser_create() or xml_parser_create_ns() instead

Not all of these extensions are on 3v4l but those that are can be confirmed at https://3v4l.org/BmAkH

I didn't test all extensions, what I have enabled locally is:

root@c1c025bca24f:/usr/src/php# php -i | grep "Configure Command"
Configure Command =>  './configure'  '--enable-fpm' '--with-pdo-mysql=mysqlnd' '--with-mysqli=mysqlnd' '--with-pgsql' '--with-pdo-pgsql' '--with-pdo-sqlite' '--enable-intl' '--without-pear' '--with-jpeg' '--with-webp' '--with-avif' '--with-freetype' '--with-xpm' '--enable-exif' '--with-zip' '--with-zlib' '--enable-soap' '--enable-xmlreader' '--with-xsl' '--with-tidy' '--enable-sysvsem' '--enable-sysvshm' '--enable-shmop' '--enable-pcntl' '--enable-mbstring' '--with-curl' '--with-gettext' '--with-bz2' '--with-gmp' '--enable-bcmath' '--enable-calendar' '--enable-ftp' '--with-enchant=/usr' '--enable-sysvmsg' '--with-ffi' '--enable-zend-test' '--enable-dl-test=shared' '--with-ldap' '--with-ldap-sasl' '--with-password-argon2' '--with-mhash' '--with-sodium' '--enable-dba' '--with-cdb' '--enable-flatfile' '--enable-inifile' '--with-tcadb' '--with-lmdb' '--with-qdbm' '--with-snmp' '--with-unixODBC' '--with-pdo-odbc=unixODBC,/usr' '--with-config-file-path=/etc' '--with-config-file-scan-dir=/etc/php.d' '--with-pdo-firebird' '--with-pdo-dblib' '--enable-debug' '--disable-zts'

Proposal

I propose that we add a new class flag, ZEND_ACC_NON_INSTANTIABLE, that classes can apply when they are registered.

PHP Version

65d4331

[Girgias]

I disagree with the proposal. We are limited in bit flags that we can use and assign.

This is also a more generic problem and affects many areas of the codebase. The reason is that instantiation is not determined just by "compile time" known information but depends on a call to the get_constructor object handler.

This is only ever overloaded by internal classes that prevent instantiation and have a custom error message. IMHO the simpler and more generic solution is to convert those classes to use a private constructor and get rid of the get_constructor object handler all together.

[iluuu1994]

Is there anything stopping us from making the constructor private in these cases, and circumventing this internally when constructing the objects?

We are limited in bit flags that we can use and assign.

We'll have to solve this eventually anyway. I did so in #13886 but it showed a significant increase in instruction count by Valgrind. We should check if this has a real-world impact. This may be caused by growing data structure, or less performant flag comparison, I'm not sure. Depending on the outcome, we can decide:

• Merge as-is, i.e. by making flags 64-bit.
• Create a union for the flags, so that the low 32-bit part can be accessed independently.
• Create a separate ce_flags_ex/fn_flags_ex field for new flags.

[DanielEScherzer]

This is only ever overloaded by internal classes that prevent instantiation and have a custom error message.

Hmm, I didn't realize that all overloads were just to report an error, but that is what github search reports. What about just having ReflectionClass::isInstantiable() return false if class object handlers have a get_constructor that is different from zend_std_get_constructor ?

[Girgias]

Is there anything stopping us from making the constructor private in these cases, and circumventing this internally when constructing the objects?

Not that I am aware off, and I don't think we necessarily need a custom error message. Would also make documentation more explicit IMHO on the php.net side.

I was planning on doing this soon-ish, but if @DanielEScherzer wants to do this I'm fhappy to let them do it (as its not that difficult either :) )

[nielsdos]

Is there anything stopping us from making the constructor private in these cases, and circumventing this internally when constructing the objects?

I agree with getting rid of the get_constructor trick an using private constructors. These private constructors still will need to throw though as they can be called via reflection, ignoring the visibility. I wasn't sure what you meant with "circumventing this internally", just wanted to point out the visibility escape hatch.

[Girgias]

AFAIK any internal object cannot be instantiated by Reflection. Am I mistaken?

[nielsdos]

AFAIK any internal object cannot be instantiated by Reflection. Am I mistaken?

That's wrong.

Here's an example btw where we call Dom\Node's private __construct function:

https://3v4l.org/uNfoU#v8.4.4

credits to #16190 where I learned you can do this

[Girgias]

I guess we need to have the constructor throw an exception then. Wait no that doesn't work...

Probably the easiest solution is to prevent reflection from instantiating internal classes... but don't know how much that will break.

[nielsdos]

At one point, this was more or less the case, but the constraint was relaxed in d586441

[DanielEScherzer]

I agree with getting rid of the get_constructor trick an using private constructors. These private constructors still will need to throw though as they can be called via reflection, ignoring the visibility.

Sounds good to me. Should I try and send one patch to update all of the extensions that currently use that get_constructor trick? Or would it be better to split them up (more patches to review but easier individually and less likely to get merge conflicts)?

[nielsdos]

Probably separate PRs, one for each ext, as otherwise we get a lot of codeowner review requests from a single PR and some parts may be already approved while others still waiting, which causes this weird effect where some parts could already be merged and some not.

[bwoebi]

I overall agree with the fact that we should get rid of these rejecting get_constructors, however the exceptions currently contain hints what should be done instead.
This feels like a good use case for (internal) attributes to enhance the error message. Like #[ConstructorForbidden("Use WeakReference::create() instead.")]. Which feels like possibly useful for userland too, whenever a private function __construct() is used.
(Using an attribute also feels perfectly fine from an overhead standpoint as it would only be fetched when an exception is anyway raised.)

[DanielEScherzer]

I overall agree with the fact that we should get rid of these rejecting get_constructors, however the exceptions currently contain hints what should be done instead. This feels like a good use case for (internal) attributes to enhance the error message. Like #[ConstructorForbidden("Use WeakReference::create() instead.")]. Which feels like possibly useful for userland too, whenever a private function __construct() is used. (Using an attribute also feels perfectly fine from an overhead standpoint as it would only be fetched when an exception is anyway raised.)

Sounds like a great idea to add such an attribute - but maybe NonpublicConstructor? Although, this could also be useful for other methods to explain why they aren't public...

I have a mostly working implementation of NonpublicConstructor, but when I tried to update zend_vm_def.h (for the code path of parent::__construct() the error message comes straight from the VM) I found that my changes were not being applied because of a weird error, and when I cleaned up my environment to reproduce it I found more errors, filed #17836.

[iluuu1994]

That's wrong.

Here's an example btw where we call Dom\Node's private __construct function:

https://3v4l.org/uNfoU#v8.4.4

Although it's worth noting that this only calls a constructor on an already constructed object, rather than constructing a new one, so it's not actually problematic. If the constructor were private and empty, calling it would simply do nothing. newInstanceWithoutConstructor on these objects is already inhibited because they are both internal and final. Likely, the rationale is that non-final internal classes could be overridden to circumvent the constructor anyway, so we assume the constructor is not necessary for correct initialization.

As Bob mentioned, one issue that remains is that we get a worse, generic error message (Call to private C::__construct()).

[DanielEScherzer]

As Bob mentioned, one issue that remains is that we get a worse, generic error message (Call to private C::__construct()).

I've sent #17846 to demonstrate how an attribute can provide the missing information. If there is any support for it I'll look into drafting an RFC