[Observer] Provide unused retvals to observers #6422

SammyK · 2020-11-11T23:03:37Z

This PR ensures that if a retval is not used, it is still available to observer end handlers.

nikic · 2020-11-12T13:24:18Z

Zend/zend_vm_def.h

@@ -4304,7 +4304,7 @@ ZEND_VM_INLINE_HANDLER(62, ZEND_RETURN, CONST|TMP|VAR|CV, ANY, SPEC(OBSERVER))
 		}
 	}
 	ZEND_OBSERVER_SAVE_OPLINE();
-	ZEND_OBSERVER_FCALL_END(execute_data, return_value);
+	ZEND_OBSERVER_FCALL_END(execute_data, retval_ptr);


I think this may use after free for the IS_VAR REFERENCE case.

Ah no, retval_ptr is derefed in that case. However the IS_CV case looks problematic, because the value gets moved into return_value there and retval_ptr is changed to a NULL zval.

Ah, nice catch. I was scanning for dtor's and totally missed that ZVAL_NULL(retval_ptr) for IS_CV. I'll fix that and add some more comprehensive tests.

I just added a fix for the IS_CV + refcounted return case and added more tests for various VM var types. Thanks again for catching that one!

Also add loads more tests for various VM var types

nikic · 2020-11-12T19:10:52Z

Zend/zend_vm_def.h

@@ -4248,13 +4248,17 @@ ZEND_VM_INLINE_HANDLER(62, ZEND_RETURN, CONST|TMP|VAR|CV, ANY, SPEC(OBSERVER))
 		if (return_value) {
 			ZVAL_NULL(return_value);
 		}
+		ZEND_OBSERVER_SAVE_OPLINE();
+		ZEND_OBSERVER_FCALL_END(execute_data, retval_ptr);


Might it make sense to just move this to the very start of the function, rather than duplication it in the branches?

That would make sense and would be simpler. But the main issue is that any errors that are raised in the return handler would trigger after the end handler had fired.

For example, the output from observer_retval_05.phpt would change to this:

<foo> + </foo:NULL> Warning: Undefined variable $i_do_not_exist in %s on line %d - </foo:NULL>

For the PHP tracer extension I'm working on, that would mean that we couldn't attach the E_WARNING to the open foo span that raised the error.

SammyK · 2020-11-16T16:18:52Z

@nikic Do you have any other issues with this PR? I'm planning on merging this first thing tomorrow morning in time for the RC5 tag.

nikic · 2020-11-16T17:25:04Z

Zend/zend_vm_def.h

@@ -4365,7 +4375,7 @@ ZEND_VM_COLD_CONST_HANDLER(111, ZEND_RETURN_BY_REF, CONST|TMP|VAR|CV, ANY, SRC,
 		FREE_OP1_VAR_PTR();
 	} while (0);

-	ZEND_OBSERVER_FCALL_END(execute_data, EX(return_value));
+	ZEND_OBSERVER_FCALL_END(execute_data, retval_ptr);


This looks like a use after free for all the !EX(return_value) cases above, that already free the value.

Very good catch. I didn't realize that freeing OP1 was freeing the retval from the opline. I fixed this in 0fa78a5.

nikic · 2020-11-16T17:26:37Z

ext/zend_test/tests/observer_retval_07.phpt

+--FILE--
+<?php
+function &getMessage() {
+    $retval = 'I should be observable';


This needs to be something refcounted to check for use after free.

A refcounted retval is being tested in observer_retval_04.phpt. In this test I was targeting this branch. Do you think we have sufficient coverage with all the retval tests?

nikic · 2020-11-16T17:28:02Z

Zend/zend_vm_def.h

 		zend_generator_close(generator, 1);
 		ZEND_VM_RETURN();
 	} else {
+		if (zend_observer_fcall_op_array_extension != -1) {
+			zend_observer_fcall_end(execute_data, EX(return_value));


This looks pretty fishy to me, especially in conjunction with the EX(return_value) initialization below.

You're absolutely right. Retval will always be null with uncaught exceptions. I don't know what I was thinking on this one. 🤦 I reverted this commit in 9332095 and added a proper fix in 95f943a.

This reverts commit d29998d.

SammyK · 2020-11-16T19:19:38Z

Thanks for another round of review on this @nikic! I think I addressed the main concerns. I also squeezed in two more tests (51d8fd9) to make sure observability was still possible during shutdown.

nikic · 2020-11-16T23:10:21Z

Zend/zend_vm_def.h

@@ -4301,10 +4311,10 @@ ZEND_VM_INLINE_HANDLER(62, ZEND_RETURN, CONST|TMP|VAR|CV, ANY, SPEC(OBSERVER))
 			} else {
 				ZVAL_COPY_VALUE(return_value, retval_ptr);
 			}
+			ZEND_OBSERVER_SAVE_OPLINE();
+			ZEND_OBSERVER_FCALL_END(execute_data, retval_ptr);


I think there's still a potential use-after-free here. If retval_ptr is a reference with refcount one, then it will be freed above, but (the new) retval_ptr will still point into it.

Generally I think that you should just do

ZEND_OBSERVER_SAVE_OPLINE(); ZEND_OBSERVER_FCALL_END(execute_data, return_value);

one at the end up the whole if (return_value) branch. There's no need to handle these differently in each case.

nikic · 2020-11-16T23:12:40Z

Just to offer a possible alternative that might be less fragile, you could do something like this:

zval observer_retval;
if (observer && !return_value) {
    return_value = &observer_retval;
}
...
if (observer && return_value == &observer_retval) {
    zval_ptr_dtor_nogc(&observer_retval);
}

I.e. pretend we always want a return_value if observer if used. Less efficient, but maybe more straightforward?

SammyK · 2020-11-17T00:30:04Z

Your suggestion is so much better than trying to follow all the branches. Thanks @nikic! 💯 I've applied your suggestion in 7b6572f. If this looks good to you, I'll merge this first thing in the morning Pacific Time in time for the RC5 tag.

Provide unused retvals to observers

226d9f3

nikic reviewed Nov 12, 2020

View reviewed changes

Fix IS_CV + refcounted return case

69061b9

Also add loads more tests for various VM var types

nikic reviewed Nov 12, 2020

View reviewed changes

SammyK added 2 commits November 12, 2020 14:07

Call observer end handler before unused refcounted IS_VAR is freed

836ef0b

Pass correct retval from generator on uncaught exception

d29998d

SammyK mentioned this pull request Nov 13, 2020

[PHP 8] Second custom pre-RC5 build DataDog/dd-trace-php#1096

Merged

3 tasks

SammyK mentioned this pull request Nov 16, 2020

[Observer] Fire open observer end handlers after a zend_bailout #6377

Closed

nikic reviewed Nov 16, 2020

View reviewed changes

SammyK added 4 commits November 16, 2020 10:28

Revert "Pass correct retval from generator on uncaught exception"

9332095

This reverts commit d29998d.

Pass NULL to end handler on uncaught exception

95f943a

Use retval by ref before it is freed

0fa78a5

Add test for observability during shutdown

51d8fd9

nikic reviewed Nov 16, 2020

View reviewed changes

Add observer_retval to observe the return_value

7b6572f

nikic approved these changes Nov 17, 2020

View reviewed changes

php-pulls closed this in 58d41b8 Nov 17, 2020

[Observer] Provide unused retvals to observers #6422

[Observer] Provide unused retvals to observers #6422

Uh oh!

Conversation

SammyK commented Nov 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SammyK commented Nov 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SammyK commented Nov 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic commented Nov 16, 2020

Uh oh!

SammyK commented Nov 17, 2020

Uh oh!

Uh oh!