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 #81351: xml_parse may fail, but has no error code #7363

Closed
wants to merge 2 commits into from

Conversation

cmb69
Copy link
Contributor

@cmb69 cmb69 commented Aug 12, 2021

The fix for bug #73135[1] cured the symptoms, but not the root cause,
namely xmlParse() must not be called recursively. Since that bugfix
also messed up the error handling, we basically revert it (but also
simplify the return), and then prevent calling the parser recursively.

[1] f2a8a8c

The fix for bug #73151[1] cured the symptoms, but not the root cause,
namely xmlParse() must not be called recursively.  Since that bugfix
also messed up the error handling, we basically revert it (but also
simplify the return), and then prevent calling the parser recursively.

[1] <php@f2a8a8c>
@cmb69 cmb69 added the Bug label Aug 12, 2021
ext/xml/xml.c Outdated Show resolved Hide resolved
Co-authored-by: Nikita Popov <nikita.ppv@gmail.com>
@cmb69 cmb69 closed this in 80a377e Aug 13, 2021
@cmb69 cmb69 deleted the cmb/81351 branch August 13, 2021 15:42
@cmb69
Copy link
Contributor Author

cmb69 commented Aug 13, 2021

As of PHP 8.0.0, xml_parse() is strictly supposed to return int. Can we throw there? Or just return 0 for failure?

@nikic
Copy link
Member

nikic commented Aug 13, 2021

@cmb69 Programmer error, so throw.

cmb69 added a commit that referenced this pull request Aug 13, 2021
As of PHP 8.0.0, these functions are supposed to return int, so we
cannot return `false`.  Since calling the parser recursively is a
programmer error, we throw an `Error` in this case.

Cf. <#7363>.
@cmb69
Copy link
Contributor Author

cmb69 commented Aug 13, 2021

Done.

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

Successfully merging this pull request may close these issues.

None yet

2 participants