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

51423: AtomParser: fixes PHP 8 fatal error in wp-includes/atomlib.php #718

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

hellofromtonya
Copy link
Contributor

@hellofromtonya hellofromtonya commented Nov 9, 2020

Trac ticket: https://core.trac.wordpress.org/ticket/51423

Split of the larger PR for this commit.

tl;dr

This change guards Core while ensuring 3rd party developers continue to be alerted to the problem from fopen Warning.

  • Guards AtomParser::parser() from PHP 8 fatal error when/if AtomParser::FILE is ever changed to an invalid or unavailable resource.
  • Retains original behavior.
  • Retains original Warning from fopen to alert 3rd party developer of an invalid/unavailable resource.
  • Does not retain the additional warnings from fread and fclose.
    • Is it problematic to hide these additional warnings?
    • Hmm, I argue "No".
    • Why? fopen will throw a Warning. The additional warnings are due to the initial problem at fopen.
  • Ensures XML Parser is freed and error handler restored.

Risk:

  • Core: none
  • 3rd party: possible

More Details

When AtomParser::FILE is not changed

This change is not needed if the property AtomParser::FILE is not changed.

Why?

By default, AtomParser::FILE is set to php://input for the read-only stream.

  • fopen will return the resource pointer for the stream.
  • If there's nothing in the stream:
    • fread returns an empty string
    • fclose closes the resource pointer.
    • There are no errors or warnings in PHP 5, 7, and 8.
  • See it in action here https://3v4l.org/p4YKQ.

What if the property AtomParser::FILE is changed?

If valid and available, fopen returns the resource. Same behavior as above.

AtomParser::FILE is invalid or unavailable

If invalid/unavailable, fopen returns false and throws a Warning in PHP 5, 7, and 8. See it in action here https://3v4l.org/pYKdD.

Warning: fopen(): open_basedir restriction in effect. File(/usr/local/something.txt) is not within the allowed path(s): (/tmp:/in:/etc) in /in/pYKdD on line 12
Warning: fopen(/usr/local/something.txt): failed to open stream: Operation not permitted in /in/pYKdD on line 12

This behavior is the same in all PHP versions. We don't need to do anything for PHP 8.

What about fread and fclose?

See it in action here https://3v4l.org/HhApV.

fopen returned false. fread and fclose throws:

  • PHP 5 and 7

Warning: fread() expects parameter 1 to be resource, bool given in /in/HhApV on line 13
Warning: fclose() expects parameter 1 to be resource, bool given in /in/HhApV on line 25

  • PHP 8

Fatal error: Uncaught TypeError: fread(): Argument #1 ($stream) must be of type resource, bool given in /in/HhApV:13

TODO

  • Validate current behavior with PHP 5-8
  • Analyze and write up explanation and results
  • Add fixes
    - [ ] Add code coverage for the AtomParser::parse(). There's no code coverage for the external library.

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@hellofromtonya hellofromtonya changed the title Trac Ticket 51423: wp-includes/atomlib.php: Check the fopen failure condition. 51423: PHPStan fixes for wp-includes/atomlib.php Nov 10, 2020
If a plugin/theme changes AtomParser::FILE to an invalid or unavailable
resource, fopen will return `false`.

- PHP 8: `fread` throws a fatal error as its 1st param requires
  a resource type.
- <PHP 8: `fread` and `fclose` throw a warning, but code continues
  to run.

This commit:

- wraps the entire `while` block and `fclose` to prevent the PHP 8
  fatal error.
- allows the remaining code to run to ensure the parser is freed
  and error handler restored.
@hellofromtonya hellofromtonya force-pushed the fix/51423-wp-includes-atomlib.php branch from 39e2823 to ba0ef76 Compare November 22, 2020 17:26
@hellofromtonya
Copy link
Contributor Author

AtomParser is part of an external library that was added to core's source in 2.3.0. The original library has not been updated since 2007.

This PR modifies this original library's source. Do we wish to make this change in core or contribute upstream?

@hellofromtonya hellofromtonya changed the title 51423: PHPStan fixes for wp-includes/atomlib.php 51423: PHP 8 fix for fatal error in AtomParser in wp-includes/atomlib.php Nov 22, 2020
@hellofromtonya hellofromtonya changed the title 51423: PHP 8 fix for fatal error in AtomParser in wp-includes/atomlib.php 51423: AtomParser: fixes PHP 8 fatal error in wp-includes/atomlib.php Nov 22, 2020
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.

1 participant