Skip to content

soap #73237 - Duplicate zval so it's not overwritten on next iteration #2153

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

Closed
wants to merge 3 commits into from

Conversation

ksmiley
Copy link
Contributor

@ksmiley ksmiley commented Oct 3, 2016

If the response includes both fields with simple types (which get concatenated into an XML string) and a complex type (which is parsed into an object), then the object will parsed into the same zval as the simple types and will overwrite the string.

https://bugs.php.net/bug.php?id=73237

If the response includes both fields with simple types (which get
concatenated into an XML string) and a complex type (which is parsed
into an object), then the object will parsed into the same zval as the
simple types and will overwrite the string.
@ksmiley ksmiley force-pushed the soap/bug73237/parseAnyElement branch from b986be1 to a44d945 Compare October 3, 2016 22:16
@cmb69
Copy link
Member

cmb69 commented Oct 4, 2016

Note that there's a memory leak in ZTS mode.

@ksmiley
Copy link
Contributor Author

ksmiley commented Oct 6, 2016

Thanks @cmb69. I added a commit that seems to fix the leak.

@jameshollenbeck
Copy link

How close are we to merging this -- ran into a pretty nasty situation at work and this seems like it would resolve it.

Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Fix looks right to me. I've left two notes to avoid unnecessary copies/leaks.

@@ -1259,6 +1259,7 @@ static void model_to_zval_any(zval *ret, xmlNodePtr node)
ZVAL_NULL(&val2);
master_to_zval(&val2, get_conversion(XSD_ANYXML), node->next);
if (Z_TYPE(val2) != IS_STRING || *Z_STRVAL(val) != '<') {
Z_TRY_DELREF(val2);
Copy link
Member

Choose a reason for hiding this comment

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

This should be zval_ptr_dtor(val2);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I change it to zval_ptr_dtor(&val2), I consistently get a segfault. It looks like calling the destructor invalidates an object that was stored in SOAP_GLOBAL(ref_map), and that object is needed (by soap_check_xml_ref) on the next iteration of the outer loop.

Copy link
Member

Choose a reason for hiding this comment

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

Hrm, in that case I'm wondering why the zval_ptr_dtor(&val2) four lines below cannot cause the same issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This inner loop is peeking ahead in the XML document (node->next), and this if decides whether to consume the next node or leave it for the outer loop. I think the reason the zval_ptr_dtor below doesn't cause a problem is that the inner loop has already consumed the next node, and it won't be needed again.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation, makes sense.

@@ -1277,7 +1278,9 @@ static void model_to_zval_any(zval *ret, xmlNodePtr node)
any = &arr;
name = NULL;
} else {
any = &val;
ZVAL_DUP(&keepVal, &val);
zval_dtor(&val);
Copy link
Member

@nikic nikic Oct 11, 2016

Choose a reason for hiding this comment

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

These two lines should be ZVAL_COPY_VALUE(&keepVal, &val);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Updated.

@nikic
Copy link
Member

nikic commented Oct 12, 2016

Merged as 2628713, thanks!

@nikic nikic closed this Oct 12, 2016
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.

4 participants