INIT_CLASS_ENTRY should use strlen() to determine class name length

[ghost]

Description

This works:

INIT_CLASS_ENTRY(ce, "mylongclassname", methods);

This does not:

void register_class(zend_class_entry * ce, const char * name, const zend_function_entry * methods) {
    INIT_CLASS_ENTRY(ce, name, methods);
}

register_class(ce, "mylongclassname", methods);

Because INIT_OVERLOADED_CLASS_ENTRY uses sizeof() to determine class_name_len:

#define INIT_OVERLOADED_CLASS_ENTRY(class_container, class_name, functions, handle_fcall, handle_propget, handle_propset) \
	INIT_OVERLOADED_CLASS_ENTRY_EX(class_container, class_name, sizeof(class_name)-1, functions, handle_fcall, handle_propget, handle_propset, NULL, NULL

PHP Version

All

Operating System

All

[ghost]

On current master (PHP-8.2) INIT_CLASS_ENTRY is calling INIT_CLASS_ENTRY_EX the same way.

[cmb69]

I don't understand why this would be a bug. Are you writing a PHP extension? If so, you can use INIT_CLASS_ENTRY_EX() in the first place, if you are not passing a constant as class.

[ghost]

I made several PHP extensions. Due to platform restrictions, these are all required to compile on multiple versions of PHP (5.4->8.0) . As the internals changed a lot over the years, I created several wrappers so that I didn't need to repeat large blocks of code doing the same just ever so slightly different.

One of these wrappers is responsible for registering classes and their corresponding methods. Among other things, this wrapper calls INIT_CLASS_ENTRY and the class name is passed as a string literal as per issue description so nothing really changed.

However, since INIT_CLASS_ENTRY is now taking the size of a pointer instead of the size of a string literal, class registration goes sideways. I worked around this issue by taking a reference to the string literal instead:

template<size_t name_len>
void register_class(const char (&name)[name_len], const zend_function_entry * methods) 

I could call INIT_CLASS_ENTRY_EX directly as you suggest but that macro also changed a lot over the years, requiring more wrapping but I've already arrived at the point where everything works as it should.

In any case, INIT_CLASS_ENTRY has a lot of "shooting yourself in the foot" potential as everything will compile without warnings or errors. Only when you actually try to use your shiny new extension you will notice weird problems.

I only encountered this problem until very late in the development process on 32-bit platforms because my class names just happened to be 7 characters long. At first I thought it was the usual 32/64-bit bug but a couple hours debugging later I found this gem, resulting in the expected "why???" response.

[cmb69]

these are all required to compile on multiple versions of PHP (5.4->8.0)

Ugh, that's a tough requirement.

Anyhow, I think we can change INIT_OVERLOADED_CLASS_ENTRY and maybe some others to no longer use sizeof, but rather strlen() instead, but not everybody may like that (see #8336).

[Girgias]

these are all required to compile on multiple versions of PHP (5.4->8.0)

Ugh, that's a tough requirement.

Anyhow, I think we can change INIT_OVERLOADED_CLASS_ENTRY and maybe some others to no longer use sizeof, but rather strlen() instead, but not everybody may like that (see #8336).

I think for stuff like this which may be used by extensions it makes sense to proceed with those changes compared to changing all possible instances within bundled extensions

[ghost]

these are all required to compile on multiple versions of PHP (5.4->8.0)

Ugh, that's a tough requirement.

It was not fun, no.

We all know sizeof() is intended to get the size of something and strlen() is intended to get the length of a string. Most compilers nowadays will substitute strlen() calls with a constant anyway so its really not optimizing anything.

I haven't gone too deep into it but historically there have been many attempts to limit/reduce copies of strings (interned strings, add_property_string* taking a duplicate Y/N flag, etc). Class name were probably required to be a string literal at some point in time (to save a copy operation) through I'm not sure this is the case no more.

The only real danger would be someone passing in a heap- or stack-allocated string, the string argument not being copied and something internally trying to dereference the original (now invalid) string pointer. Would be really easy to test, though.

By the way most/all of the non-ex add_property_* macros take sizeof(name) too. Or used to, argh... so many versions I had to deal with I can't remember!

[cmb69]

@dbuteyn, maybe you want to provide a respective PR?

[ghost]

make test reported no additional errors. The class name itself gets turned into a zend_string further down with a memcpy and the original argument is never referenced again.

[cmb69]

Yeah, I don't think sizeof has been used for any other purpose than having the optimization (old compilers may not have optimized strlen()).