[PATCH] Fix for bug #48607 (ftp_fopen_wrapper)

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;