Skip to content
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.

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

Fixed issue #1665: Segfault when overriding a function object parameter #474

Closed
wants to merge 7 commits into from

Conversation

@SpyroTEQ
Copy link
Contributor

commented May 16, 2019

XDebug might need to dump stack traces and such,
so we must hold a reference to the PHP variables
Otherwise, PHP might garbage collect these variables,
and XDebug will segfault when trying to dump them

SpyroTEQ added 2 commits May 15, 2019
Fixed issue #1665: Segfault when overriding a function object paramet…
…er + collect_params > 0

XDebug might need to dump stack traces and such,
so we must hold a reference to the PHP variables
Otherwise, PHP might garbage collect these variables,
and XDebug will segfault when trying to dump them
Fixed issue #1534: Segfault when exception thrown in a closure bound …
…to class scope

It's actually the same root cause than #1665 so fixing #1665 fixes this #1534 issue too
@derickr
Copy link
Contributor

left a comment

Thanks for this patch. It looks pretty good. I've a bunch of comments, questions, and suggestions for changes for ya though :-)

cheers,
Derick

tests/bug01534.phpt Outdated Show resolved Hide resolved
tests/bug01534.phpt Show resolved Hide resolved
tests/bug01665-0.phpt Outdated Show resolved Hide resolved
tests/bug01665-1-nonobject.phpt Outdated Show resolved Hide resolved
xdebug_stack.c Outdated Show resolved Hide resolved
xdebug_stack.c Outdated Show resolved Hide resolved
#endif

/* Save the GC lock references so we can remove them once the frame is discarded */
tmp->gc_locked_objects = realloc(tmp->gc_locked_objects, tmp->gc_locked_objects_count + 1);

This comment has been minimized.

Copy link
@derickr

derickr May 16, 2019

Contributor

It's probably not very efficient to do this for each call, especially if this happens a lot. Most other bits of code use not only a count (like you do), but also an allocated size. See for example: https://github.com/xdebug/xdebug/blob/master/xdebug_branch_info.c#L163-L165

This comment has been minimized.

Copy link
@derickr

derickr May 17, 2019

Contributor

Did you miss this comment, or hadn't you gotten to it yet?

This comment has been minimized.

Copy link
@SpyroTEQ

SpyroTEQ May 17, 2019

Author Contributor

I'll leave that to the very last (optimization!) ;)

This comment has been minimized.

Copy link
@SpyroTEQ

SpyroTEQ May 17, 2019

Author Contributor

Oooooh, now I get why I had "random" segfaults now: I've switched from x86 to x64, and since I had forgotten the sizeof(zend_object *), then the realloc was generating the segfault I got : )
So I've added the size you said, it seems now all good, I'll look for the performance question now.

This comment has been minimized.

Copy link
@SpyroTEQ

SpyroTEQ May 17, 2019

Author Contributor

for the efficiency, I see no way to improve this without making code unreadable, if you have clues, please say so. I did a quick stress test and ended up with 100000 * [200 nested function calls] PHP 7.3.3 64bits, without triggering the segfault this PR fixes (=normal execution with no exception thrown):

  • 27-28 seconds without xdebug 2.7 (xdebug.default_enabled=0)
  • 28-29 seconds with xdebug 2.7 (xdebug.default_enabled=1)
  • 29-30 seconds without patched xdebug (xdebug.default_enabled=0)
  • 29-31 seconds with patched xdebug (xdebug.default_enabled=1)

So I would say the overhead is acceptable for a debug extension?

xdebug_stack.c Outdated Show resolved Hide resolved
xdebug_stack.c Outdated Show resolved Hide resolved
xdebug_stack.c Outdated Show resolved Hide resolved
xdebug_stack.c Outdated Show resolved Hide resolved
#endif

/* Save the GC lock references so we can remove them once the frame is discarded */
tmp->gc_locked_objects = realloc(tmp->gc_locked_objects, tmp->gc_locked_objects_count + 1);

This comment has been minimized.

Copy link
@derickr

derickr May 17, 2019

Contributor

Did you miss this comment, or hadn't you gotten to it yet?

#else
GC_REFCOUNT(fse->gc_locked_objects[i])--;
#endif
}

This comment has been minimized.

Copy link
@derickr

derickr May 17, 2019

Contributor

I'm OK with leaving the function name as it is, because it makes sense to have it as counter to xdebug_add_stack_frame as you mentioned. I would probably add a new line in between line 1407 and 1408 though :-).

I would probably also set fse->gc_locked_objects_count to 0 at the end here. And shouldn't fse->gc_locked_objects be freed? It's allocated at https://github.com/xdebug/xdebug/pull/474/files#diff-70352c82b3e971e39da6aad79610c901R1306

This comment has been minimized.

Copy link
@SpyroTEQ

SpyroTEQ May 20, 2019

Author Contributor

Done, but please check I've freed it properly: my C skills are pretty old... :/

This comment has been minimized.

Copy link
@SpyroTEQ

SpyroTEQ May 28, 2019

Author Contributor

After reading https://www.owasp.org/index.php/Doubly_freeing_memory I think I should also set fse->gc_locked_objects = NULL? Can you confirm/infirm this suggestion @derickr ?

@derickr

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

@nikic — What do you think of this patch?

@nikic

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

I don't really get why this patch is specialized to objects.

@@ -1288,6 +1312,7 @@ function_stack_entry *xdebug_add_stack_frame(zend_execute_data *zdata, zend_op_a
} else {
ZVAL_COPY_VALUE(&(tmp->var[tmp->varc].data), ZEND_CALL_VAR_NUM(zdata, zdata->func->op_array.last_var + zdata->func->op_array.T + i - arguments_wanted));
}
gc_locker(tmp, &(tmp->var[tmp->varc].data));

This comment has been minimized.

Copy link
@nikic

nikic Jul 16, 2019

Contributor

It would make more sense to me if the above ZVAL_COPY_VALUES were ZVAL_COPY and had corresponding ptr_dtors. The whole object GC thing sounds like a red herring.

This comment has been minimized.

Copy link
@SpyroTEQ

SpyroTEQ Jul 22, 2019

Author Contributor

I'm not good enough in PHP code code to get that done well, I don't know what "ptr_dtors" would refer to.

As for the "why only objects?" thing, I couldn't make a failing test for arrays and such #474 (comment) So, I too think the arrays could fail too, but I haven't met these case so far.

This comment has been minimized.

Copy link
@nikic

nikic Jul 22, 2019

Contributor

My first guess is that you were testing with immutable arrays or interned strings. The values need to look "sufficiently dynamic", such as $foo = "foo"; $foo .= "bar" or $foo = [1]; $foo[] = 2.

By ptr_dtors I meant using the zval_ptr_dtor() function.

This comment has been minimized.

Copy link
@nikic

nikic Jul 22, 2019

Contributor

Having looked at your existing tests you'd also want it to be a temporary, so something like $foo = "foo"; call($foo . "bar") and $foo = [1]; call($foo + [2]) might do it.

@derickr

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

Hi!

Thanks for the Pull Request. In the end, I did not end up using your code changes (#503), but giving me a kick in the bum about it, in combination with reproducible cases made me fix this. The fix is considerably easier, which is always a good thing :-)

Thanks again!

cheers,
Derick

@derickr derickr closed this Sep 6, 2019

@SpyroTEQ

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

Sorry I didn't have time to get back at the PR fixes, so, if you've resolved the bugs with a lighter code, then it's all good ! : ) I'm glad I've contributed one way another

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.