php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #39215 Inappropriate close of stdin/stdout/stderr
Submitted: 2006-10-20 16:07 UTC Modified: 2006-11-03 13:34 UTC
From: tstarling at wikimedia dot org Assigned: iliaa
Status: Closed Package: Streams related
PHP Version: 5CVS-2006-10-20 (CVS) OS: Linux & Windows
Private report: No CVE-ID:
 [2006-10-20 16:07 UTC] tstarling at wikimedia dot org
Description:
------------
The stream created by fopen('php://stdin','r') has inappropriate ownership semantics. It closes the underlying FD when it is destroyed, despite the fact that it didn't open it. If you create two distinct streams which refer to the same FD, as demonstrated below, you can cause a double-close, which causes a segfault on Windows XP. 

This may well be a regression caused by the fix of bug #38199

Reproduce code:
---------------
<?php

function foo() {
        static $stdin;
        $stdin = fopen( 'php://stdin', 'r' );
        return fgets( $stdin );
}
print foo();

?>


Expected result:
----------------
FD 0 should not be closed.

Actual result:
--------------
You can see that FD 0 is closed using strace. In fact it is closed twice, once by the static variable destructor and once by the destructor of the STDIN constant.

[1546][tstarling@zwinger:~]$ strace -e trace=close php -n stdin-test.php
close(3)                                = 0
close(3)                                = 0

...

hello
hello
close(0)                                = 0
close(0)                                = -1 EBADF (Bad file descriptor)
Process 28429 detached


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2006-10-20 16:31 UTC] tony2001@php.net
How do you expect to work then?
You should be able to close it AND you shouldn't be able to close it in the same time. These two statements are mutually exclusive.

And I can't reproduce the segfault, so please provide a backtrace.
 [2006-10-20 16:38 UTC] tony2001@php.net
.. and the "double close" is actually much easier to reproduce with just:
<?php
$s = fopen("php://stdin", "r");
?>
On shutdown both $s and STDIN constant are destroyed, but they both point to the same resource. It's reproducible only with CLI, though.

 [2006-10-21 01:20 UTC] tstarling at wikimedia dot org
In reply to Tony: they're not mutually exclusive statements, you just need to prevent implicit or duplicate closes while allowing explicit closes. This could be done by setting a flag in the stream structure at the end of php_stream_url_wrap_php(). The flag could be detected in _php_stream_free() and a close avoided. Duplicate closes can be prevented either by keeping an array of filedescriptor states in memory, or by somehow detecting the state of the FD before attempting a close. 

I decided to submit a bug report rather than a patch because I wasn't sure about the best way to implement it. 

According to the MSVC Run-Time Library Reference, regarding _close():

"This function validates its parameters. If fd is a bad file descriptor, the invalid parameter handler is invoked, as described in Parameter Validation. If execution is allowed to continue, the functions returns -1 and errno is set to EBADF."

And on parameter validation generally:

"The behavior of the C Runtime when an invalid parameter is found is to call the currently assigned invalid parameter handler. The default invalid parameter handler raises an Access Violation exception, which normally makes continued execution impossible. In Debug mode, an assertion is also raised."

I have compiled PHP in debug mode, so here is the assertion as documented:

Debug assertion failed!
Program: ...
File: close.c
Line: 48

Expression: (_osfile(fh) & FOPEN)


Then the access violation:

 	msvcr80d.dll!00c49dc0() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for msvcr80d.dll]	
 	msvcr80d.dll!00c3f564() 	
 	msvcr80d.dll!00c7ff39() 	
>	php5ts_debug.dll!php_stdiop_close(_php_stream * stream=0x014f19c0, int close_handle=1, void * * * tsrm_ls=0x003c4f90)  Line 380 + 0xf bytes	C
 	php5ts_debug.dll!_php_stream_free(_php_stream * stream=0x014f19c0, int close_options=11, void * * * tsrm_ls=0x003c4f90)  Line 342 + 0x1e bytes	C
 	php5ts_debug.dll!stream_resource_regular_dtor(_zend_rsrc_list_entry * rsrc=0x014f8a50, void * * * tsrm_ls=0x003c4f90)  Line 1365 + 0xf bytes	C
 	php5ts_debug.dll!list_entry_destructor(void * ptr=0x014f8a50)  Line 184 + 0x12 bytes	C
 	php5ts_debug.dll!zend_hash_del_key_or_index(_hashtable * ht=0x003c76d8, char * arKey=0x00000000, unsigned int nKeyLength=0, unsigned long h=1, int flag=1)  Line 492 + 0x11 bytes	C
 	php5ts_debug.dll!_zend_list_delete(int id=1, void * * * tsrm_ls=0x003c4f90)  Line 58 + 0x24 bytes	C
 	php5ts_debug.dll!_zval_dtor_func(_zval_struct * zvalue=0x01529758, char * __zend_filename=0x10714738, unsigned int __zend_lineno=35)  Line 60 + 0xf bytes	C
 	php5ts_debug.dll!_zval_dtor(_zval_struct * zvalue=0x01529758, char * __zend_filename=0x107177b8, unsigned int __zend_lineno=33)  Line 35 + 0x17 bytes	C
 	php5ts_debug.dll!free_zend_constant(_zend_constant * c=0x01529758)  Line 33 + 0x17 bytes	C
 	php5ts_debug.dll!zend_hash_apply_deleter(_hashtable * ht=0x003c6f08, bucket * p=0x01529700)  Line 606 + 0x11 bytes	C
 	php5ts_debug.dll!zend_hash_reverse_apply(_hashtable * ht=0x003c6f08, int (void *, void * * *)* apply_func=0x1029adc0, void * * * tsrm_ls=0x003c4f90)  Line 736 + 0xd bytes	C
 	php5ts_debug.dll!clean_non_persistent_constants(void * * * tsrm_ls=0x003c4f90)  Line 162 + 0x23 bytes	C
 	php5ts_debug.dll!shutdown_executor(void * * * tsrm_ls=0x003c4f90)  Line 303 + 0x9 bytes	C
 	php5ts_debug.dll!zend_deactivate(void * * * tsrm_ls=0x003c4f90)  Line 840 + 0x9 bytes	C
 	php5ts_debug.dll!php_request_shutdown(void * dummy=0x00000000)  Line 1300 + 0x9 bytes	C
 [2006-10-21 01:56 UTC] tstarling at wikimedia dot org
Although I cringe to say it, it looks like the easiest way to avoid the access violation on windows may be to temporarily disable assertions and parameter checking. Some references:

_close: http://msdn2.microsoft.com/en-us/library/5fzwd5ss.aspx
_set_invalid_parameter_handler: http://msdn2.microsoft.com/en-us/library/a9yf33zb.aspx
 [2006-10-21 21:00 UTC] wez@php.net
We added the constant STDIN to avoid this issue.

 [2006-10-21 21:10 UTC] wez@php.net
Hmm, someone changed the close behavior of those constants.  Re-opening while I check into it.
 [2006-10-21 22:27 UTC] wez@php.net
This patch is scheduled for PHP 5.2.1.
Suggested workaround is to use the STDIN constant instead of explicitly opening php://stdin.


Index: ext/standard/php_fopen_wrapper.c
===================================================================
RCS file: /repository/php-src/ext/standard/php_fopen_wrapper.c,v
retrieving revision 1.45.2.4.2.2
diff -u -p -r1.45.2.4.2.2 php_fopen_wrapper.c
--- ext/standard/php_fopen_wrapper.c    5 Jul 2006 17:38:14 -0000       1.45.2.4.2.2
+++ ext/standard/php_fopen_wrapper.c    21 Oct 2006 22:13:22 -0000
@@ -191,11 +191,41 @@ php_stream * php_stream_url_wrap_php(php
        }  
 
        if (!strcasecmp(path, "stdin")) {
-               fd = !strcmp(sapi_module.name, "cli") ? STDIN_FILENO : dup(STDIN_FILENO);
+               if (!strcmp(sapi_module.name, "cli")) {
+                       static int cli_in = 0;
+                       fd = STDIN_FILENO;
+                       if (cli_in) {
+                               fd = dup(fd);
+                       } else {
+                               cli_in = 1;
+                       }
+               } else {
+                       fd = dup(STDIN_FILENO);
+               }
        } else if (!strcasecmp(path, "stdout")) {
-               fd = !strcmp(sapi_module.name, "cli") ? STDOUT_FILENO : dup(STDOUT_FILENO);
+               if (!strcmp(sapi_module.name, "cli")) {
+                       static int cli_out = 0;
+                       fd = STDOUT_FILENO;
+                       if (cli_out++) {
+                               fd = dup(fd);
+                       } else {
+                               cli_out = 1;
+                       }
+               } else {
+                       fd = dup(STDOUT_FILENO);
+               }
        } else if (!strcasecmp(path, "stderr")) {
-               fd = !strcmp(sapi_module.name, "cli") ? STDERR_FILENO : dup(STDERR_FILENO);
+               if (!strcmp(sapi_module.name, "cli")) {
+                       static int cli_err = 0;
+                       fd = STDERR_FILENO;
+                       if (cli_err++) {
+                               fd = dup(fd);
+                       } else {
+                               cli_err = 1;
+                       }
+               } else {
+                       fd = dup(STDERR_FILENO);
+               }
        } else if (!strncasecmp(path, "filter/", 7)) {
                /* Save time/memory when chain isn't specified */
                if (strchr(mode, 'r') || strchr(mode, '+')) {
 [2006-11-03 13:34 UTC] iliaa@php.net
This bug has been fixed in CVS.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.
 
Thank you for the report, and for helping us make PHP better.


 
PHP Copyright © 2001-2014 The PHP Group
All rights reserved.
Last updated: Sun Apr 20 20:02:01 2014 UTC