Propagate errors from stream read/write operations

[nikic]

This is like #4433 but also extended to read operations.

[cmb69]

@nikic, I can reproduce ext/mysqli/tests/mysqli_fetch_array_large.phpt hanging. Will try to debug.

[nikic]

@cmb69 I think I need to adjust some usages of php_stream_read in mysqlnd...

[cmb69]

@nikic, yes that should do. For me, fixing mysqlnd_vio.c:90 already let the test succeed.

[cmb69]

Windows test fixes:

 ext/standard/tests/file/007_variation11-win32-mb.phpt | 4 ++--
 ext/standard/tests/file/007_variation11-win32.phpt    | 4 ++--
 ext/standard/tests/file/007_variation13-win32.phpt    | 4 ++--
 ext/standard/tests/streams/egain_is_not_an_error.phpt | 4 ++++
 4 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/ext/standard/tests/file/007_variation11-win32-mb.phpt b/ext/standard/tests/file/007_variation11-win32-mb.phpt
index b0d84dfe93..c2e42f68d2 100644
--- a/ext/standard/tests/file/007_variation11-win32-mb.phpt
+++ b/ext/standard/tests/file/007_variation11-win32-mb.phpt
@@ -40,7 +40,7 @@ var_dump( ftell($file_handle) );  //Initial file pointer position, expected at t
 var_dump( fwrite($file_handle, $string) );  //Check for write operation; passes; expected:size of the $string
 var_dump( ftell($file_handle) );  //File pointer position after write operation, expected at the end of the file
 rewind($file_handle);
-var_dump( fread($file_handle, 100) );  //Check for read operation; fails; expected: empty string
+var_dump( fread($file_handle, 100) );  //Check for read operation; fails; expected: false
 var_dump( ftell($file_handle) );  //File pointer position after read operation, expected at the beginning of the file
 var_dump( fclose($file_handle) );  //Check for close operation on the file handle
 var_dump( get_resource_type($file_handle) );  //Check whether resource is lost after close operation
@@ -68,7 +68,7 @@ string(6) "stream"
 int(0)
 int(37)
 int(37)
-string(0) ""
+bool(false)
 int(0)
 bool(true)
 string(7) "Unknown"
diff --git a/ext/standard/tests/file/007_variation11-win32.phpt b/ext/standard/tests/file/007_variation11-win32.phpt
index 436b22f8eb..7b6ae81465 100644
--- a/ext/standard/tests/file/007_variation11-win32.phpt
+++ b/ext/standard/tests/file/007_variation11-win32.phpt
@@ -40,7 +40,7 @@ var_dump( ftell($file_handle) );  //Initial file pointer position, expected at t
 var_dump( fwrite($file_handle, $string) );  //Check for write operation; passes; expected:size of the $string
 var_dump( ftell($file_handle) );  //File pointer position after write operation, expected at the end of the file
 rewind($file_handle);
-var_dump( fread($file_handle, 100) );  //Check for read operation; fails; expected: empty string
+var_dump( fread($file_handle, 100) );  //Check for read operation; fails; expected: false
 var_dump( ftell($file_handle) );  //File pointer position after read operation, expected at the beginning of the file
 var_dump( fclose($file_handle) );  //Check for close operation on the file handle
 var_dump( get_resource_type($file_handle) );  //Check whether resource is lost after close operation
@@ -66,7 +66,7 @@ string(6) "stream"
 int(0)
 int(37)
 int(37)
-string(0) ""
+bool(false)
 int(0)
 bool(true)
 string(7) "Unknown"
diff --git a/ext/standard/tests/file/007_variation13-win32.phpt b/ext/standard/tests/file/007_variation13-win32.phpt
index 12484cfcde..72292d057f 100644
--- a/ext/standard/tests/file/007_variation13-win32.phpt
+++ b/ext/standard/tests/file/007_variation13-win32.phpt
@@ -37,7 +37,7 @@ var_dump($file_handle);  //Check for the content of handle
 var_dump( get_resource_type($file_handle) );  //Check for the type of resource
 var_dump( fwrite($file_handle, $string) );  //Check for write operation; passes; expected:size of the $string
 rewind($file_handle);
-var_dump( fread($file_handle, 100) );  //Check for read operation; fails; expected: empty string
+var_dump( fread($file_handle, 100) );  //Check for read operation; fails; expected: false
 var_dump( ftell($file_handle) );  //File pointer position after read operation, expected at the end of the file
 var_dump( fclose($file_handle) );  //Check for close operation on the file handle
 var_dump( get_resource_type($file_handle) );  //Check whether resource is lost after close operation
@@ -56,7 +56,7 @@ unlink(__DIR__."/007_variation13.tmp");
 resource(%d) of type (stream)
 string(6) "stream"
 int(37)
-string(0) ""
+bool(false)
 int(0)
 bool(true)
 string(7) "Unknown"
diff --git a/ext/standard/tests/streams/egain_is_not_an_error.phpt b/ext/standard/tests/streams/egain_is_not_an_error.phpt
index db1554547e..7bc50dc9b7 100644
--- a/ext/standard/tests/streams/egain_is_not_an_error.phpt
+++ b/ext/standard/tests/streams/egain_is_not_an_error.phpt
@@ -1,5 +1,9 @@
 --TEST--
 EAGAIN/EWOULDBLOCK on a non-blocking socket should not result in an error return value
+--SKIPIF--
+<?php
+if (substr(PHP_OS, 0, 3) == 'WIN') die('skip not for Windows');
+?>
 --FILE--
 <?php
 

[cmb69]

@nikic, this is an example where cl.exe throws a warning, since we're comparing a ssize_t with a size_t value. If we're sure that toread >= 0 we could simply apply a cast.

[nikic]

I've inserted casts for the two cases that you mentioned, though generally I think that it would be better to suppress those warnings for the MSVC build. We don't have them enabled in the gcc/clang builds.

[cmb69]

I think these are very valid warnings, and instead of suppresing them, we should "fix" the respective code. In this case, for instance, if toread was ever negative, the condition would almost certainly evaluate to true, which is unlikely to be desired. If we're sure that toread can never be negative, adding a cast clarifies this for the compiler and for readers. (If we're not absolutely sure, we could add an additional assertion.)

[weltling]

Perhaps I could attract your attention to some macros in Zend/zend_range_check.h. Something like ZEND_SIZE_T_GT_ZEND_LONG could suffice for the runtime checks. Could also be wrapped with UNEXPECTED where makes sense. Developed back then in 7.0 when there was a lot of stuff to fix for the size_t migration :)

Thanks.

[cmb69]

@nikic, seems this has been committed as d59aac5, but this PR has not been closed.

[nikic]

Yeah, github was down at the time.