Fix bug #63937: Upload speed 10 times slower with PHP (#13041)

There are two slow parts in the upload logic:
- Reading from the input stream character by character
- Checking each character one by one to normalize line endings

First of all, the line normalization isn't necessary for binary
transfers, so we can use a simple read while loop to read bytes into the
transfer buffer.

Second, for the ASCII transfer where we do have to normalize line
endings, we can be smarter than reading one character at a time. There's
a php_stream_get_line() function that we can repurpose if the flags for
the stream are set up properly.

This patch implements these fixes.

Results: I tested this on an 850 MiB file, transferring this to an FTP
server running locally.

Results before patch:
Binary/ASCII transfer (same code path): 8.21s

Results after patch:
Binary transfer: 0.65s
ASCII transfer: 0.74s

Further improvement is probably possible by having a larger send buffer.

ext/ftp/ftp.c

@@ -974,14 +974,84 @@ bail:
 }
 /* }}} */
 
+static zend_result ftp_send_stream_to_data_socket(ftpbuf_t *ftp, databuf_t *data, php_stream *instream, ftptype_t type, bool send_once_and_return)
+{
+	if (type == FTPTYPE_ASCII) {
+		/* Change (and later restore) flags to make sure php_stream_get_line() searches '\n'. */
+		const uint32_t flags_mask = PHP_STREAM_FLAG_EOL_UNIX | PHP_STREAM_FLAG_DETECT_EOL | PHP_STREAM_FLAG_EOL_MAC;
+		uint32_t old_flags = instream->flags & flags_mask;
+		instream->flags = (instream->flags & ~flags_mask) | PHP_STREAM_FLAG_EOL_UNIX;
+
+		char *ptr = data->buf;
+		const char *end = data->buf + FTP_BUFSIZE;
+		while (!php_stream_eof(instream)) {
+			size_t line_length;
+			if (!php_stream_get_line(instream, ptr, end - ptr, &line_length)) {
+				break;
+			}
+
+			ZEND_ASSERT(line_length != 0);
+
+			ptr += line_length - 1;
+			/* Replace \n with \r\n */
+			if (*ptr == '\n') {
+				*ptr = '\r';
+				/* The streams layer always puts a \0 byte at the end of a line,
+				 * so there is always place to add an extra byte. */
+				*++ptr = '\n';
+			}
+
+			ptr++;
+
+			/* If less than 2 bytes remain, either the buffer is completely full or there is a single byte left to put a '\0'
+			 * which isn't really useful, in this case send and reset the buffer. */
+			if (end - ptr < 2) {
+				size_t send_size = FTP_BUFSIZE - (end - ptr);
+				if (UNEXPECTED(my_send(ftp, data->fd, data->buf, send_size) != send_size)) {
+					instream->flags = (instream->flags & ~flags_mask) | old_flags;
+					return FAILURE;
+				}
+				ptr = data->buf;
+				if (send_once_and_return) {
+					break;
+				}
+			}
+		}
+
+		instream->flags = (instream->flags & ~flags_mask) | old_flags;
+
+		if (end - ptr < FTP_BUFSIZE) {
+			size_t send_size = FTP_BUFSIZE - (end - ptr);
+			if (UNEXPECTED(my_send(ftp, data->fd, data->buf, send_size) != send_size)) {
+				return FAILURE;
+			}
+		}
+	} else {
+		while (!php_stream_eof(instream)) {
+			ssize_t size = php_stream_read(instream, data->buf, FTP_BUFSIZE);
+			if (size == 0) {
+				break;
+			}
+			if (UNEXPECTED(size < 0)) {
+				return FAILURE;
+			}
+			if (UNEXPECTED(my_send(ftp, data->fd, data->buf, size) != size)) {
+				return FAILURE;
+			}
+			if (send_once_and_return) {
+				break;
+			}
+		}
+	}
+
+	return SUCCESS;
+}
+
 /* {{{ ftp_put */
 int
 ftp_put(ftpbuf_t *ftp, const char *path, const size_t path_len, php_stream *instream, ftptype_t type, zend_long startpos)
 {
 	databuf_t		*data = NULL;
-	zend_long			size;
-	char			*ptr;
-	int			ch;
 	char			arg[MAX_LENGTH_OF_LONG];
 
 	if (ftp == NULL) {
@@ -1018,30 +1088,10 @@ ftp_put(ftpbuf_t *ftp, const char *path, const size_t path_len, php_stream *inst
 		goto bail;
 	}
 
-	size = 0;
-	ptr = data->buf;
-	while (!php_stream_eof(instream) && (ch = php_stream_getc(instream))!=EOF) {
-		/* flush if necessary */
-		if (FTP_BUFSIZE - size < 2) {
-			if (my_send(ftp, data->fd, data->buf, size) != size) {
-				goto bail;
-			}
-			ptr = data->buf;
-			size = 0;
-		}
-
-		if (ch == '\n' && type == FTPTYPE_ASCII) {
-			*ptr++ = '\r';
-			size++;
-		}
-
-		*ptr++ = ch;
-		size++;
-	}
-
-	if (size && my_send(ftp, data->fd, data->buf, size) != size) {
+	if (ftp_send_stream_to_data_socket(ftp, data, instream, type, false) != SUCCESS) {
 		goto bail;
 	}
+
 	data_close(ftp);
 
 	if (!ftp_getresp(ftp) || (ftp->resp != 226 && ftp->resp != 250 && ftp->resp != 200)) {
@@ -1060,9 +1110,6 @@ int
 ftp_append(ftpbuf_t *ftp, const char *path, const size_t path_len, php_stream *instream, ftptype_t type)
 {
 	databuf_t		*data = NULL;
-	zend_long			size;
-	char			*ptr;
-	int			ch;
 
 	if (ftp == NULL) {
 		return 0;
@@ -1085,30 +1132,10 @@ ftp_append(ftpbuf_t *ftp, const char *path, const size_t path_len, php_stream *i
 		goto bail;
 	}
 
-	size = 0;
-	ptr = data->buf;
-	while (!php_stream_eof(instream) && (ch = php_stream_getc(instream))!=EOF) {
-		/* flush if necessary */
-		if (FTP_BUFSIZE - size < 2) {
-			if (my_send(ftp, data->fd, data->buf, size) != size) {
-				goto bail;
-			}
-			ptr = data->buf;
-			size = 0;
-		}
-
-		if (ch == '\n' && type == FTPTYPE_ASCII) {
-			*ptr++ = '\r';
-			size++;
-		}
-
-		*ptr++ = ch;
-		size++;
-	}
-
-	if (size && my_send(ftp, data->fd, data->buf, size) != size) {
+	if (ftp_send_stream_to_data_socket(ftp, data, instream, type, false) != SUCCESS) {
 		goto bail;
 	}
+
 	data_close(ftp);
 
 	if (!ftp_getresp(ftp) || (ftp->resp != 226 && ftp->resp != 250 && ftp->resp != 200)) {
@@ -2256,39 +2283,19 @@ bail:
 int
 ftp_nb_continue_write(ftpbuf_t *ftp)
 {
-	long			size;
-	char			*ptr;
-	int 			ch;
-
 	/* check if we can write more data */
 	if (!data_writeable(ftp, ftp->data->fd)) {
 		return PHP_FTP_MOREDATA;
 	}
 
-	size = 0;
-	ptr = ftp->data->buf;
-	while (!php_stream_eof(ftp->stream) && (ch = php_stream_getc(ftp->stream)) != EOF) {
-
-		if (ch == '\n' && ftp->type == FTPTYPE_ASCII) {
-			*ptr++ = '\r';
-			size++;
-		}
-
-		*ptr++ = ch;
-		size++;
-
-		/* flush if necessary */
-		if (FTP_BUFSIZE - size < 2) {
-			if (my_send(ftp, ftp->data->fd, ftp->data->buf, size) != size) {
-				goto bail;
-			}
-			return PHP_FTP_MOREDATA;
-		}
-	}
-
-	if (size && my_send(ftp, ftp->data->fd, ftp->data->buf, size) != size) {
+	if (ftp_send_stream_to_data_socket(ftp, ftp->data, ftp->stream, ftp->type, true) != SUCCESS) {
 		goto bail;
 	}
+
+	if (!php_stream_eof(ftp->stream)) {
+		return PHP_FTP_MOREDATA;
+	}
+
 	data_close(ftp);
 
 	if (!ftp_getresp(ftp) || (ftp->resp != 226 && ftp->resp != 250)) {

ext/ftp/tests/ftp_fput_ascii_over_4_kib.phpt

@@ -0,0 +1,33 @@
+--TEST--
+Testing ftp_fput basic functionality
+--EXTENSIONS--
+ftp
+pcntl
+--FILE--
+<?php
+require 'server.inc';
+
+$ftp = ftp_connect('127.0.0.1', $port);
+ftp_login($ftp, 'user', 'pass');
+$ftp or die("Couldn't connect to the server");
+
+$filename = "large_file.txt";
+$filepath = __DIR__ . "/" . $filename;
+
+// Test on boundary of 2 buffers
+for ($i = 4094; $i < 4098; $i++) {
+    $contents = str_repeat("a", $i) . "\n" . str_repeat("b", 10);
+    file_put_contents($filepath, $contents);
+    var_dump(ftp_put($ftp, "large_file.txt", $filepath, FTP_ASCII));
+}
+
+?>
+--CLEAN--
+<?php
+@unlink(__DIR__ . "/large_file.txt");
+?>
+--EXPECT--
+bool(true)
+bool(true)
+bool(true)
+bool(true)