php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #48607 fwrite() doesn't check reply from ftp server before exiting
Submitted: 2009-06-19 16:37 UTC Modified: 2010-12-13 17:53 UTC
Votes:7
Avg. Score:4.3 ± 0.7
Reproduced:6 of 6 (100.0%)
Same Version:1 (16.7%)
Same OS:1 (16.7%)
From: karachi at mail dot ru Assigned: iliaa
Status: Closed Package: FTP related
PHP Version: 5.2.10 OS: FreeBSD
Private report: No CVE-ID:
 [2009-06-19 16:37 UTC] karachi at mail dot ru
Description:
------------
I tried to upload a file to ftp server using ftp wrapper functions. Sequence of steps are: fopen, fwrite, fclose. Sometimes file uploaded successfully and sometimes several last bytes of file didn't save. Server reports in log file that transmission was aborted, but fwrite() returns "true".
I found that fwrite() doesn't check ftp reply code before exiting. That's why ftp server can receive QUIT command before it processes data stored and close data connection as described in RFC 959: "The server may abort data transfer if the control connections
are closed without command." If I insert sleep(1) between fwrite() and fclose() everything works as expected.
Ftp server used: ProFTPD 1.3.2

Reproduce code:
---------------
<?php
$host = 'localhost';
$user = 'ftp';
$passwd = 'anonymous';

$f = fopen("/home/user/test/test_file", 'r') or
    die ("Unable to open file");

$context = stream_context_create(
    array(
        "ftp" => array(
            "overwrite" => true
        )
    )
);

$f1 = fopen("ftp://$host/test", 'wb', false, $context) or
    die("Unable to open the file on ftp");

while ($str = fread($f, 1000))
{
    fwrite($f1, $str) or die("Unable to write");
}

fclose($f1);
fclose($f);
?>

Expected result:
----------------
File is successfully stored on ftp server

Actual result:
--------------
Sometimes end of the file isn't stored. It doesn't depend on the size of the file.

Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2009-06-26 20:24 UTC] sjoerd-php at linuxonly dot nl
Thank you for your bug report.

To further investigate this bug, it would be useful if you can supply some more information. Specifically, the data sent from and to the FTP server would be useful. You can obtain this with a sniffer such as tcpdump or Wireshark or maybe by configuring your FTP server. You would be looking for something like this:

230 Anonymous access granted, restrictions apply.
SIZE test_file
550 test_file: No such file or directory
EPSV 
229 Entering Extended Passive Mode (|||3110|)
STOR /test_file
 [2009-08-05 21:57 UTC] jani@php.net
Yes, we really need more info about this.
 [2009-08-13 01:00 UTC] php-bugs at lists dot php dot net
No feedback was provided for this bug for over a week, so it is
being suspended automatically. If you are able to provide the
information that was originally requested, please do so and change
the status of the bug back to "Open".
 [2009-08-18 16:23 UTC] karachi at mail dot ru
Where can I save captured ftp data?
 [2009-08-18 16:24 UTC] karachi at mail dot ru
I mean I captured data but I can't attach them to the bug
 [2009-08-25 08:28 UTC] sjoerd@php.net
You can use a pastebin or e-mail it to sjoerd-php at linuxonly dot nl.
 [2009-08-25 16:33 UTC] sjoerd@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 17:45 UTC] sjoerd@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-12-17 20:17 UTC] 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-12-17 21:59 UTC] 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.
 [2010-08-31 19:03 UTC] savageman86 at yahoo dot fr
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 ! :-)
 [2010-10-14 19:41 UTC] eric dot caron at gmail dot com
I can reproduce this on my CentOS 5 box running PHP 5.3.3. It occurs when sending a large file across a slow network. Wireshark reports getting the QUIT before the FTP server sends TRANSFER COMPLETE. Adding the sleep before the close fixes the issue.

Reproduce code:
------------------
<?php
$fileName = SOME_LARGE_BINARY_FILE;
$conn_id = ftp_connect(FTP_SERVER);
$login_result = ftp_login($conn_id, USERNAME, PASSWORD);
ftp_pasv($conn_id, true);
ftp_put($conn_id, 'file_dump', $fileName, FTP_BINARY);
//Putting a sleep here fixes the problem
ftp_close($conn_id);
 [2010-10-14 20:11 UTC] pajoye@php.net
-Package: Streams related +Package: FTP related
 [2010-12-13 17:53 UTC] iliaa@php.net
Automatic comment from SVN on behalf of iliaa
Revision: http://svn.php.net/viewvc/?view=revision&amp;revision=306342
Log: Fixed bug #48607 (fwrite() doesn't check reply from ftp server before exiting)
 [2010-12-13 17:53 UTC] iliaa@php.net
-Status: Verified +Status: Closed -Assigned To: +Assigned To: iliaa
 [2010-12-13 17:53 UTC] iliaa@php.net
This bug has been fixed in SVN.

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.


 [2011-03-28 16:59 UTC] eric dot caron at gmail dot com
What are the steps involved in having this bug moved from SVN into the current 5.3.x branch? It is a bug fix, no new features are added nor does any functionality change, yet two 5.3.x releases have come out since this bug was marked CLOSED and they don't include this fix.
 
PHP Copyright © 2001-2014 The PHP Group
All rights reserved.
Last updated: Thu Apr 17 21:01:56 2014 UTC