Edit report at http://bugs.php.net/bug.php?id=48607&edit=1

 ID:                 48607
 Comment by:         savageman86 at yahoo dot fr
 Reported by:        karachi at mail dot ru
 Summary:            fwrite() doesn't check reply from ftp server before
                     exiting
 Status:             Verified
 Type:               Bug
 Package:            Streams related
 Operating System:   FreeBSD
 PHP Version:        5.2.10
 Block user comment: N

 New Comment:

I also ran into this bug. One possible solution is to use a custom ftp
stream wrapper which encapsulates the ftp_* functions, because
ftp_fput() works well and waits the file has finished uploading before
returning.



At the end, the only current solution is to use the ftp lib and not the
default ftp stream wrapper, which is buggy. It's sad, because stream
wrappers are really a killer feature ! :-)


Previous Comments:
------------------------------------------------------------------------
[2009-12-17 21:59:06] b dot vontobel at meteonews dot ch

Sorry, just realized that I went a little bit too far when cleaning up 

my mess for the diff/patch. :)



--- php-5.3.1/ext/standard/ftp_fopen_wrapper.c  2008-12-31 

11:15:49.000000000 +0000

+++ php-5.3.1-ftp_fopen_wrapper_patch/ext/standard/ftp_fopen_wrapper.c  

2009-12-17 21:32:53.000000000 +0000

@@ -97,14 +97,34 @@

  */

 static int php_stream_ftp_stream_close(php_stream_wrapper *wrapper, 

php_stream *stream TSRMLS_DC)

 {

+       int ret = 0, result = 0;

+       char tmp_line[512];

        php_stream *controlstream = (php_stream *)stream->wrapperdata;

-       

+

+       /* For write modes close data stream first to signal EOF to 

server */

+       if (strpbrk(stream->mode, "wa+")) {

+               if (stream && controlstream) {

+                       php_stream_close(stream);

+                       stream = NULL;

+

+                       result = GET_FTP_RESULT(controlstream);

+                       if (result != 226 && result != 250) {

+                               php_error_docref(NULL TSRMLS_CC, 

E_WARNING, "FTP server reports %s", tmp_line);

+                               ret = EOF;

+                       }

+               } else {

+                       php_error_docref(NULL TSRMLS_CC, E_WARNING, 

"Broken streams to FTP server");

+                       ret = EOF;

+               }

+       }

+

        if (controlstream) {

                php_stream_write_string(controlstream, "QUIT\r\n");

                php_stream_close(controlstream);

-               stream->wrapperdata = NULL;

+               if (stream)

+                       stream->wrapperdata = NULL;

        }

-       return 0;

+       return ret;

 }



Also make sure that I or somebody else afterwards really does not call 

something on/in the streams after closing and probably freeing them 

(didn't really check out the internals of _php_stream_free() et al. -- 

and the control stream sort of being embedded within the data stream 

here, but me having to close them the other way round due to the FTP 

protocol, didn't really help in understanding what might go wrong 

somewhere deep in your API). But as I said, for me the patch works.

------------------------------------------------------------------------
[2009-12-17 20:17:41] b dot vontobel at meteonews dot ch

Just stumbled across this (still in 5.3.1) a few days ago, trying to 

transmit data to three different FTP servers. One of the servers 

_never_ got a file, one got files, but in 9 out of 10 runs the last 

part of the files was cut off, only the last one got the files intact 

in about 8 of 10 runs (with the others also corrupted).



I didn't find this bug report at first and so I opened up the PHP 

source for the first time in my life and was rather shocked: There's 

really no way that write operations using the ftp stream wrapper ever 

could've worked. If it works, it's out of pure luck. Was this never 

tested?



The problem is, that FTP (see RFC959) uses the tear down of the 

_data_stream_ as its EOF marker. What this code does on the other 

hand, is just send a QUIT on the control stream and then tear down 

that one. So from the perspective of the FTP server it looks like an 

abort (transmission still in progress, but control channel lost). Now 

it just depends on the implementation of the server and sometimes some 

random timing issues (which TCP close is handled first) what the 

outcome is: Some FTP servers just annihilate everything that was 

transmitted so far (realizing it was a client abort during 

transmission or a network glitch - and the file probably corrupted 

anyway), others keep what they got so far. Only sometimes (out of 

luck) they maybe get the close on the data stream first and are still 

able to send the okay on the control stream (which is not handled by 

the current code, but what sjoerd added in his first idea of a patch).



Now, I'm not really familiar with the PHP stream wrapper API at all, 

but below is my imagination of how this code could be made to work (I 

actually run it on a 30+ vhosts cluster): If we were only reading from 

the stream, it's probably okay to not care about a clean shutdown 

(especially because in this code part nothing really tells us reliably 

what exact state the FTP session is in). But if we have written to the 

FTP server, we MUST close the data stream first, then wait for a 

confirmation from the server - and only then both of us know, the data 

was sent completely:



--- php-5.3.1/ext/standard/ftp_fopen_wrapper.c  2008-12-31 

11:15:49.000000000 +0000

+++ php-5.3.1-ftp_fopen_wrapper_patch/ext/standard/ftp_fopen_wrapper.c  

2009-12-16 18:41:07.000000000 +0000

@@ -97,14 +97,33 @@

  */

 static int php_stream_ftp_stream_close(php_stream_wrapper *wrapper, 

php_stream *stream TSRMLS_DC)

 {

+       int ret = 0, result = 0;

+       char tmp_line[512];

        php_stream *controlstream = (php_stream *)stream->wrapperdata;

-       

+

+       /* For write modes close data stream first to signal EOF to 

server */

+       if (strpbrk(stream->mode, "wa+")) {

+               if (stream && controlstream) {

+                       php_stream_close(stream);

+                       stream = NULL;

+

+                       result = GET_FTP_RESULT(controlstream);

+                       if (result != 226 && result != 250) {

+                               php_error_docref(NULL TSRMLS_CC, 

E_WARNING, "FTP server reports %s", tmp_line);

+                               ret = EOF;

+                       }

+               } else {

+                       php_error_docref(NULL TSRMLS_CC, E_WARNING, 

"Broken streams to FTP server");

+                       ret = EOF;

+               }

+       }

+

        if (controlstream) {

                php_stream_write_string(controlstream, "QUIT\r\n");

                php_stream_close(controlstream);

-               stream->wrapperdata = NULL;

+               controlstream = NULL;

        }

-       return 0;

+       return ret;

 }

 /* }}} */

 

@@ -563,8 +582,9 @@

                goto errexit;

        }

 

-       /* remember control stream */   

+       /* remember control stream and mode */  

        datastream->wrapperdata = (zval *)stream;

+       strcpy(datastream->mode, mode); 

 

        php_url_free(resource);

        return datastream;



Now, I'd be very happy if somebody more familiar with the PHP API's 

could have a glance at this patch, maybe do some fine-tuning and then 

apply it and hopefully close this bug.



I especially scratched my head regarding access to the mode of the 

stream and regarding error handling: Mode seems to be handled locally 

only in php_stream_url_wrap_ftp() and then to be discarded. I didn't 

see a cleaner and quicker way than to overwrite the mode in the stream 

from php_stream_xport_create() which seems to always be "r+", hoping 

to not break something elsewhere. Where would the clean way be to keep 

such things or get them from in this API? Is there some usable 

internals documentation? Or should I add some struct's to 

abstract/wrapperdata? But this quickly looked like changing _a_lot_ 

throughout this whole code...



Then the error handling: From what I see in the code, 

php_stream_wrapper_log_error() would probably be the correct way? But 

I couldn't make it work (and would need access to the "context", 

that's handed over to the other functions, but not to close). I could 

also not get PHP's fclose() to return a failure. From some glances 

through the rest of the code, on all the higher levels, 

success/failure is just discarded? Despite the manual that says 

"Returns TRUE on success or FALSE on failure.". Is it really just not 

possible to get a FALSE value out of fclose within this API if I 

couldn't close my streams? So am I just plain stupid if I check the 

return values of PHP functions in my PHP code, because they're really 

just hardcoded to TRUE? I'm really sorry about being so ranty and 

would love to help, but it just looks like quite a few things are 

broken around here -- I should probably add some more bug reports.



Currently I just used php_error_docref() to at least get a warning 

through to the top-level and hope that somebody familiar with this API 

can add a real solution. While my patch fixes the primary issue and 

catches errors, the PHP code still thinks it successfully wrote its 

data...

------------------------------------------------------------------------
[2009-08-25 17:45:17] sjo...@php.net

The solution may be something like this, although this may break things
when the current transaction is not about to send a 226 Transfer
complete.



Index: ext/standard/ftp_fopen_wrapper.c

===================================================================

--- ext/standard/ftp_fopen_wrapper.c    (revision 287652)

+++ ext/standard/ftp_fopen_wrapper.c    (working copy)

@@ -97,7 +97,16 @@

  */

 static int php_stream_ftp_stream_close(php_stream_wrapper *wrapper,
php_stream *stream TSRMLS_DC)

 {

+       int result;

+       char tmp_line[512];

+

        php_stream *controlstream = (php_stream *)stream->wrapperdata;

+

+       result = GET_FTP_RESULT(controlstream);

+       if (result != 226 && result != 250) {

+               // Maybe throw a warning?

+               return 1;

+       }

        

        if (controlstream) {

                php_stream_write_string(controlstream, "QUIT\r\n");



------------------------------------------------------------------------
[2009-08-25 16:33:02] sjo...@php.net

I could reproduce the QUIT before the transfer being complete.



FTP     Request: STOR /a.php

FTP     Response: 150 Opening BINARY mode data connection for /a.php

FTP     Request: QUIT

FTP     Response: 226 Transfer complete

------------------------------------------------------------------------
[2009-08-25 08:28:23] sjo...@php.net

You can use a pastebin or e-mail it to sjoerd-php at linuxonly dot nl.

------------------------------------------------------------------------


The remainder of the comments for this report are too long. To view
the rest of the comments, please view the bug report online at

    http://bugs.php.net/bug.php?id=48607


-- 
Edit this bug report at http://bugs.php.net/bug.php?id=48607&edit=1

Reply via email to