php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #60110 fclose(), file_put_contents(), copy() do not return false properly
Submitted: 2011-10-21 19:38 UTC Modified: 2023-08-29 15:41 UTC
Votes:22
Avg. Score:5.0 ± 0.2
Reproduced:16 of 18 (88.9%)
Same Version:5 (31.2%)
Same OS:11 (68.8%)
From: tom at punkave dot com Assigned: bukka (profile)
Status: Assigned Package: Streams related
PHP Version: 5.3.8 OS: all
Private report: No CVE-ID: None
 [2011-10-21 19:38 UTC] tom at punkave dot com
Description:
------------
The fclose() function does not check the result of flushing the stream:

	if (!stream->is_persistent) {
		zend_list_delete(stream->rsrc_id);
	} else {
		php_stream_pclose(stream);
	}

	RETURN_TRUE;

php_stream_pclose has a return value but it is ignored.

php_stream_pclose, in turn, calls _php_stream_free which flushes the stream but 
does not check the return value either:

	/* make sure everything is saved */
	_php_stream_flush(stream, 1 TSRMLS_CC);

Contrary to the comment we did not make sure of anything (:

So the fix has to be made at least as deep as _php_stream_flush to be effective. 
I have verified that _php_stream_flush does pay attention to the result of the 
underlying flush operation. Note that fflush() reports write errors successfully 
while fclose() doesn't).

In many environments, including stdio (plain old files), the final write to disk 
might not sometimes occur until the stream is flushed by _php_stream_flush. If 
this fails, for instance because the disk is full, PHP pays no heed to the 
error.

This is especially obvious if you use a stream wrapper that does its actual 
writing when the stream is closed (for instance storing an object with a single 
HTTP request), but again, it could happen with normal files too.

Because of this deeper issue with _php_stream_free, all other PHP convenience 
functions for files such as copy() and file_put_contents() also fail to 
correctly report false when the final flush of data to disk fails when closing a 
file.

This has serious consequences for any application that is counting on data 
integrity, which would be pretty much every application that uses files I guess.



Patches

Pull Requests

Pull requests:

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2011-10-21 21:23 UTC] cataphract@php.net
See bug #53328. This is a good candidate for trunk, for stable versions I fear (perhaps unfoundedly) that, because the return value of php_stream_close/free is almost always ignored, some wrappers might have gotten away with incorrect return values on the close handler.
 [2011-10-21 21:35 UTC] tom at punkave dot com
This can definitely happen with the regular stdio stuff. stdio employs buffering 
as a matter of course. It is a serious bug, not a change in behavior.

As for stream wrappers, the documentation specifies what stream_flush is 
supposed to return, and fflush() would already be failing for people with bad 
stream wrappers who did not heed that documentation.

stream_close is not supposed to return anything but is not affected by this bug 
because stream_flush has already been called (and cheerfully ignored) before 
stream_close is called (I checked). 

So there is no need to change the behavior of stream_close (which would be a bc 
break). We just need to pay attention to what stream_flush is already telling 
us.
 [2011-10-24 17:05 UTC] tom at punkave dot com
How about a close_checks_flush php.ini flag which defaults to false in 5.3 and to 
true in 5.4?
 [2011-12-29 04:45 UTC] tstarling@php.net
It would be nice if the return value of php_stream_close() was checked, but note that fclose() does not call php_stream_close(), it calls zend_list_delete(), which can't return anything sensible because rsrc_dtor_func_t returns void.

It would be ugly and difficult to change rsrc_dtor_func_t at this stage, and in any case, during shutdown or zval destruction we don't really care about flush failures. I think the way to go would be to call php_stream_close() from fclose() before calling the resource destructor. There probably won't be any dangling pointers, it looks pretty well guarded already.

As for cataphract's concern: we could audit all the wrapper close functions, there's not that many of them is there?
 [2011-12-30 17:25 UTC] cataphract@php.net
fclose() calls php_stream_close since PHP 5.4, see http://svn.php.net/viewvc?view=revision&revision=309491
 [2012-08-20 05:38 UTC] stas@php.net
This looks like a bug, I think it'd be fine to have the fix in 5.4. Any pull reqs 
with fixes?
 [2013-05-07 20:16 UTC] scottix at gmail dot com
Still a bug
PHP 5.4.14 (cli) (built: May  5 2013 12:09:08)

I am using gluster to write to a file and then in the middle of the write I 
perform a unplug power shutoff. Gluster goes into lock mode which is good, what 
it is supposed to do. Although php doesn't seem to do the right thing, I did a 
strace of the fclose and you can clearly see a -1 is sent, but php is ignoring 
it.

//Gluster shut off and gluster mount log shows volume down

//fwrite here although can succeed since it buffers
write(3, "Some Text 2", 11)             = 11
write(1, "int(11)\n", 8int(11)
)                = 8

// Performed fflush(), not sure about this one
write(1, "bool(true)\n", 11bool(true)
)            = 11

// Performed fclose() here
close(3)                                = -1 ENOTCONN (Transport endpoint is not 
connected)
write(1, "bool(true)\n", 11bool(true)

Please make this higher priority.
 [2015-05-19 14:10 UTC] tim at bishnet dot net
Same issue here using file_put_contents:

82607 open("hello.txt", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 3
82607 fstat(3, {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
82607 lseek(3, 0, SEEK_CUR)             = 0
82607 write(3, "Hello, world", 12)      = 12
82607 close(3)                          = -1 EDQUOT (Disk quota exceeded)

Write succeeds but the close fails. file_put_contents doesn't check the result of the close:

https://github.com/php/php-src/blob/master/ext/standard/file.c#L695

Looks unlikely anybody is going to fix this though, given the age of this bug :-(
 [2021-08-18 11:10 UTC] cmb@php.net
-Package: Filesystem function related +Package: Streams related
 [2023-08-29 15:40 UTC] bukka@php.net
The following pull request has been associated:

Patch Name: Fix bug #60110 (fclose(), file_put_contents(), copy() do not return false properly)
On GitHub:  https://github.com/php/php-src/pull/12067
Patch:      https://github.com/php/php-src/pull/12067.patch
 [2023-08-29 15:41 UTC] bukka@php.net
-Assigned To: +Assigned To: bukka
 [2023-08-29 15:41 UTC] bukka@php.net
I have been looking into this and implemented initial changes in the linked PR. It needs more checking as noted in the PR though.
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Fri Oct 11 09:01:26 2024 UTC