Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:46953
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain meteonews.ch designates 195.141.194.3 as permitted sender)
Message-ID: <679B196B-E276-44BE-87EB-6949A673A205@meteonews.ch>
To: internals@lists.php.net
Content-Type: text/plain; charset=US-ASCII; format=flowed; delsp=yes
Content-Transfer-Encoding: 7bit
Mime-Version: 1.0 (Apple Message framework v936)
Date: Mon, 8 Feb 2010 12:03:42 +0100
Subject: [PATCH] Fix for bug #48607 (ftp_fopen_wrapper)
From: b.vontobel@meteonews.ch (Beat Vontobel)

Hi,

I'm new to the PHP internals mailing list (and also to PHP internals  
in general), so let's first say hello to everybody and thanks for your  
work!

I needed to dive into PHP's internals myself because I hit a problem  
(which I later found to be bug #48607) with the ftp_fopen_wrapper:  
fwrite basically doesn't work at all with FTP, because PHP's  
implementation is in violation of the FTP specs. No EOF is signalled  
at the end of a transmission -- PHP's behaviour after writing to an  
FTP server has to be treated as an aborted connection by a clean  
server. If an fwrite to an FTP server ever succeeded, it was only due  
to lucky timing coincidence or buggy servers.

The patch below fixes this issue for me. I run it successfully since  
more than a month on two high-load production webservers. I'd be happy  
if somebody more familiar with PHP's internals could review it, apply  
it to SVN and then hopefully close bug #48607.

Just one important note for the reviewer: I wasn't quite sure on how  
to correctly "hand up" errors to the higher levels of the API. That's  
where I just used php_error_docref(), which is probably not the  
intended way, but worked for me to at least generate a warning. I also  
now set the return value of the function on failure, but realized that  
it's again hardcoded to "success" on higher levels of the code. This  
would probably make for another bug report and more patches. Sorry for  
my hard words on this in the bug database, I just fell into "rant- 
mode" when I realized that an exit code for fclose() was hardcoded to  
success in many places throughout PHP's code -- while I keep telling  
my developers to always check return values... :)

Cheers,
Beat


--- 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;
  }
  /* }}} */

@@ -563,8 +583,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;