Fixed crash that occurred when a promoted constructor parameter had an attribute #5891

qxzkjp · 2020-07-25T11:13:36Z

When a class constructor parameter had both a visibility modifier (promoting it to a class property) and an annotation with at least one argument, the PHP parser would crash when attempting to compile it.

This was caused by the attribute AST being used twice, and was fixed by creating a temporary copy of it (and destroying said copy) when neccesary.

Bug report: https://bugs.php.net/bug.php?id=79897

Minimal reproduction (warning: this will terminate your PHP process):


<<Attribute>>
class B {
    public function __construct($value)
    {
    }
}

class A {
    public function __construct(
        <<B(12)>> public $b
    )
    {
    }
}

carusogabriel · 2020-07-25T11:54:55Z

@qxzkjp Can you add a .phpt test case, please?

qxzkjp · 2020-07-25T12:58:08Z

Of course. I had a look over the other unit tests to see what sort of assertions to make, but I couldn't find any other crashing bug tests. I decided to just put an echo at the end of the file and test for the output from that. I hope that's an acceptable way to write the test, let me know if it's not.

beberlei

Two coding style nitpicks

beberlei · 2020-07-27T18:27:09Z

Zend/zend_compile.c

+		zend_ast_ref *attributes_copy = 0;
+
+		if (visibility && attributes_ast)
+		{


can you put this on the same line as the if?

beberlei · 2020-07-27T18:27:18Z

Zend/zend_compile.c

+			attributes_copy = zend_ast_copy(attributes_ast);
+		}
+
+


can you remove this space?

I'm assuming it was literally just this one blank line, not one of them? If so, it's done.

…n annotation This was caused by the attribute AST being used twice, and was fixed by creating a temporary copy of it (and destroying said copy) when neccesary.

qxzkjp · 2020-07-28T21:47:15Z

If anyone saw the pipeline failure, that was me being caught out by the new attribute syntax changes being merged into master. The tests ran fine on my local machine, but the test server is always using the tip of master to parse the phpt files (apparently). So the old << syntax caused a test failure.

I've done a rebase onto master and updated the test. The changes have been squashed down into two commits: one implementing the fix, one adding the test.

nikic

This is a simple solution, but I'm not sure it is correct. zend_ast_copy is a bit badly named, it's not a plain AST copy, it's a copy from compile-time to run-time AST. At the least, I expect this to cause issues if the argument contains declarations, like <<A(function() {})>>. That's illegal, but will be rejected only later and will probably crash the copying code.

The solution I had in mind for this was to only compile the attribute once, and then copy the compiled attribute structure.

Zend/zend_compile.c

Co-authored-by: Nikita Popov <nikita.ppv@googlemail.com>

qxzkjp · 2020-07-29T09:25:34Z

For the record, @nikic , I tested your code snippet and it returns Fatal error: Constant expression contains invalid operations when used as a class attribute, a plain parameter attribute, and a promoted parameter attribute. So the copying code seems to not mind invalid ASTs (at least not that invalid AST). I leave it to the other experienced contributors to evaluate your concerns more broadly.

As far as copying the attributes goes, there does not yet seem to be a function to copy attributes (makes sense, why would there be). Would it be worth my time creating a second pull request (as an alternative to this) to implement that, or is it a big enough change that you'd rather a regular contributor did it?

nikic

After trying various edge cases and running some fuzz tests, it does seem like this works, so let's go with it.

nikic · 2020-07-29T14:21:41Z

Zend/tests/bug79897/bug79897.phpt

+    }
+}
+
+echo "file compiled successfully";


It would be more useful to check the actual attribute arguments here:

var_dump((new ReflectionParameter(['A', '__construct'], 'b'))->getAttributes()[0]->getArguments()); var_dump((new ReflectionProperty('A', 'b'))->getAttributes()[0]->getArguments());

Also, please add a closing ?> here and drop the extra directory from the test (just Zend/tests/bug79897.phpt is enough).

Alright, I've made the requested changes to this test. It definitely feels better to have the assertion actually asserting something specific 👍

Assertions now check the values that the property and parameter attributes were initialised with

…ix-attributes-bug

nikic · 2020-08-03T14:10:40Z

This got merged in f475edc, looks like I forgot to close the PR.

carusogabriel added the Bug label Jul 25, 2020

qxzkjp force-pushed the fix-attributes-bug branch from 6b15fc0 to 08433c9 Compare July 27, 2020 16:45

beberlei suggested changes Jul 28, 2020

View reviewed changes

qxzkjp added 2 commits July 28, 2020 22:40

Fixed crash that occurred when a promoted constructor parameter had a…

011e899

…n annotation This was caused by the attribute AST being used twice, and was fixed by creating a temporary copy of it (and destroying said copy) when neccesary.

Added unit test for bug 79897

9e8cd20

qxzkjp force-pushed the fix-attributes-bug branch from 2f7a388 to 9e8cd20 Compare July 28, 2020 21:43

nikic reviewed Jul 29, 2020

View reviewed changes

Zend/zend_compile.c Outdated Show resolved Hide resolved

qxzkjp force-pushed the fix-attributes-bug branch from 198eb54 to 9e8cd20 Compare July 29, 2020 09:16

Changed 0 to NULL for pointer value

309a896

Co-authored-by: Nikita Popov <nikita.ppv@googlemail.com>

nikic reviewed Jul 29, 2020

View reviewed changes

qxzkjp added 2 commits July 29, 2020 17:13

Changed test to make more useful assertions

7b0c1b9

Assertions now check the values that the property and parameter attributes were initialised with

Merge branch 'fix-attributes-bug' of github.com:qxzkjp/php-src into f…

75a6945

…ix-attributes-bug

nikic approved these changes Jul 29, 2020

View reviewed changes

nikic closed this Aug 3, 2020

Fixed crash that occurred when a promoted constructor parameter had an attribute #5891

Fixed crash that occurred when a promoted constructor parameter had an attribute #5891

Uh oh!

Conversation

qxzkjp commented Jul 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carusogabriel commented Jul 25, 2020

Uh oh!

qxzkjp commented Jul 25, 2020

Uh oh!

beberlei left a comment

Choose a reason for hiding this comment

Uh oh!

beberlei Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

qxzkjp Jul 28, 2020

Choose a reason for hiding this comment

Uh oh!

beberlei Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

qxzkjp Jul 28, 2020

Choose a reason for hiding this comment

Uh oh!

qxzkjp commented Jul 28, 2020

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

qxzkjp commented Jul 29, 2020

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

nikic Jul 29, 2020

Choose a reason for hiding this comment

Uh oh!

qxzkjp Jul 29, 2020

Choose a reason for hiding this comment

Uh oh!

nikic commented Aug 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

qxzkjp commented Jul 25, 2020 •

edited

Loading