# Null bytes in anonymous class names

- Thread: https://externals.io/message/89079
- Original archive: https://news-web.php.net/php.internals/89079
- Started by Niklas Keller on 2015-11-05T14:21:46+00:00
- Messages: 19
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · RFCs at https://externals.io/rfc.md · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #89079 — Niklas Keller — 2015-11-05T14:21:46+00:00

URL: https://externals.io/message/89079#89079

Hello,

I discovered today that anonymous class names contain a null byte right
after "class@anonymous". I don't think class names should contain
non-printable characters.

How about removing that null byte?

https://3v4l.org/QUKpV

https://github.com/php/php-src/blob/da8e6ec4a5063d9f60f83f43c55bc17d015cac8b/Zend/zend_compile.c#L5207

Regards, Niklas

---

## #89081 — Leigh — 2015-11-05T14:45:05+00:00

URL: https://externals.io/message/89079#89081 · In reply to: #89079

> Hello,
>
> I discovered today that anonymous class names contain a null byte right
> after "class@anonymous". I don't think class names should contain
> non-printable characters.
>
> How about removing that null byte?
>
> https://3v4l.org/QUKpV
>
>
> https://github.com/php/php-src/blob/da8e6ec4a5063d9f60f83f43c55bc17d015cac8b/Zend/zend_compile.c#L5207
>
> Regards, Niklas
>

It's also quite worrying that it is leaking memory addresses.

---

## #89082 — Rowan Collins — 2015-11-05T14:59:29+00:00

URL: https://externals.io/message/89079#89082 · In reply to: #89079

> Hello,
>
> I discovered today that anonymous class names contain a null byte right
> after "class@anonymous". I don't think class names should contain
> non-printable characters.
>
> How about removing that null byte?
>
> https://3v4l.org/QUKpV
>
> https://github.com/php/php-src/blob/da8e6ec4a5063d9f60f83f43c55bc17d015cac8b/Zend/zend_compile.c#L5207
>
> Regards, Niklas
>

PHP uses null bytes quite a lot to produce deliberately illegal 
identifiers. For instance the old eval-like create_function() [e.g. 
https://3v4l.org/hqHjh] and the serialization of private members [e.g. 
https://3v4l.org/R6Y6k]

In this case, I guess the "@" in "class@anonymous" makes the name 
illegal anyway, but I'm not sold on the null byte being more 
unacceptable here than anywhere else.

Regards,

-- 
Rowan Collins
[IMSoP]

---

## #89083 — Leigh — 2015-11-05T15:14:47+00:00

URL: https://externals.io/message/89079#89083 · In reply to: #89082

>
> PHP uses null bytes quite a lot to produce deliberately illegal
> identifiers. For instance the old eval-like create_function() [e.g.
> https://3v4l.org/hqHjh] and the serialization of private members [e.g.
> https://3v4l.org/R6Y6k]
>
> In this case, I guess the "@" in "class@anonymous" makes the name illegal
> anyway, but I'm not sold on the null byte being more unacceptable here than
> anywhere else.
>
> Regards,
>
> --
> Rowan Collins
> [IMSoP]
>
> That doesn't mean it's a good approach (*cough* namespaces *cough*), and
these bits of "magic" are supposed to be hidden away from users. I'm
guessing in this particular instance, the point of the null is to make
string operations cut off after "anonymous", however string operations that
respect the zval string length aren't going to do this.

e.g. `var_dump()` the class name is put through sprintf and it cuts off at
the null, but get_class or ReflectionClass::getName() just returns the
original string, and exposes the implementation details.

---

## #89096 — Niklas Keller — 2015-11-06T13:25:40+00:00

URL: https://externals.io/message/89079#89096 · In reply to: #89083

Problem is that e.g. exception to string casts do not handle it properly,
there may be other affected areas.

https://3v4l.org/e9AGZ

Regards, Niklas

2015-11-05 16:14 GMT+01:00 Leigh <leight@gmail.com>:

> On 5 November 2015 at 14:59, Rowan Collins <rowan.collins@gmail.com>
> wrote:
>
> > […]
> illegal
> > […]
> than
> > […]
> these bits of "magic" are supposed to be hidden away from users. I'm
> guessing in this particular instance, the point of the null is to make
> string operations cut off after "anonymous", however string operations that
> respect the zval string length aren't going to do this.
>
> e.g. `var_dump()` the class name is put through sprintf and it cuts off at
> the null, but get_class or ReflectionClass::getName() just returns the
> original string, and exposes the implementation details.

---

## #89122 — Steven Hilder — 2015-11-09T15:27:14+00:00

URL: https://externals.io/message/89079#89122 · In reply to: #89096

> On 5 November 2015 at 14:59, Rowan Collins <rowan.collins@gmail.com>
> wrote:
> > […]
>
> That doesn't mean it's a good approach (*cough* namespaces *cough*), and
> these bits of "magic" are supposed to be hidden away from users. I'm
> guessing in this particular instance, the point of the null is to make
> string operations cut off after "anonymous", however string operations
> that respect the zval string length aren't going to do this.
>
> e.g. `var_dump()` the class name is put through sprintf and it cuts off at
> the null, but get_class or ReflectionClass::getName() just returns the
> original string, and exposes the implementation details.

Internal names for anonymous classes need to be unique.

The current method of ensuring uniqueness ('\0' + filename + lexer pos) was
written by Dmitry[1], following from Joe's original implementation[2]. As I
understand it, this was supposed to be entirely hidden from userland; hence
the null byte.

So, I prepared a patch for `get_class()` and `ReflectionClass::getName()`,
which in my view should behave the same way as `var_dump()` etc., but I've
now realised that ignoring the unique suffix from the class name will break
functionality that is otherwise desirable:

* Aliasing an anonymous class... (see bug70106[3])

     $instance = new class {};
     $class_name = get_class($instance);
     class_alias($class_name, 'MyAlias');

* Creating a new anonymous class instance dynamically...

     $instance = new class {};
     $class_name = get_class($instance);
     $new_instance = new $class_name;

     ...although the `get_class()` used here isn't necessary, and you could
     write `= new $instance;` without ever knowing $instance's class.

Our options seem to be:

(A) Leave everything as it is. This puts us in a situation where anonymous
     class names are treated inconsistently in userland; a situation that I
     think could lead to considerable confusion about the "real" names of
     these classes and how they should be used.

(B) Patch `get_class()` and `ReflectionClass::getName()` (any others?) to
     strip the suffix (the '\0' and everything after it) from anonymous  
class
     names prior to display. This prevents userland code from distinguishing
     between multiple anonymous classes; breaking functionality as described
     above. To mitigate this, we could allow `class_alias()` to accept  
either
     a class name OR an instance in the first argument. The `new` operator
     already supports an instance in place of a class name.

(C) Assuming that userland SHOULD have access to the internal names of
     anonymous classes, remove the null byte so that the full names are
     displayed everywhere. The drawback of this option is that class names
     won't be as predictable. It occurs to me that a different naming scheme
     could help here - wouldn't a monotonically increasing integer suffix be
     enough? The current scheme seems like an awful waste of memory, too.

Option (B) seems like the best to me, because I can't really see a need to
know the unique suffix -- provided any use-cases are catered for by an
alternative (modifying `class_alias()` should be enough).

Thoughts?

Steve

[1] http://git.php.net/?p=php-src.git;a=commitdiff;h=2a1b0b1#patch1
[2] http://git.php.net/?p=php-src.git;a=commitdiff;h=49608e0#patch12
[3] https://bugs.php.net/bug.php?id=70106

---

## #89124 — Niklas Keller — 2015-11-09T16:42:26+00:00

URL: https://externals.io/message/89079#89124 · In reply to: #89122

2015-11-09 16:27 GMT+01:00 Steven Hilder <
steven.hilder@sevenpercent.solutions>:

> 
>
> > […]
>
> Internal names for anonymous classes need to be unique.
>
> The current method of ensuring uniqueness ('\0' + filename + lexer pos) was
> written by Dmitry[1], following from Joe's original implementation[2]. As I
> understand it, this was supposed to be entirely hidden from userland; hence
> the null byte.
>
> So, I prepared a patch for `get_class()` and `ReflectionClass::getName()`,
> which in my view should behave the same way as `var_dump()` etc., but I've
> now realised that ignoring the unique suffix from the class name will break
> functionality that is otherwise desirable:
>
> * Aliasing an anonymous class... (see bug70106[3])
>
>     $instance = new class {};
>     $class_name = get_class($instance);
>     class_alias($class_name, 'MyAlias');
>
> * Creating a new anonymous class instance dynamically...
>
>     $instance = new class {};
>     $class_name = get_class($instance);
>     $new_instance = new $class_name;
>
>     ...although the `get_class()` used here isn't necessary, and you could
>     write `= new $instance;` without ever knowing $instance's class.
>
> Our options seem to be:
>
> (A) Leave everything as it is. This puts us in a situation where anonymous
>     class names are treated inconsistently in userland; a situation that I
>     think could lead to considerable confusion about the "real" names of
>     these classes and how they should be used.
>
> (B) Patch `get_class()` and `ReflectionClass::getName()` (any others?) to
>     strip the suffix (the '\0' and everything after it) from anonymous
> class
>     names prior to display. This prevents userland code from distinguishing
>     between multiple anonymous classes; breaking functionality as described
>     above. To mitigate this, we could allow `class_alias()` to accept
> either
>     a class name OR an instance in the first argument. The `new` operator
>     already supports an instance in place of a class name.
>
> (C) Assuming that userland SHOULD have access to the internal names of
>     anonymous classes, remove the null byte so that the full names are
>     displayed everywhere. The drawback of this option is that class names
>     won't be as predictable. It occurs to me that a different naming scheme
>     could help here - wouldn't a monotonically increasing integer suffix be
>     enough? The current scheme seems like an awful waste of memory, too.
>
> Option (B) seems like the best to me, because I can't really see a need to
> know the unique suffix -- provided any use-cases are catered for by an
> alternative (modifying `class_alias()` should be enough).
>
> Thoughts?
>
> Steve
>
> [1] http://git.php.net/?p=php-src.git;a=commitdiff;h=2a1b0b1#patch1
> [2] http://git.php.net/?p=php-src.git;a=commitdiff;h=49608e0#patch12
> [3] https://bugs.php.net/bug.php?id=70106
>

Having the path info is quite useful for debugging purposes.

Regards, Niklas

---

## #89125 — Leigh — 2015-11-09T16:46:09+00:00

URL: https://externals.io/message/89079#89125 · In reply to: #89124

>
> Having the path info is quite useful for debugging purposes.
>
> Regards, Niklas
>

It is, but it will always still be available from
ReflectionClass::getFileName()

---

## #89126 — Leigh — 2015-11-09T16:48:57+00:00

URL: https://externals.io/message/89079#89126 · In reply to: #89122

On 9 November 2015 at 15:27, Steven Hilder
>
>
> So, I prepared a patch for `get_class()` and `ReflectionClass::getName()`,
> which in my view should behave the same way as `var_dump()` etc., but I've
> now realised that ignoring the unique suffix from the class name will break
> functionality that is otherwise desirable:
>

Can you share your patch?

It should be possible to return the sanitised name without removing it
internally.(would have to cover the get_parent_class and getParentClass
versions too, and have a quick audit of other places class names can be
spat out.)

Can we at least get the memory address hidden (non-debug builds only
perhaps? - var_dump appends a #n why not just use this number.)

---

## #89128 — Niklas Keller — 2015-11-09T17:48:30+00:00

URL: https://externals.io/message/89079#89128 · In reply to: #89125

2015-11-09 17:46 GMT+01:00 Leigh <leight@gmail.com>:

>
> 
>
> > […]
>
> It is, but it will always still be available from
> ReflectionClass::getFileName()
>

But most implementations just use something like:

$type = is_object($x) ? get_class($x) : gettype($x);

This results in non-useful debugging info when the path gets stripped.

---

## #89132 — Derick Rethans — 2015-11-09T21:46:24+00:00

URL: https://externals.io/message/89079#89132 · In reply to: #89079

> I discovered today that anonymous class names contain a null byte 
> right after "class@anonymous". I don't think class names should 
> contain non-printable characters.
> 
> How about removing that null byte?

I would suggest replacing it with something else. As I am making this 
work for Xdebug, I really need to ship the full name to the IDE so that 
they can later do look-ups on anonymous class properties, and with the 
\0 the IDEs seem to fuck this up. I suggest using another @ instead of 
the \0.

cheers,
Derick
-- 
http://derickrethans.nl | http://xdebug.org
Like Xdebug? Consider a donation: http://xdebug.org/donate.php
twitter: @derickr and @xdebug
Posted with an email client that doesn't mangle email: alpine

---

## #89133 — Adam Harvey — 2015-11-09T23:51:47+00:00

URL: https://externals.io/message/89079#89133 · In reply to: #89132

> 
>
> > […]
>
> I would suggest replacing it with something else. As I am making this
> work for Xdebug, I really need to ship the full name to the IDE so that
> they can later do look-ups on anonymous class properties, and with the
> \0 the IDEs seem to fuck this up. I suggest using another @ instead of
> the \0.

Just to add another voice: +1 for this (or something similar). Having
the \0 can be worked around, but just not having to deal with it at
all is a hell of a lot easier for those of us working on third party
tooling.

Adam

---

## #89157 — Steven Hilder — 2015-11-10T22:04:12+00:00

URL: https://externals.io/message/89079#89157 · In reply to: #89126

> On 9 November 2015 at 15:27, Steven Hilder
> Can you share your patch?

See  
https://github.com/php/php-src/compare/master...stevenhilder:hide-anon-class-suffix

Feedback very welcome :)

> It should be possible to return the sanitised name without removing it
> internally.(would have to cover the get_parent_class and getParentClass
> versions too, and have a quick audit of other places class names can be
> spat out.)

Yes, that's what I had in mind. The class names are sanitised only for
display in userland; I haven't changed the way that the names are generated
or stored internally.

I've applied the change to `get_parent_class()` and `get_called_class()`.
Nothing needed to be done for `ReflectionClass::getParentClass()` because  
it
returns another `ReflectionClass` object, not the parent's name.

The `__CLASS__` magic constant and `::class` syntax also needed changes.  
The
patch includes a test which shows all of the scenarios that I've accounted
for. Please do let me know if you can think of any others.

I've also implemented the change to `class_alias()` as I described in my
previous email.

> Can we at least get the memory address hidden (non-debug builds only
> perhaps? - var_dump appends a #n why not just use this number.)

It isn't possible to use the number that `var_dump()` shows, because the
names for anonymous classes are generated during compile-time, so the  
handle
isn't available yet.


> 
> > […]
>
> I would suggest replacing it with something else. As I am making this  
> work
> for Xdebug, I really need to ship the full name to the IDE so that they
> can later do look-ups on anonymous class properties, and with the \0 the
> IDEs seem to fuck this up. I suggest using another @ instead of the \0.

Derick - forgive me, I'm not at all familiar with how Xdebug+IDE  
integration
works, but can you please elaborate a little on the following...?

* Since Xdebug is an extension, why does the display of class names in
   userland bear any impact?

* If IDEs had access to the filename portion of the class name, how would
   that help them to lookup that specific class (since there could be many
   anonymous classes defined in that one file)?

* Even if IDEs had access to the full anonymous class name - including the
   (unpredictable) memory address of the lexer position at compile-time, how
   would an IDE have any better ability to lookup the correct class?

Kind regards,
Steve

---

## #89163 — Andrea Faulds — 2015-11-11T01:06:32+00:00

URL: https://externals.io/message/89079#89163 · In reply to: #89132

Hi Derick,

Derick Rethans wrote:
> I would suggest replacing it with something else. As I am making this
> work for Xdebug, I really need to ship the full name to the IDE so that
> they can later do look-ups on anonymous class properties, and with the
> \0 the IDEs seem to fuck this up. I suggest using another @ instead of
> the \0.

This is mostly tangential, but I note that we use "{closure}" as the 
name of anonymous functions. So, maybe it should be "{class}@..." for 
anonymous classes? It's at least semi-consistent...

Thanks.
-- 
Andrea Faulds
http://ajf.me/

---

## #89166 — Steven Hilder — 2015-11-11T02:13:28+00:00

URL: https://externals.io/message/89079#89166 · In reply to: #89163

> This is mostly tangential, but I note that we use "{closure}" as the name
> of anonymous functions. So, maybe it should be "{class}@..." for  
> anonymous
> classes? It's at least semi-consistent...

Hi Andrea,

Only slightly tangential - I'm interested in your input on this issue, and
I'm keen to get this sorted out before 7.0 launches, so that we don't get
stuck with something suboptimal until 8.0 because of BC concerns.

To be honest, the original meaning of the class names has been lost since
Phil and Joe's RFC[1]. It had originally been `class@0x[addr]`, which had a
reasonable semantic interpretation of "the (otherwise anonymous) class  
which
can be found at the address [addr]". Dmitry added[2] the null byte and the
filename, but he did so between the '@' and the '0x[addr]', with  
'anonymous'
added (just for good measure?). At this point, the purpose of the  
'@'-symbol
was gone, in my opinion.

Furthermore, the RFC allowed for the portion of the name before the '@' to
identify the parent class:

     class mine {}
     new class extends mine {};

     This class name will be “mine@0x7fc4be471000”.

This behaviour was present in Joe's original implementation, but later
removed[3] by Nikita. His commit message says that this was "dead code",  
but
I don't see what was wrong with it.

The result of all of this is that we have a naming convention for anonymous
classes that is not only inconsistent with an existing feature of the
language, but has little in the way of reasoning behind its current format.

My patch[4] aims to save userland from this mess by hiding the entire  
suffix
wherever anonymous classes' names are to be displayed. This means that all
anonymous classes will show as "class@anonymous" (which I'd happily change
to "{class}" in keeping with "{closure}") in userland, with no way of
retrieving the internal name; not even using `ReflectionClass`. My  
reasoning
is that users should never need to know the underlying names; if a class  
has
a distinct name, then it's not really *anonymous*, is it?

Kind regards,
Steve

[1] https://wiki.php.net/rfc/anonymous_classes#internal_class_naming
[2] http://git.php.net/?p=php-src.git;a=commitdiff;h=2a1b0b1#patch1
[3] http://git.php.net/?p=php-src.git;a=commitdiff;h=4a7061b#patch1
[4]  
https://github.com/php/php-src/compare/master...stevenhilder:hide-anon-class-suffix

---

## #89172 — Nikita Popov — 2015-11-11T10:01:15+00:00

URL: https://externals.io/message/89079#89172 · In reply to: #89157

On Tue, Nov 10, 2015 at 11:04 PM, Steven Hilder <
steven.hilder@sevenpercent.solutions> wrote:


>
> > […]
>
> See
> https://github.com/php/php-src/compare/master...stevenhilder:hide-anon-class-suffix
>
> Feedback very welcome :)
>
> It should be possible to return the sanitised name without removing it
> > […]
>
> Yes, that's what I had in mind. The class names are sanitised only for
> display in userland; I haven't changed the way that the names are generated
> or stored internally.
>
> I've applied the change to `get_parent_class()` and `get_called_class()`.
> Nothing needed to be done for `ReflectionClass::getParentClass()` because
> it
> returns another `ReflectionClass` object, not the parent's name.
>
> The `__CLASS__` magic constant and `::class` syntax also needed changes.
> The
> patch includes a test which shows all of the scenarios that I've accounted
> for. Please do let me know if you can think of any others.
>
> I've also implemented the change to `class_alias()` as I described in my
> previous email.
>

I'd like to clarify some things here from the perspective of the people who
implemented this naming scheme.

The naming was chosen to be this way intentionally, to combine two
behaviors we would like to have:

a) Things like `get_class()` MUST return a class name that uniquely
identifies the anonymous class. You should be able to instantiate a new
instance of the same class based on it, etc. Having these return truncated
names is an absolute no-go, because it would require people to explicitly
handle anonymous classes in many context that currently "just work".

b) We do not want to display the fully mangled name of an anonymous class
in error messages or debug dumps. While the information contained there can
be useful, it also significantly bloats outputs. Furthermore we will likely
change the exact naming structure in the future, in which case the
anonymous class name will no longer contain any directly useful information
(maybe some hash).

Using a NUL byte conveniently achieves both of these goals. We did not
however take into account that this would cause issues with 3rd party
tooling that does not support binary data.

As such, we may have to change the naming. However we must retain goal a)
and would like to retain b). To do this we could use names along the lines
of class@anonymous$1, where the uniquely-identifying information is short
enough to be no bother when displayed. Generating these kinds of names is
non-trivial if you consider caching and may require some changes to the way
we perform class bindings. (As a side bonus, depending on how this is done,
we might fix https://bugs.php.net/bug.php?id=64291 as well.)

Not sure if this is still in scope for PHP 7.0.

Nikita

---

## #89174 — Rowan Collins — 2015-11-11T10:57:49+00:00

URL: https://externals.io/message/89079#89174 · In reply to: #89166

Steven Hilder wrote on 11/11/2015 02:13:
> This behaviour was present in Joe's original implementation, but later
> removed[3] by Nikita. His commit message says that this was "dead 
> code", but
> I don't see what was wrong with it. 

It looks to me like 'decl->name' will never be set for an anonymous 
class, so the old code was just passing a null into 
zend_name_anon_class(), which is pointless (hence "dead" code, i.e. 
"unreachable").

I guess what it should have been passed (maybe was in previous 
revisions) was extends_name, which is only extracted from the AST much 
further down the function. 
http://git.php.net/?p=php-src.git;a=blob;f=Zend/zend_compile.c;hb=HEAD#l5315

---

## #89178 — Derick Rethans — 2015-11-11T13:44:33+00:00

URL: https://externals.io/message/89079#89178 · In reply to: #89172

> Using a NUL byte conveniently achieves both of these goals. We did not 
> however take into account that this would cause issues with 3rd party 
> tooling that does not support binary data.

I withdraw my comment. It does not cause issues for me (anymore). Having 
the NUL is actually making things easier.

cheers,
Derick

---

## #89197 — Niklas Keller — 2015-11-12T13:46:10+00:00

URL: https://externals.io/message/89079#89197 · In reply to: #89178

2015-11-11 14:44 GMT+01:00 Derick Rethans <derick@php.net>:

> 
>
> > […]
>
> I withdraw my comment. It does not cause issues for me (anymore). Having
> the NUL is actually making things easier.
>
> cheers,
> Derick
>

Semi-Related: NUL is especially an issue in exception messages in
combination with get_class, because they truncate the messages:
https://bugs.php.net/bug.php?id=70894

Regards, Niklas