Fix GH-8841: error on return type check of func declaration isn't removing the function from function_table

[alexandre-daubois]

At compile time, if the return type check fails during a function declaration, then the function isn't removed from the global function_table in SAPI. It causes 2 problems:

• Calling the function even after the failed declaration results in a core dump
• As the function is present in the global function_table, it is not possible to (try to) redefine the function, as function name is considered as "already in use"

Fixes #8841

[cmb69]

Thank you for the PR!

While this would obviously fix the reported issue, I think it is too small in scope, since there are many reasons why the compilation of a function declaration might fail. E.g. function foo() {return $a[];} foo(); would still cause the same segfault.

[alexandre-daubois]

Thank you for your feedback, I'm taking a look to improve the fix!

[alexandre-daubois]

Alright, so I surrounded the AST compilation in zend_compile_func_decl, which is more global. I added a test regarding this change. The one you told me. Makes way more sense now!

As far as I understood, zend_bailout() is like "rethrowing the error" that occured to Zend engine during a zend_try, so the engine can handle it after we do some operations in zend_catch? Like calling a throw in the catch clause in PHP?

[cmb69]

Alright, so I surrounded the AST compilation in zend_compile_func_decl, which is more global.

I think that is much better. Thank you! It seems to be possible to guard the zend_hash_del() call with if (!is_method && toplevel) what might be reasonable to avoid the function call overhead (no big deal either way, though).

I'm not quite sure about zend_register_seen_symbol() in zend_begin_func_decl(). Would we need to undo this as well?

As far as I understood, zend_bailout() is like "rethrowing the error" that occured to Zend engine during a zend_try, so the engine can handle it after we do some operations in zend_catch? Like calling a throw in the catch clause in PHP?

Yeah, it is similar.

[alexandre-daubois]

Thank you for the feedback!
Added the condition, I agree it's better like this.

I'm not quite sure about zend_register_seen_symbol() in zend_begin_func_decl(). Would we need to undo this as well?

If I understand correctly, zend_register_seen_symbol() only takes in account the file context. Conflicting with a previous seen symbol here would mean that the file already contains two function declarations with the same function name?
If this is the case, then I think we can do without the seen_symbols undo, as it is really unlikely to happen and would, I guess, cause an error earlier.

[cmb69]

@alexandre-daubois, I think you're right regarding zend_register_seen_symbol(), so the implementation is fine. I'm not happy with the test, though. While that matches other tests in this directory, I find it unfortunate to not test the behavior with libedit which is affected by the bug, too. So I suggest to use something like the following instead:

sapi/cli/tests/gh8841.phpt

--TEST--
GH-8841: Fix invalid return type compilation doesn't register function
--SKIPIF--
<?php
include "skipif.inc";
if (!extension_loaded('readline')) die("skip need readline support");
if (READLINE_LIB !== "libedit") die('skip libedit only');
?>
--FILE--
<?php
$php = getenv('TEST_PHP_EXECUTABLE');

// disallow console escape sequences that may break the output
putenv('TERM=VT100');

$codes = array();

$codes[1] = <<<EOT
function f(\$x): void { return \$x; }
f(1);
EOT;

$codes[2] = <<<EOT
function f(\$x): void { return \$x; }
function f(\$x): int { return \$x; }
echo f(1);
EOT;

$codes[3] = <<<EOT
function foo() { return \$x[]; }
foo();
EOT;

foreach ($codes as $key => $code) {
    echo "\n--------------\nSnippet no. $key:\n--------------\n";
    $php = getenv('TEST_PHP_EXECUTABLE');
    $ini = getenv('TEST_PHP_EXTRA_ARGS');
    $descriptorspec = [['pipe', 'r'], STDOUT, STDERR];
    $proc = proc_open("$php $ini -a", $descriptorspec, $pipes);
    fwrite($pipes[0], $code);
    fclose($pipes[0]);
    proc_close($proc);
}
?>
--EXPECT--
--------------
Snippet no. 1:
--------------
Interactive shell


Fatal error: A void function must not return a value in php shell code on line 1

Warning: Uncaught Error: Call to undefined function f() in php shell code:1
Stack trace:
#0 {main}
  thrown in php shell code on line 1

--------------
Snippet no. 2:
--------------
Interactive shell


Fatal error: A void function must not return a value in php shell code on line 1
1

--------------
Snippet no. 3:
--------------
Interactive shell


Fatal error: Cannot use [] for reading in php shell code on line 1

Warning: Uncaught Error: Call to undefined function foo() in php shell code:1
Stack trace:
#0 {main}
  thrown in php shell code on line 1

That would either require a separate test for libreadline, or maybe we could unify the test expectation by inserting %A for the lines which are printed with libreadline but not with libedit (that would require to replace --EXPECT-- with --EXPECTF--).

[nikic]

An interactive shell shouldn't be necessary to reproduce this at all -- can't you just use a shutdown function?

This also doesn't seem like the right way to fix the issue: We should be delaying function registration instead.

[nikic]

Here's a simple test: https://3v4l.org/XMg3k

[nielsdos]

This was fixed by an alternative fix in #10989 instead.
So I'll close this now. Thanks for your interest in fixing this anyway!