-
Notifications
You must be signed in to change notification settings - Fork 387
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 PHP8 crash due to insufficient isset test #670
Conversation
library/SimplePie/Item.php
Outdated
{ | ||
$type = $this->sanitize($enclosure[0]['attribs']['']['type'], SIMPLEPIE_CONSTRUCT_TEXT); | ||
} | ||
if (isset($enclosure[0]['attribs']['']['length'])) | ||
if (!empty($enclosure[0]['attribs']['']['length'])) | ||
{ | ||
$length = ceil($enclosure[0]['attribs']['']['length']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here was the actual crash with PHP8+
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will still crash when invalid enclosure="foo"
attribute is provided. Perhaps rather than checking emptiness, we should check if the value is a number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
intval()
will cast any wrong value to 0, which is also the behaviour for ceil()
in PHP7-
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should work (with slightly different semantics for numbers with decimal point) but now the ceil
is not longer necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right, on mobile now, but will simplify tomorrow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise floatval()
could be an option to keep ceil()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
intval()
is probably better, since length and fileSize are both expressed in bytes anyway
The Travis integration does not work, but I passed the tests manually, and they are unchanged before/after my PR:
|
thanks for fixing the |
@mblaney I kept |
`ceil()` crashes in PHP8+ in case of invalid input such as empty string. `intval()` fixes the problem with almost identical beahviour than `ceil()` in PHP7- (except for flotting point values) #fix FreshRSS/FreshRSS#3401 (crash with PHP 8+) Example with feed http://podcast.hr2.de/derTag/podcast.xml ```xml <enclosure url="https://mp3podcasthr-a.akamaihd.net:443/mp3/podcast/derTag/derTag_20210129_87093232.mp3" length="" type="audio/mpeg"/> ``` `isset("")` passes and then `ceil("")` crashes due to wrong type in PHP8+: ``` Uncaught TypeError: ceil(): Argument #1 ($num) must be of type int|float, string given in ./SimplePie/SimplePie/Item.php:2871 ```
6abfae5
to
6323ec7
Compare
@mblaney I have reverted to address only the specific problem of |
#fix #3401 (crash with PHP 8+) `ceil()` crashes in PHP8+ in case of invalid input such as empty string. `intval()` fixes the problem with almost identical behaviour than `ceil()` in PHP7- (except for floating point values) #fix #3401 (crash with PHP 8+) Example with feed http://podcast.hr2.de/derTag/podcast.xml ```xml <enclosure url="https://mp3podcasthr-a.akamaihd.net:443/mp3/podcast/derTag/derTag_20210129_87093232.mp3" length="" type="audio/mpeg"/> ``` `isset("")` passes and then `ceil("")` crashes due to wrong type in PHP8+: ``` Uncaught TypeError: ceil(): Argument #1 ($num) must be of type int|float, string given in ./SimplePie/SimplePie/Item.php:2871 ``` Upstream patch simplepie/simplepie#670
#fix FreshRSS/FreshRSS#3401 (crash with PHP 8+)
ceil()
crashes in PHP8+ in case of invalid input such as empty string.intval()
fixes the problem with almost identical behaviour thanceil()
in PHP7- (except for floating point values)#fix FreshRSS/FreshRSS#3401 (crash with PHP 8+)
Example with feed http://podcast.hr2.de/derTag/podcast.xml
isset("")
passes and thenceil("")
crashes due to wrong type in PHP8+: