Skip to content

Handle reallocated root buffer during GC destroy phase (v2) #4935

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Handle reallocated root buffer during GC destroy phase (v2) #4935

wants to merge 2 commits into from

Conversation

TysonAndre
Copy link
Contributor

@TysonAndre TysonAndre commented Nov 20, 2019 •
edited
Loading

We no longer protect GC during the destroy phase, so we need to
deal with buffer reallocation.

Note that the implementation of spl_SplObjectStorage_free_storage
will call the destructor of SplObjectStorage, and free the instance properties,
which I think is what caused the root buffer to be reallocated.

This fixes bug #78811 for me.

I needed both of the lines - the crash still happened before current = GC_IDX2PTR(idx) was added (gdb said that current->ptr was invalid memory)

The build failure is spurious

========DIFF========
130+ [0494] Expecting string/00:50:01, got string/00:50:00 resp. string/00:50:00. [0]
========DONE========
FAIL mysqli_fetch_all() [C:\projects\php-src\ext\mysqli\tests\mysqli_fetch_all.phpt]

@TysonAndre
Copy link
Contributor Author

TysonAndre commented Nov 20, 2019 •
edited
Loading 

						GC_ADDREF(obj);
						obj->handlers->dtor_obj(obj);
						GC_DELREF(obj);

I suspect that the above lines calling dtor_obj may also cause a similar issue. But the issue I'm having is fixed by this PR. (they won't, because current was the problem, not obj)

  • Zend/zend_objects.c for zend_objects_destroy_object looks like it will call __destruct in zend_call_function(&fci, &fcic);, and invoking that method (user-defined or internal) might free other objects/arrays?

TysonAndre
TysonAndre commented Nov 20, 2019
View reviewed changes
Zend/zend_gc.c Outdated
@@ -1555,6 +1555,8 @@ ZEND_API int zend_gc_collect_cycles(void)
GC_ADD_FLAGS(obj, IS_OBJ_FREE_CALLED);
GC_ADDREF(obj);
obj->handlers->free_obj(obj);
current = GC_IDX2PTR(idx); // bug #78811: free_obj() can cause the root buffer to be reallocated.
obj = (zend_object*)GC_GET_PTR(current->ref);
Copy link
Contributor Author

@TysonAndre TysonAndre Nov 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, reassigning obj might be the unnecessary part.

current->ref = GC_MAKE_GARBAGE(((char*)obj) - obj->handlers->offset); - The left hand assignment was to the pointer current, which is the expression that should be recomputed.

The fix continues to work for me

@TysonAndre TysonAndre force-pushed the fix-cyclic-gc-SplObjectStorage-free branch from a9a1f40 to 1a90ab9 Compare November 20, 2019 00:32
@dstogov
Copy link
Member

dstogov commented Nov 20, 2019

@nikic please take a look.
I suppose, "current->ref = GC_MAKE_GARBAGE(((char*)obj) - obj->handlers->offset);" should be moved above the "free_obj" call.
The fix needs to be applied to 7.3 and above.

@nikic
Copy link
Member

nikic commented Nov 20, 2019

I agree that it would be better to fix this by moving it above the call, rather than recomputing the pointer. I don't think this is needed for 7.3 though, as GC is still protected during "destroy value" phase on 7.3, this only changed in 7.4.

@TysonAndre
Handle reallocated root buffer during GC destroy phase (v2)
f205cf3 
We no longer protect GC during the destroy phase, so we need to
deal with buffer reallocation.

Note that the implementation of spl_SplObjectStorage_free_storage
will call the destructor of SplObjectStorage, and free the instance properties,
which I think is what caused the root buffer to be reallocated.
(`current` is a pointer for an index within the root buffer?)

This fixes bug #78811 for me.
@TysonAndre TysonAndre force-pushed the fix-cyclic-gc-SplObjectStorage-free branch from 1a90ab9 to f205cf3 Compare November 20, 2019 13:57
@TysonAndre TysonAndre changed the base branch from master to PHP-7.4 November 20, 2019 13:58
@TysonAndre
Copy link
Contributor Author

I agree that it would be better to fix this by moving it above the call, rather than recomputing the pointer. I don't think this is needed for 7.3 though, as GC is still protected during "destroy value" phase on 7.3, this only changed in 7.4.

Done and changed the target branch to PHP-7.4. The fix still works when applied to master

nikic
nikic approved these changes Nov 20, 2019
View reviewed changes
Zend/zend_gc.c Outdated
@@ -1561,6 +1561,8 @@ ZEND_API int zend_gc_collect_cycles(void)
EG(objects_store).object_buckets[obj->handle] = SET_OBJ_INVALID(obj);
GC_TYPE_INFO(obj) = IS_NULL |
(GC_TYPE_INFO(obj) & ~GC_TYPE_MASK);
/* Modify current before calling free_obj(bug #78811: free_obj() can cause the root buffer (with current) to be reallocated.) */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing whitespace before (.

@php-pulls php-pulls closed this in 500ba8b Nov 23, 2019
@TysonAndre
Copy link
Contributor Author

Squashed and merged - the latest commit was just a comment spacing change

php-pulls pushed a commit that referenced this pull request Nov 23, 2019
@TysonAndre
Handle reallocated root buffer during GC destroy phase (v2)
b037fe5 
We no longer protect GC during the destroy phase, so we need to
deal with buffer reallocation.

Note that the implementation of spl_SplObjectStorage_free_storage
will call the destructor of SplObjectStorage, and free the instance properties,
which I think is what caused the root buffer to be reallocated.
(`current` is a pointer for an index within the root buffer?)

This fixes bug #78811 for me.

Closes GH-4935
@TysonAndre
Copy link
Contributor Author

I forgot that the target branch was changed to PHP-7.4. Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikic nikic nikic approved these changes

Assignees
No one assigned
Labels
Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@TysonAndre @dstogov @nikic @carusogabriel