Report errors from stream read and write operations

The php_stream_read() and php_stream_write() functions now return
an ssize_t value, with negative results indicating failure. Functions
like fread() and fwrite() will return false in that case.

As a special case, EWOULDBLOCK and EAGAIN on non-blocking streams
should not be regarded as error conditions, and be reported as
successful zero-length reads/writes instead. The handling of EINTR
remains unclear and is internally inconsistent (e.g. some code-paths
will automatically retry on EINTR, while some won't).

I'm landing this now to make sure the stream wrapper ops API changes
make it into 7.4 -- however, if the user-facing changes turn out to
be problematic we have the option of clamping negative returns to
zero in php_stream_read() and php_stream_write() to restore the
old behavior in a relatively non-intrusive manner.

1 files changed, 115 insertions, 66 deletions

diff --git a/main/streams/streams.c b/main/streams/streams.c
index 5f467c12ce..296bf9600e 100644
--- a/main/streams/streams.c
+++ b/main/streams/streams.c
@@ -525,13 +525,12 @@ fprintf(stderr, "stream_free: %s:%p[%s] preserve_handle=%d release_cast=%d remov
 
 /* {{{ generic stream operations */
 
-PHPAPI void _php_stream_fill_read_buffer(php_stream *stream, size_t size)
+PHPAPI int _php_stream_fill_read_buffer(php_stream *stream, size_t size)
 {
 	/* allocate/fill the buffer */
 
 	if (stream->readfilters.head) {
 		char *chunk_buf;
-		int err_flag = 0;
 		php_stream_bucket_brigade brig_in = { NULL, NULL }, brig_out = { NULL, NULL };
 		php_stream_bucket_brigade *brig_inp = &brig_in, *brig_outp = &brig_out, *brig_swap;
 
@@ -542,8 +541,8 @@ PHPAPI void _php_stream_fill_read_buffer(php_stream *stream, size_t size)
 		/* allocate a buffer for reading chunks */
 		chunk_buf = emalloc(stream->chunk_size);
 
-		while (!stream->eof && !err_flag && (stream->writepos - stream->readpos < (zend_off_t)size)) {
-			size_t justread = 0;
+		while (!stream->eof && (stream->writepos - stream->readpos < (zend_off_t)size)) {
+			ssize_t justread = 0;
 			int flags;
 			php_stream_bucket *bucket;
 			php_stream_filter_status_t status = PSFS_ERR_FATAL;
@@ -551,7 +550,10 @@ PHPAPI void _php_stream_fill_read_buffer(php_stream *stream, size_t size)
 
 			/* read a chunk into a bucket */
 			justread = stream->ops->read(stream, chunk_buf, stream->chunk_size);
-			if (justread && justread != (size_t)-1) {
+			if (justread < 0 && stream->writepos == stream->readpos) {
+				efree(chunk_buf);
+				return FAILURE;
+			} else if (justread > 0) {
 				bucket = php_stream_bucket_new(stream, chunk_buf, justread, 0, 0);
 
 				/* after this call, bucket is owned by the brigade */
@@ -607,31 +609,28 @@ PHPAPI void _php_stream_fill_read_buffer(php_stream *stream, size_t size)
 					/* when a filter needs feeding, there is no brig_out to deal with.
 					 * we simply continue the loop; if the caller needs more data,
 					 * we will read again, otherwise out job is done here */
-					if (justread == 0) {
-						/* there is no data */
-						err_flag = 1;
-						break;
-					}
-					continue;
+					break;
 
 				case PSFS_ERR_FATAL:
 					/* some fatal error. Theoretically, the stream is borked, so all
 					 * further reads should fail. */
-					err_flag = 1;
-					break;
+					stream->eof = 1;
+					efree(chunk_buf);
+					return FAILURE;
 			}
 
-			if (justread == 0 || justread == (size_t)-1) {
+			if (justread <= 0) {
 				break;
 			}
 		}
 
 		efree(chunk_buf);
+		return SUCCESS;
 
 	} else {
 		/* is there enough data in the buffer ? */
 		if (stream->writepos - stream->readpos < (zend_off_t)size) {
-			size_t justread = 0;
+			ssize_t justread = 0;
 
 			/* reduce buffer memory consumption if possible, to avoid a realloc */
 			if (stream->readbuf && stream->readbuflen - stream->writepos < stream->chunk_size) {
@@ -653,17 +652,18 @@ PHPAPI void _php_stream_fill_read_buffer(php_stream *stream, size_t size)
 			justread = stream->ops->read(stream, (char*)stream->readbuf + stream->writepos,
 					stream->readbuflen - stream->writepos
 					);
-
-			if (justread != (size_t)-1) {
-				stream->writepos += justread;
+			if (justread < 0) {
+				return FAILURE;
 			}
+			stream->writepos += justread;
 		}
+		return SUCCESS;
 	}
 }
 
-PHPAPI size_t _php_stream_read(php_stream *stream, char *buf, size_t size)
+PHPAPI ssize_t _php_stream_read(php_stream *stream, char *buf, size_t size)
 {
-	size_t toread = 0, didread = 0;
+	ssize_t toread = 0, didread = 0;
 
 	while (size > 0) {
 
@@ -692,15 +692,24 @@ PHPAPI size_t _php_stream_read(php_stream *stream, char *buf, size_t size)
 
 		if (!stream->readfilters.head && (stream->flags & PHP_STREAM_FLAG_NO_BUFFER || stream->chunk_size == 1)) {
 			toread = stream->ops->read(stream, buf, size);
-			if (toread == (size_t) -1) {
-				/* e.g. underlying read(2) returned -1 */
+			if (toread < 0) {
+				/* Report an error if the read failed and we did not read any data
+				 * before that. Otherwise return the data we did read. */
+				if (didread == 0) {
+					return toread;
+				}
 				break;
 			}
 		} else {
-			php_stream_fill_read_buffer(stream, size);
+			if (php_stream_fill_read_buffer(stream, size) != SUCCESS) {
+				if (didread == 0) {
+					return -1;
+				}
+				break;
+			}
 
 			toread = stream->writepos - stream->readpos;
-			if (toread > size) {
+			if ((size_t) toread > size) {
 				toread = size;
 			}
 
@@ -733,6 +742,26 @@ PHPAPI size_t _php_stream_read(php_stream *stream, char *buf, size_t size)
 	return didread;
 }
 
+/* Like php_stream_read(), but reading into a zend_string buffer. This has some similarity
+ * to the copy_to_mem() operation, but only performs a single direct read. */
+PHPAPI zend_string *php_stream_read_to_str(php_stream *stream, size_t len)
+{
+	zend_string *str = zend_string_alloc(len, 0);
+	ssize_t read = php_stream_read(stream, ZSTR_VAL(str), len);
+	if (read < 0) {
+		zend_string_efree(str);
+		return NULL;
+	}
+
+	ZSTR_LEN(str) = read;
+	ZSTR_VAL(str)[read] = 0;
+
+	if ((size_t) read < len / 2) {
+		return zend_string_truncate(str, read, 0);
+	}
+	return str;
+}
+
 PHPAPI int _php_stream_eof(php_stream *stream)
 {
 	/* if there is data in the buffer, it's not EOF */
@@ -776,7 +805,7 @@ PHPAPI int _php_stream_puts(php_stream *stream, const char *buf)
 	char newline[2] = "\n"; /* is this OK for Win? */
 	len = strlen(buf);
 
-	if (len > 0 && php_stream_write(stream, buf, len) && php_stream_write(stream, newline, 1)) {
+	if (len > 0 && php_stream_write(stream, buf, len) > 0 && php_stream_write(stream, newline, 1) > 0) {
 		return 1;
 	}
 	return 0;
@@ -1074,9 +1103,9 @@ PHPAPI zend_string *php_stream_get_record(php_stream *stream, size_t maxlen, con
 }
 
 /* Writes a buffer directly to a stream, using multiple of the chunk size */
-static size_t _php_stream_write_buffer(php_stream *stream, const char *buf, size_t count)
+static ssize_t _php_stream_write_buffer(php_stream *stream, const char *buf, size_t count)
 {
-	size_t didwrite = 0, towrite, justwrote;
+	ssize_t didwrite = 0, justwrote;
 
  	/* if we have a seekable stream we need to ensure that data is written at the
  	 * current stream->position. This means invalidating the read buffer and then
@@ -1089,29 +1118,32 @@ static size_t _php_stream_write_buffer(php_stream *stream, const char *buf, size
 
 
 	while (count > 0) {
-		towrite = count;
+		size_t towrite = count;
 		if (towrite > stream->chunk_size)
 			towrite = stream->chunk_size;
 
 		justwrote = stream->ops->write(stream, buf, towrite);
+		if (justwrote <= 0) {
+			/* If we already successfully wrote some bytes and a write error occurred
+			 * later, report the successfully written bytes. */
+			if (didwrite == 0) {
+				return justwrote;
+			}
+			return didwrite;
+		}
 
-		/* convert justwrote to an integer, since normally it is unsigned */
-		if ((int)justwrote > 0) {
-			buf += justwrote;
-			count -= justwrote;
-			didwrite += justwrote;
+		buf += justwrote;
+		count -= justwrote;
+		didwrite += justwrote;
 
-			/* Only screw with the buffer if we can seek, otherwise we lose data
-			 * buffered from fifos and sockets */
-			if (stream->ops->seek && (stream->flags & PHP_STREAM_FLAG_NO_SEEK) == 0) {
-				stream->position += justwrote;
-			}
-		} else {
-			break;
+		/* Only screw with the buffer if we can seek, otherwise we lose data
+		 * buffered from fifos and sockets */
+		if (stream->ops->seek && (stream->flags & PHP_STREAM_FLAG_NO_SEEK) == 0) {
+			stream->position += justwrote;
 		}
 	}
-	return didwrite;
 
+	return didwrite;
 }
 
 /* push some data through the write filter chain.
@@ -1119,7 +1151,7 @@ static size_t _php_stream_write_buffer(php_stream *stream, const char *buf, size
  * This may trigger a real write to the stream.
  * Returns the number of bytes consumed from buf by the first filter in the chain.
  * */
-static size_t _php_stream_write_filtered(php_stream *stream, const char *buf, size_t count, int flags)
+static ssize_t _php_stream_write_filtered(php_stream *stream, const char *buf, size_t count, int flags)
 {
 	size_t consumed = 0;
 	php_stream_bucket *bucket;
@@ -1157,7 +1189,10 @@ static size_t _php_stream_write_filtered(php_stream *stream, const char *buf, si
 			 * underlying stream */
 			while (brig_inp->head) {
 				bucket = brig_inp->head;
-				_php_stream_write_buffer(stream, bucket->buf, bucket->buflen);
+				if (_php_stream_write_buffer(stream, bucket->buf, bucket->buflen) < 0) {
+					consumed = (ssize_t) -1;
+				}
+
 				/* Potential error situation - eg: no space on device. Perhaps we should keep this brigade
 				 * hanging around and try to write it later.
 				 * At the moment, we just drop it on the floor
@@ -1174,7 +1209,7 @@ static size_t _php_stream_write_filtered(php_stream *stream, const char *buf, si
 		case PSFS_ERR_FATAL:
 			/* some fatal error.  Theoretically, the stream is borked, so all
 			 * further writes should fail. */
-			break;
+			return (ssize_t) -1;
 	}
 
 	return consumed;
@@ -1197,14 +1232,18 @@ PHPAPI int _php_stream_flush(php_stream *stream, int closing)
 	return ret;
 }
 
-PHPAPI size_t _php_stream_write(php_stream *stream, const char *buf, size_t count)
+PHPAPI ssize_t _php_stream_write(php_stream *stream, const char *buf, size_t count)
 {
-	size_t bytes;
+	ssize_t bytes;
 
-	if (buf == NULL || count == 0 || stream->ops->write == NULL) {
+	if (count == 0) {
 		return 0;
 	}
 
+	if (buf == NULL || stream->ops->write == NULL) {
+		return (ssize_t) -1;
+	}
+
 	if (stream->writefilters.head) {
 		bytes = _php_stream_write_filtered(stream, buf, count, PSFS_FLAG_NORMAL);
 	} else {
@@ -1218,9 +1257,9 @@ PHPAPI size_t _php_stream_write(php_stream *stream, const char *buf, size_t coun
 	return bytes;
 }
 
-PHPAPI size_t _php_stream_printf(php_stream *stream, const char *fmt, ...)
+PHPAPI ssize_t _php_stream_printf(php_stream *stream, const char *fmt, ...)
 {
-	size_t count;
+	ssize_t count;
 	char *buf;
 	va_list ap;
 
@@ -1229,7 +1268,7 @@ PHPAPI size_t _php_stream_printf(php_stream *stream, const char *fmt, ...)
 	va_end(ap);
 
 	if (!buf) {
-		return 0; /* error condition */
+		return -1; /* error condition */
 	}
 
 	count = php_stream_write(stream, buf, count);
@@ -1306,9 +1345,9 @@ PHPAPI int _php_stream_seek(php_stream *stream, zend_off_t offset, int whence)
 	/* emulate forward moving seeks with reads */
 	if (whence == SEEK_CUR && offset >= 0) {
 		char tmp[1024];
-		size_t didread;
-		while(offset > 0) {
-			if ((didread = php_stream_read(stream, tmp, MIN(offset, sizeof(tmp)))) == 0) {
+		ssize_t didread;
+		while (offset > 0) {
+			if ((didread = php_stream_read(stream, tmp, MIN(offset, sizeof(tmp)))) <= 0) {
 				return -1;
 			}
 			offset -= didread;
@@ -1361,11 +1400,11 @@ PHPAPI int _php_stream_truncate_set_size(php_stream *stream, size_t newsize)
 	return php_stream_set_option(stream, PHP_STREAM_OPTION_TRUNCATE_API, PHP_STREAM_TRUNCATE_SET_SIZE, &newsize);
 }
 
-PHPAPI size_t _php_stream_passthru(php_stream * stream STREAMS_DC)
+PHPAPI ssize_t _php_stream_passthru(php_stream * stream STREAMS_DC)
 {
 	size_t bcount = 0;
 	char buf[8192];
-	size_t b;
+	ssize_t b;
 
 	if (php_stream_mmap_possible(stream)) {
 		char *p;
@@ -1392,13 +1431,17 @@ PHPAPI size_t _php_stream_passthru(php_stream * stream STREAMS_DC)
 		bcount += b;
 	}
 
+	if (b < 0 && bcount == 0) {
+		return b;
+	}
+
 	return bcount;
 }
 
 
 PHPAPI zend_string *_php_stream_copy_to_mem(php_stream *src, size_t maxlen, int persistent STREAMS_DC)
 {
-	size_t ret = 0;
+	ssize_t ret = 0;
 	char *ptr;
 	size_t len = 0, max_len;
 	int step = CHUNK_SIZE;
@@ -1419,7 +1462,8 @@ PHPAPI zend_string *_php_stream_copy_to_mem(php_stream *src, size_t maxlen, int
 		ptr = ZSTR_VAL(result);
 		while ((len < maxlen) && !php_stream_eof(src)) {
 			ret = php_stream_read(src, ptr, maxlen - len);
-			if (!ret) {
+			if (ret <= 0) {
+				// TODO: Propagate error?
 				break;
 			}
 			len += ret;
@@ -1450,7 +1494,8 @@ PHPAPI zend_string *_php_stream_copy_to_mem(php_stream *src, size_t maxlen, int
 	result = zend_string_alloc(max_len, persistent);
 	ptr = ZSTR_VAL(result);
 
-	while ((ret = php_stream_read(src, ptr, max_len - len)))	{
+	// TODO: Propagate error?
+	while ((ret = php_stream_read(src, ptr, max_len - len)) > 0){
 		len += ret;
 		if (len + min_room >= max_len) {
 			result = zend_string_extend(result, max_len + step, persistent);
@@ -1475,9 +1520,8 @@ PHPAPI zend_string *_php_stream_copy_to_mem(php_stream *src, size_t maxlen, int
 PHPAPI int _php_stream_copy_to_stream_ex(php_stream *src, php_stream *dest, size_t maxlen, size_t *len STREAMS_DC)
 {
 	char buf[CHUNK_SIZE];
-	size_t readchunk;
 	size_t haveread = 0;
-	size_t didread, didwrite, towrite;
+	size_t towrite;
 	size_t dummy;
 	php_stream_statbuf ssbuf;
 
@@ -1512,7 +1556,11 @@ PHPAPI int _php_stream_copy_to_stream_ex(php_stream *src, php_stream *dest, size
 		p = php_stream_mmap_range(src, php_stream_tell(src), maxlen, PHP_STREAM_MAP_MODE_SHARED_READONLY, &mapped);
 
 		if (p) {
-			didwrite = php_stream_write(dest, p, mapped);
+			ssize_t didwrite = php_stream_write(dest, p, mapped);
+			if (didwrite < 0) {
+				*len = 0;
+				return FAILURE;
+			}
 
 			php_stream_mmap_unmap_ex(src, mapped);
 
@@ -1529,7 +1577,8 @@ PHPAPI int _php_stream_copy_to_stream_ex(php_stream *src, php_stream *dest, size
 	}
 
 	while(1) {
-		readchunk = sizeof(buf);
+		size_t readchunk = sizeof(buf);
+		ssize_t didread;
 
 		if (maxlen && (maxlen - haveread) < readchunk) {
 			readchunk = maxlen - haveread;
@@ -1537,7 +1586,7 @@ PHPAPI int _php_stream_copy_to_stream_ex(php_stream *src, php_stream *dest, size
 
 		didread = php_stream_read(src, buf, readchunk);
 
-		if (didread) {
+		if (didread > 0) {
 			/* extra paranoid */
 			char *writeptr;
 
@@ -1545,9 +1594,9 @@ PHPAPI int _php_stream_copy_to_stream_ex(php_stream *src, php_stream *dest, size
 			writeptr = buf;
 			haveread += didread;
 
-			while(towrite) {
-				didwrite = php_stream_write(dest, writeptr, towrite);
-				if (didwrite == 0) {
+			while (towrite) {
+				ssize_t didwrite = php_stream_write(dest, writeptr, towrite);
+				if (didwrite <= 0) {
 					*len = haveread - (didread - towrite);
 					return FAILURE;
 				}