Delay freeing of overwritten values in assignments #10606

iluuu1994 · 2023-02-16T23:17:03Z

@dstogov Your patch looks good. The path for references was missing (4c580af). This PR also contains the adjustment for write_property. If you think these are too many changes I drop that commit (6694d28).

One alternative solution for write_property would be to utilize EG. delay_garbage would signal that the garbage should be stored in EG(garbage) instead of being released directly. That would avoid the additional parameter. I think that should work too, assuming we can avoid running userland code between storing and reading EG(garbage) that could potentially overwrite it. Edit: Implemented here for comparison: 9c7f0aa

/cc @nielsdos

mvorisek · 2023-02-16T23:44:19Z

Zend/tests/gh10168/assign.phpt

+
+class Test {
+    function __destruct() {
+        $GLOBALS['a'] = null;


in each __destruct I would suggest to add echo "D\n"; at the start to make sure the destruction is really called at the time when expected/needed to test

nielsdos · 2023-02-17T07:19:28Z

I guess this also fixes GH-10582 given the assign_dim_ref test. I'll have a closer look tonight and also run some static analysis tools. Thanks for working on this.

nielsdos

It looks good to me. Since this targets master anyway, my preference goes to the non-EG solution for the write_property case because it is more explicit and feels less dirty.
The only minor nit I have is that some code repetition could be avoided by creating 2 new zend_always_inline functions to cleanup the garbage; because the following two patterns are copied a lot:

if (garbage) {
	if (GC_DELREF(garbage) == 0) {
		rc_dtor_func(garbage);
	} else {
		gc_check_possible_root_no_ref(garbage);
	}
}

and the same for the other case:

if (garbage) {
	if (GC_DELREF(garbage) == 0) {
		rc_dtor_func(garbage);
	} else {
		gc_check_possible_root(garbage);
	}
}

With those helper functions you can always just unconditionally call them and they will perform the checks and cleanups.
Since they would be always inline it shouldn't matter for the codegen.

dstogov · 2023-02-20T20:40:27Z

I definitely don't like 6694d28
Changing object API will break third party extensions.

9c7f0aa seems like a better compromise, but it introduces a "state" and in general some write_property handler may lead to unrelated call(s) to zend_std_write_property() and as result another weird family of bugs.

Similar may be achieved without a "state", but this may be less efficient.

diff --git a/Zend/zend_object_handlers.c b/Zend/zend_object_handlers.c
index 20d773e1d3..cdca0cdf69 100644
--- a/Zend/zend_object_handlers.c
+++ b/Zend/zend_object_handlers.c
@@ -837,8 +837,29 @@ ZEND_API zval *zend_std_write_property(zend_object *zobj, zend_string *name, zva
 			}
 
 found:
-			variable_ptr = zend_assign_to_variable(
-				variable_ptr, value, IS_TMP_VAR, property_uses_strict_types());
+			zend_refcounted *garbage = NULL;
+
+			variable_ptr = zend_assign_to_variable_ex(
+				variable_ptr, value, IS_TMP_VAR, property_uses_strict_types(), &garbage);
+
+			if (garbage) {
+				zend_execute_data *execute_data = EG(current_execute_data);
+
+				if (execute_data
+				 && EX(func)
+				 && ZEND_USER_CODE(EX(func)->common.type)
+				 && EX(opline)
+				 && EX(opline)->opcode == ZEND_ASSIGN_OBJ
+				 && EX(opline)->result_type) {
+					ZVAL_COPY_DEREF(EX_VAR(EX(opline)->result.var), variable_ptr);
+					variable_ptr = NULL;
+				}
+				if (GC_DELREF(garbage) == 0) {
+					rc_dtor_func(garbage);
+				} else {
+					gc_check_possible_root_no_ref(garbage);
+				}
+			}
 			goto exit;
 		}
 		if (Z_PROP_FLAG_P(variable_ptr) == IS_PROP_UNINIT) {
diff --git a/Zend/zend_vm_def.h b/Zend/zend_vm_def.h
index 7323b5d9a1..facdd09611 100644
--- a/Zend/zend_vm_def.h
+++ b/Zend/zend_vm_def.h
@@ -2494,7 +2494,7 @@ ZEND_VM_C_LABEL(fast_assign_obj):
 	}
 
 ZEND_VM_C_LABEL(free_and_exit_assign_obj):
-	if (UNEXPECTED(RETURN_VALUE_USED(opline))) {
+	if (UNEXPECTED(RETURN_VALUE_USED(opline)) && value) {
 		ZVAL_COPY_DEREF(EX_VAR(opline->result.var), value);
 	}
 	FREE_OP_DATA();

All three solutions are not ideal. May be you have any new ideas?

iluuu1994 · 2023-02-21T01:01:32Z

@dstogov Thank you for your suggestion!

zend_execute_data *execute_data = EG(current_execute_data);

Is it safe to assume EG(execute_data) is up to date? Could some handler that doesn't SAVE_OPLINE() trigger zend_std_write_property()? The DO_CALL handlers can (through ReflectionProperty::setValue()) but those do save the opline.

&& EX(func)
&& ZEND_USER_CODE(EX(func)->common.type)

Do we need this check? I think this is only necessary if ZEND_ASSIGN_OBJ can trigger an internal function that calls zend_std_write_property(), like in a destructor of some internal class.

Depending on how often the garbage is owned by the property (i.e. being the only reference) a GC_REFCOUNT(garbage) == 1 might or might not be useful to skip the other checks.

Another idea I tried was to pass the EX_VAR(opline->result.var) through the cache slot but this doesn't work well because zend_get_property_offset assumes member is CONST when a cache slot is passed.

dstogov · 2023-02-21T06:42:13Z

@dstogov Thank you for your suggestion!

zend_execute_data *execute_data = EG(current_execute_data);

Is it safe to assume EG(execute_data) is up to date?

Yes. We have similar check in few other places (e.g. ext/opcache/ZendAccelerator.c)

Could some handler that doesn't SAVE_OPLINE() trigger zend_std_write_property()?

No. write_property() may emit error or throw exception so EX(opline) must be up to date.

The DO_CALL handlers can (through ReflectionProperty::setValue()) but those do save the opline.

I'm not sure what do you mean. Tty to construct a test case.

&& EX(func)
&& ZEND_USER_CODE(EX(func)->common.type)

Do we need this check?

Unfortunately yes. For internal function we don't set EX(opline).

I think this is only necessary if ZEND_ASSIGN_OBJ can trigger an internal function that calls zend_std_write_property(), like in a destructor of some internal class.

Remove these lines, run make test and you'll see the problems.

Depending on how often the garbage is owned by the property (i.e. being the only reference) a GC_REFCOUNT(garbage) == 1 might or might not be useful to skip the other checks.

Good idea.

Another idea I tried was to pass the EX_VAR(opline->result.var) through the cache slot but this doesn't work well because zend_get_property_offset assumes member is CONST when a cache slot is passed.

I also thought about storing EX_VAR(opline->result.var) in EG(assign_result). I don't like this, but this may simplify some other places.

bwoebi · 2023-02-21T12:18:09Z

As an extension maintainer, I'll certainly grumble a little about breaking the signature of write_property, but I rather have a clean API where I assign a garbage var, which I see in the signature to be freed, than having a very subtle distinction to do based on the current opcode (and having to know about this, rather than seeing "oh there's a garbage parameter, I guess I just copy the old value there").
As ultimately, this is nothing which a small #if can't handle.

As such I would oppose your proposal @dstogov.

dstogov · 2023-02-21T13:45:07Z

@bwoebi see my comment...

I definitely don't like 6694d28

and the following...

We are trying to find a different solution to keep the signature of write_property() unchanged. We already have two different solutions but both are not idea. @bwoebi you are also welcome to think about the better approach.

iluuu1994 · 2023-02-22T13:09:40Z

At this point I don't have a different idea.

No. write_property() may emit error or throw exception so EX(opline) must be up to date.

Of course!

I'm not sure what do you mean. Tty to construct a test case.

My comment doesn't make sense 🙂

Unfortunately yes. For internal function we don't set EX(opline).

Ah, so it's uninitialized memory. Ok.

I would also prefer to adjust write_property in the long run. I suppose we could eliminate the BC break.

Create a new, optional object handler as an alternative to write_property that sets the **garbage parameter
- When not available, the caller must fall back to write_property
Add zend_std_write_property_ex() to avoid breaking calls to zend_std_write_property()

dstogov · 2023-02-22T15:04:12Z

I would also prefer to adjust write_property in the long run. I suppose we could eliminate the BC break.

* Create a new, optional object handler as an alternative to `write_property` that sets the `**garbage` parameter
  
  * When not available, the caller must fall back to `write_property`

* Add `zend_std_write_property_ex()` to avoid breaking calls to `zend_std_write_property()`

This *garbage is specific to a concrete implementation problem. It's hard to understand and I wouldn't like to add it to "user" API. I think, it might be better to add zval *additional_result argument instead and use a simplified version of my last proposed patch.

iluuu1994 · 2023-02-22T15:11:07Z

Ok, I will create a patch for this so we can compare the solutions.

dstogov · 2023-03-06T19:59:50Z

I think this may be accepted.
Of course, I don't like all these over-complications, but this looks like an almost minimal possible fix for this family of problems.
@nikic it would be great if you could take a final look and give you suggestion(s).
I think it's better to take care about JIT related fixes before committing this. I plan to do this myself.

iluuu1994 · 2023-03-30T18:23:30Z

I don't think a review is coming for this one 🙂 How shall we move forward?

dstogov · 2023-04-03T13:11:59Z

I see 4 test failures with tracing JIT make test TESTS="-d opcache.jit=1254 --repeat 3 Zend/tests/gh10168"

GH-10168: Assign prop with prop ref [Zend/tests/gh10168/assign_untyped_prop_with_prop_ref.phpt]
GH-10168: Assign with prop ref [Zend/tests/gh10168/assign_with_prop_ref.phpt]
GH-10168: Assign prop [Zend/tests/gh10168/assign_prop.phpt]
GH-10168: Assign prop with prop ref [Zend/tests/gh10168/assign_prop_with_prop_ref.phpt]

Let me try to fix them and take the final decision.
Do you see any other problems/failures?

iluuu1994 · 2023-04-03T13:39:28Z

Do you see any other problems/failures?

Nope, that's all the ones I'm seeing too.

Fixes phpGH-10168

iluuu1994 · 2023-04-04T17:00:25Z

This was merged into master. @nielsdos Thank you also for your help!

iluuu1994 requested a review from dstogov February 16, 2023 23:17

iluuu1994 mentioned this pull request Feb 16, 2023

Fix another use-after-free with static properties/destructors #10546

Closed

mvorisek reviewed Feb 16, 2023

View reviewed changes

iluuu1994 force-pushed the gh-10168-alternative branch from 6694d28 to 6936692 Compare February 17, 2023 13:44

github-actions bot added the Extension: com_dotnet label Feb 17, 2023

nielsdos reviewed Feb 17, 2023

View reviewed changes

github-actions bot removed Extension: pdo (core) Extension: mysqli Extension: zip Extension: dom Extension: reflection Extension: date Extension: xmlreader Extension: ffi Extension: json Extension: standard Extension: snmp Extension: spl Extension: simplexml Extension: com_dotnet labels Mar 3, 2023

iluuu1994 force-pushed the gh-10168-alternative branch 2 times, most recently from d77d08b to 1510191 Compare March 3, 2023 11:38

dstogov and others added 5 commits April 3, 2023 17:53

Delay freeing of overwritten values in assignments

1c0327c

Fixes phpGH-10168

Add various tests for phpGH-10168

47d3b6f

Delay destructor for zend_assign_to_typed_ref

4811078

Delay destructor for zend_std_write_property

5116ca5

Add GC_DTOR/GC_DTOR_NO_REF macros

94392ec

iluuu1994 force-pushed the gh-10168-alternative branch from 1510191 to 94392ec Compare April 3, 2023 15:57

dstogov mentioned this pull request Apr 3, 2023

Gh 10168 alternative #11006

Closed

iluuu1994 closed this Apr 4, 2023

nielsdos mentioned this pull request Apr 4, 2023

Heap use after free at zend_is_smaller_helper_SPEC #10582

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delay freeing of overwritten values in assignments #10606

Delay freeing of overwritten values in assignments #10606

iluuu1994 commented Feb 16, 2023 •

edited

Loading

mvorisek Feb 16, 2023

nielsdos commented Feb 17, 2023

nielsdos left a comment

dstogov commented Feb 20, 2023

iluuu1994 commented Feb 21, 2023

dstogov commented Feb 21, 2023

bwoebi commented Feb 21, 2023

dstogov commented Feb 21, 2023

iluuu1994 commented Feb 22, 2023

dstogov commented Feb 22, 2023

iluuu1994 commented Feb 22, 2023

dstogov commented Mar 6, 2023

iluuu1994 commented Mar 30, 2023

dstogov commented Apr 3, 2023

iluuu1994 commented Apr 3, 2023

iluuu1994 commented Apr 4, 2023

Delay freeing of overwritten values in assignments #10606

Delay freeing of overwritten values in assignments #10606

Conversation

iluuu1994 commented Feb 16, 2023 • edited Loading

mvorisek Feb 16, 2023

Choose a reason for hiding this comment

nielsdos commented Feb 17, 2023

nielsdos left a comment

Choose a reason for hiding this comment

dstogov commented Feb 20, 2023

iluuu1994 commented Feb 21, 2023

dstogov commented Feb 21, 2023

bwoebi commented Feb 21, 2023

dstogov commented Feb 21, 2023

iluuu1994 commented Feb 22, 2023

dstogov commented Feb 22, 2023

iluuu1994 commented Feb 22, 2023

dstogov commented Mar 6, 2023

iluuu1994 commented Mar 30, 2023

dstogov commented Apr 3, 2023

iluuu1994 commented Apr 3, 2023

iluuu1994 commented Apr 4, 2023

iluuu1994 commented Feb 16, 2023 •

edited

Loading