php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Doc Bug #79965 Why does the manual say fread/fwrite changed in PHP 7.4?
Submitted: 2020-08-12 17:03 UTC Modified: 2020-08-12 22:28 UTC
From: thiemo dot kreuz at wikimedia dot de Assigned:
Status: Verified Package: *Directory/Filesystem functions
PHP Version: 7.4.9 OS:
Private report: No CVE-ID: None
Welcome back! If you're the original bug submitter, here's where you can edit the bug or add additional notes.
If this is not your bug, you can add a comment by following this link.
If this is your bug, but you forgot your password, you can retrieve your password here.
Password:
Status:
Package:
Bug Type:
Summary:
From: thiemo dot kreuz at wikimedia dot de
New email:
PHP Version: OS:

 

 [2020-08-12 17:03 UTC] thiemo dot kreuz at wikimedia dot de
Description:
------------
---
From manual page: https://php.net/migration74.incompatible
---

Quote: "fread() and fwrite() will now return FALSE if the operation failed. Previously an empty string or 0 was returned."

This is not correct, or at least misleading. Both functions are documented to return false in case of a failure ever since. What changed in PHP 7.4? I tried to find the related code change, but all I found are bugfixes. Does this section refer to a bugfix? Maybe there was a very specific failure situation that was not reported via false?

TL;DR: Please re-write or remove this misleading section from the migration manual.

https://www.php.net/manual/en/migration74.incompatible.php#migration74.incompatible.core.fread-fwrite


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2020-08-12 21:57 UTC] cmb@php.net
-Status: Open +Status: Verified
 [2020-08-12 21:57 UTC] cmb@php.net
The relevant commit is d59aac5[1]; the PHPTs show that some, but
not all return values in case of failure have been changed from
int(0) to bool(false).

[1] <http://git.php.net/?p=php-src.git;a=commit;h=d59aac58b3e7da7ad01a194fe9840d89725ea229>
 [2020-08-12 22:28 UTC] thiemo dot kreuz at wikimedia dot de
Oh wow, that's a massive change. Thanks!

I guess the manual should say something like "fread/fwrite will now return false in all error situations, as it was always documented. Before, an empty string or 0 was returned in many error situations, e.g. when a gzip stream couldn't be decoded, or when writing to a file that was opened in read mode."

Note it's not only fread/fwrite, but also fgetcsv/fputcsv.
 [2020-10-22 16:01 UTC] webmaster_php20201022 at cubiclesoft dot com
"Does this section refer to a bugfix?"

The change actually originally stemmed from a potential denial of service opportunity in PHP that I ran into back in 2016 while building TCP socket servers that was impossible to work around without incurring another denial of service opportunity.  For the gory details (and some of the drama):

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

There was a formal CVE issued for this bug (CVE-2016-9321) but, as you can see, it was never applied to the ticket nor mentioned anywhere in release notes.  I believe the PHP devs ended up very quietly deploying the fix because it took so long to fix the problem and tried to keep the messaging as simple as possible in the release notes.  Specifically, fixing it required a BC break, which only happens in major version releases (e.g. 7.3 to 7.4) and those only happen once a year.

This particular bug exposes a significant weakness in the software release cycles of languages (not just PHP):  Discovered security vulnerabilities in a major language but requires a BC break in order to fix the problem might take more than one year to fix.  The issue was opened too late for inclusion in 7.2 and was accidentally forgotten for 7.3.  As a dev, what would *you* do in that situation?  Fixing the bug as quietly as possible for 7.4 and using a simple two sentence string in the migration guide to minimize questions from userland devs seems like a decent strategy to me.  This is a perfect example of the fact that software is written by humans, humans are fallable and can miss stuff, and that even perfectly good processes like a standard software release cycle could even be exploited by malicious actors.

Some people who read the previous paragraph might be tempted to jump ship from PHP, but all formal language development has the same problem if the devs of a language adhere to good software development methodologies.  In this particular instance, the documentation reflected good wishes prior to 7.4 rather than how the software was actually written.  The issue might have been noticed and fixed a lot sooner if PHP userland documentation pages linked to the relevant C source code.  There's always a lot of inertia to go from documentation to actual source code.  When was the last time you cracked open and read the C source code to PHP itself?  For 99% of PHP userland devs, the answer is "never."

Personally, I'm just glad it got fixed because it's a low-level stream handling problem that had significant negative repercussions throughout PHP userland beyond socket streams.

Regardless of how we got here, PHP 7.4 is a good upgrade because stream error handling is improved.
 [2020-10-22 16:22 UTC] thiemo dot kreuz at wikimedia dot de
Thanks a lot for this deep dive, that's really good to know. Unfortunately it doesn't solve the confusion caused by the line in the migration guide. From what I learned so far I don't think a slightly more specific explanation would pose a security risk:
* Mention all affected functions.
* Be more specific about what changed. Currently it sounds like fread/fwrite *never* returned false. Instead, the return value in a failure situation was ""/0 only in *some* situations. Why not say that?

Where can I upload a pull request for this manual page?
 
PHP Copyright © 2001-2020 The PHP Group
All rights reserved.
Last updated: Fri Oct 30 04:01:20 2020 UTC