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

Fix for crash with references on PHP7 #113

Closed
wants to merge 2 commits into from

Conversation

mheijkoop
Copy link
Contributor

@mheijkoop mheijkoop commented Dec 2, 2016

This solves #94.

I've turned the example in the issue into a test for the issue and added a workaround which passes the test.

It's likely there's a better solution for this, but we were seeing this problem in production so making the symptoms go away needed to be done urgently.

I would expect this change to introduce a memory leak, but after running for a couple of days on our high-traffic php-fpm backend I'm not seeing much of that. Obviously ymmv.

I've managed to trace the issue to the zval of $bad[1] getting corrupted after $bad[10] is parsed. $bad[1] turns into an IS_INDIRECT pointing to an IS_REFERENCE, pointing to an array. The second element array ($bad[10]) does not turn into an indirect and is merely a reference to an array.

This corruption doesn't occur when I exempt IS_INDIRECT zvals in msgpack_var_replace.

I'm hoping this either suffices as a fix or points someone with more experience with these internals to the 'proper' solution.

(Note the build for this pull fails due to master currently failing 041.phpt, which is fixed by #109 )

Marlies Heijkoop added 2 commits December 2, 2016 17:26
@Sean-Der
Copy link
Member

Sean-Der commented Dec 7, 2016

Hey @mheijkoop thanks for the patch! This has bitten multiple people, but I never got around to fixing it this is awesome.

You mentioned leaking, I ran with make tests TESTS=-m and the only thing the test suite hit was some invalid reads (which I will be fixing now) did you hit anything to suspect a memory leak in particular?

Merged!

@Sean-Der Sean-Der closed this Dec 7, 2016
@mheijkoop
Copy link
Contributor Author

You mentioned leaking, I ran with make tests TESTS=-m and the only thing the test suite hit was some invalid reads (which I will be fixing now) did you hit anything to suspect a memory leak in particular?

Nah, I thought so based on the original commits where the bug was introduced, but my reasoning was flawed, no worries :)

Sorry for the late response, I was on holidays for quite a while :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants