Skip to content

Commit

Permalink
Avoid double buffering in Zend streams
Browse files Browse the repository at this point in the history
Disable buffering in PHP streams, to avoid storing and copying the
file contents twice.

This will call stream_set_option() on custom stream wrapper as
well, so the method needs to be implemented to avoid a warning.
  • Loading branch information
nikic committed Jul 17, 2019
1 parent 918f09f commit a986e70
Show file tree
Hide file tree
Showing 6 changed files with 15 additions and 5 deletions.
3 changes: 3 additions & 0 deletions Zend/tests/bug38779.phpt
Expand Up @@ -22,6 +22,9 @@ class Loader {
function stream_stat() {
return array('size' => strlen($this->data));
}
function stream_set_option($option, $arg1, $arg2) {
return false;
}
}
stream_wrapper_register('Loader', 'Loader');
require 'Loader://qqq.php';
Expand Down
8 changes: 3 additions & 5 deletions Zend/zend_stream.c
Expand Up @@ -126,7 +126,7 @@ static size_t zend_stream_read(zend_file_handle *file_handle, char *buf, size_t
ZEND_API int zend_stream_fixup(zend_file_handle *file_handle, char **buf, size_t *len) /* {{{ */
{
size_t size;
zend_stream_type old_type;
zend_bool is_fp = 0;

if (file_handle->buf) {
*buf = file_handle->buf;
Expand All @@ -145,6 +145,7 @@ ZEND_API int zend_stream_fixup(zend_file_handle *file_handle, char **buf, size_t
return FAILURE;
}

is_fp = 1;
file_handle->type = ZEND_HANDLE_STREAM;
file_handle->handle.stream.handle = file_handle->handle.fp;
file_handle->handle.stream.isatty = isatty(fileno((FILE *)file_handle->handle.stream.handle));
Expand All @@ -158,10 +159,7 @@ ZEND_API int zend_stream_fixup(zend_file_handle *file_handle, char **buf, size_t
return FAILURE;
}

old_type = file_handle->type;
file_handle->type = ZEND_HANDLE_STREAM; /* we might still be _FP but we need fsize() work */

if (old_type == ZEND_HANDLE_FP && !file_handle->handle.stream.isatty && size) {
if (is_fp && !file_handle->handle.stream.isatty && size) {

This comment has been minimized.

Copy link
@dstogov

dstogov Jul 17, 2019

Member

Why "is_fp" here? It leads to use slow path for most PHP scripts.

This comment has been minimized.

Copy link
@nikic

nikic Jul 17, 2019

Author Member

This preserves existing behavior (I made a mistake in a previous patch that always skipped this codepath). I think the reason is that file size reported by PHP stream layer cannot be trusted.

This comment has been minimized.

Copy link
@dstogov

dstogov Jul 17, 2019

Member

The previous behavior disabled mmap() for streams, but I don't see any problem to read stream contents at once.

This comment has been minimized.

Copy link
@nikic

nikic Jul 17, 2019

Author Member

Okay, I agree. Done in c2c5c9a. Now there will be a single read() syscall and no realloc/copying.

file_handle->buf = *buf = safe_emalloc(1, size, ZEND_MMAP_AHEAD);
file_handle->len = zend_stream_read(file_handle, *buf, size);
} else {
Expand Down
3 changes: 3 additions & 0 deletions ext/standard/tests/file/fopencookie.phpt
Expand Up @@ -76,6 +76,9 @@ class userstream {
function stream_stat() {
return array('size' => strlen($this->data));
}
function stream_set_option($option, $arg1, $arg2) {
return false;
}
}

stream_wrapper_register("cookietest", "userstream");
Expand Down
3 changes: 3 additions & 0 deletions ext/standard/tests/file/include_streams.phpt
Expand Up @@ -96,6 +96,9 @@ class mystream
}
}

function stream_set_option($option, $arg1, $arg2) {
return false;
}
}

if (!stream_wrapper_register("test", "mystream")) {
Expand Down
2 changes: 2 additions & 0 deletions main/main.c
Expand Up @@ -1594,6 +1594,8 @@ PHPAPI int php_stream_open_for_zend_ex(const char *filename, zend_file_handle *h
handle->handle.stream.closer = php_zend_stream_closer;
/* suppress warning if this stream is not explicitly closed */
php_stream_auto_cleanup(stream);
/* Disable buffering to avoid double buffering between PHP and Zend streams. */
php_stream_set_option(stream, PHP_STREAM_OPTION_READ_BUFFER, PHP_STREAM_BUFFER_NONE, NULL);

return SUCCESS;
}
Expand Down
Expand Up @@ -59,6 +59,7 @@ final class StreamWrapper
public function stream_close() : bool { return \fclose($this->stream); }
public function stream_eof() : bool { return \feof($this->stream); }
public function stream_stat() { return \fstat($this->stream); }
public function stream_set_option($option, $arg1, $arg2) { return false; }

private $stream = false;
}
Expand Down

0 comments on commit a986e70

Please sign in to comment.