Skip to content

Conversation

TysonAndre
Copy link
Contributor

@TysonAndre TysonAndre commented Aug 31, 2021

Strangely, uses of eval and 'php -a' will not treat non-FQ true/false/null as magic
keywords, while compiled php required from a file would do that.

Currently, opcache assumes that the non-FQ true/false/null constant reference is safe to convert to the literal, even though it may also be defined in the same namespace. See https://github.com/php/php-src/blob/php-8.1.0beta3/Zend/Optimizer/block_pass.c#L32-L52

This may confuse people learning the language, and result in code loaded with
eval() behaving differently from the same snippet in a file loaded by require.

Interactive shell

php > define('foo\true', 'test');
php > namespace foo { var_dump(true); }
string(4) "test"

This PR will make the same session instead properly emit bool(true); like it already would if running those statements in files. (Interactive shell internally uses the same APIs eval uses)

  • https://3v4l.org/ppBdV reproduces this (expected all lines to be bool(true)). I couldn't find related bugs in a quick search on bugs.php.net
  • Earlier, I thought it was due to eval. But that doesn't seem to be the case (it also happens when there are multiple files), it's probably related to the constant not existing when the file is loaded.

zend_resolve_non_class_name is a static C function where case_sensitive is only true for global constants (and false for global functions).

This targets php 8.1 - I'm not certain if it makes sense to fix this in earlier versions as well, if anything using eval() somehow relies on this.

@nikic
Copy link
Member

nikic commented Sep 1, 2021

Why is the resolution in eval() different?

@TysonAndre TysonAndre changed the title Fix inconsistency in true/false/null constant resolution when eval() is used Fix inconsistency in true/false/null constant resolution when opcache is used Sep 1, 2021
@TysonAndre TysonAndre changed the title Fix inconsistency in true/false/null constant resolution when opcache is used Fix inconsistency in true/false/null constant resolution when opcache is not used Sep 1, 2021
@TysonAndre
Copy link
Contributor Author

TysonAndre commented Sep 1, 2021

Why is the resolution in eval() different?

It seems like that was a red herring - the resolution appears to be different when the constant is defined before the file is loaded, AND opcache is not used (neither opcache nor the optimizer is used for eval right now, I think?)

The constant resolution code checks if there are constants in the namespace (for non-FQ false, true, null) before trying the fallback zend_get_special_const

That's because opcache has a pass that assumes these non-FQ constant names should always be the literals. https://github.com/php/php-src/blob/php-8.1.0beta3/Zend/Optimizer/block_pass.c#L32-L52

  • So I propose always doing effectively the same thing as that pass in this PR, whether or not opcache is used.
<?php // test.php
namespace foo {
define('foo\true', 'test');
require_once __DIR__ . '/other.php';
}
<?php // other.php
namespace foo {
var_dump(true);
}

Old behavior (proposed behavior is to always result in bool(true))

» php -d opcache.enable=0 test.php
string(4) "test"
» php -d opcache.enable=1 test.php
bool(true)

@nikic
Copy link
Member

nikic commented Sep 1, 2021

Shouldn't this be fixed by moving the zend_get_special_const() call in zend_try_ct_eval_const() first, before the lookup?

@TysonAndre TysonAndre force-pushed the fix-constant-resolution branch from 5619251 to b061002 Compare September 1, 2021 16:54
@TysonAndre
Copy link
Contributor Author

Shouldn't this be fixed by moving the zend_get_special_const() call in zend_try_ct_eval_const() first, before the lookup?

That seems to work - all callers of zend_resolve_const_name immediately call zend_try_ct_eval_const. Tests of the new behavior continue to pass

…is used

Strangely, uses of eval and 'php -a' will not treat non-FQ true/false/null as magic
keywords, while compiled php required from a file would do that.

This may confuse people learning the language, and result in code loaded with
eval() behaving differently from the same snippet in a file loaded by require.

```
Interactive shell

php > define('foo\true', 'test');
php > namespace foo { var_dump(true); }
string(4) "test"
```

This will make the same session instead properly emit `bool(true);` like it
already would if running those statements in files.
(Interactive shell internally uses the same APIs eval uses)

`zend_resolve_non_class_name` is a static C function where case_sensitive
is only true for global constants (and false for global functions).
@TysonAndre TysonAndre force-pushed the fix-constant-resolution branch from b061002 to bdd72d1 Compare September 2, 2021 14:04
@TysonAndre TysonAndre merged commit 4c48fd2 into php:PHP-8.1 Sep 3, 2021
@TysonAndre TysonAndre deleted the fix-constant-resolution branch September 3, 2021 12:42
TysonAndre added a commit to TysonAndre/phan that referenced this pull request Sep 3, 2021
php/php-src#7441 fixes other inconsistencies in
8.1 found while discovering this.
nikic pushed a commit that referenced this pull request Nov 9, 2021
… is not used (#7441)

Strangely, uses of eval and 'php -a' (or loading a file without opcache after a namespaced constant was declared)
will not treat non-FQ true/false/null as magic keywords, while compiled php required from a file would do that.

This may confuse people learning the language, and result in code loaded with
eval() behaving differently from the same snippet in a file loaded by require.

```
Interactive shell

php > define('foo\true', 'test');
php > namespace foo { var_dump(true); }
string(4) "test"
```

This will make the same session instead properly emit `bool(true);` like it
already would if running those statements in files when opcache was used.

(cherry picked from commit 4c48fd2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants