fgets() does not return false on error from a zlib.inflate filtered stream with large content and invalid checksum

Description

The following code:

<?php

// Prepare a big enough input so that it is not entirely buffered
$stream = fopen('php://memory', 'r+');
$content = '';
for ($i = 0; $i < 10000; $i++) {
	$content .= "Hello $i\n";
}
fwrite($stream, gzcompress($content));

// Mess up the checksum
fseek($stream, -1, SEEK_CUR);
fwrite($stream, '1');

// Rewind and add the zlib filter
rewind($stream);
stream_filter_append($stream, 'zlib.inflate', STREAM_FILTER_READ, ['window' => 15]);

// Read the filtered stream line by line.
while (($line = fgets($stream)) !== false) {
	$error = error_get_last();
	if ($error !== null) {
		// An error is thrown but fgets didn't return false
		var_dump(error_get_last());
		var_dump($line);
	}
}

fclose($stream);

Resulted in this output:

PHP Notice:  fgets(): zlib: data error in /home/nicolas/Source/php/sphinx-inventory-parser/test.php on line 20
PHP Stack trace:
PHP   1. {main}() /home/nicolas/Source/php/sphinx-inventory-parser/test.php:0
PHP   2. fgets($stream = resource(5) of type (stream)) /home/nicolas/Source/php/sphinx-inventory-parser/test.php:20
/home/nicolas/Source/php/sphinx-inventory-parser/test.php:24:
array(4) {
  'type' =>
  int(8)
  'message' =>
  string(25) "fgets(): zlib: data error"
  'file' =>
  string(57) "/home/nicolas/Source/php/sphinx-inventory-parser/test.php"
  'line' =>
  int(20)
}
/home/nicolas/Source/php/sphinx-inventory-parser/test.php:25:
string(7) "Hello 6"

But I expected this output instead:

PHP Notice:  fgets(): zlib: data error in /home/nicolas/Source/php/sphinx-inventory-parser/test.php on line 20
PHP Stack trace:
PHP   1. {main}() /home/nicolas/Source/php/sphinx-inventory-parser/test.php:0
PHP   2. fgets($stream = resource(5) of type (stream)) /home/nicolas/Source/php/sphinx-inventory-parser/test.php:20

As fgets manual says:

If an error occurs, false is returned.

Also note that by replacing 10000 with a smaller number, say 1000 I get the expected output.

PHP Version

PHP 8.2.12

Operating System

Debian testing

Can reproduce, and changing to 1000 gets the expected behaviour. Thanks for the report.
Bonus points for the memory leak when using 10000...

Another strange thing that may be related is that adding var_dump(feof($stream)); just before the fclose($stream), with either 10000 or 1000, always prints bool(true). I thought that this function could be used to determine if no error happened as we can see in the example.

Yes, it's supposed to set the stream to eof on PSFS_ERR_FATAL such that feof($stream) return false, and therein lies the implementation error probably.

The problem is that PSFS_ERR_FATAL is returned from php_zlib_inflate_filter to php_stream_fill_read_buffer and that propagates FAILURE to its caller _php_stream_get_line inside the switch statement. However, _php_stream_get_line does not check the error state.
So this would fix the error propagation, but does not yet fix the bucket leaks:

diff --git a/main/streams/streams.c b/main/streams/streams.c
index d45a9bfab8..aa4e949d7f 100644
--- a/main/streams/streams.c
+++ b/main/streams/streams.c
@@ -981,7 +981,12 @@ PHPAPI char *_php_stream_get_line(php_stream *stream, char *buf, size_t maxlen,
 				}
 			}
 
-			php_stream_fill_read_buffer(stream, toread);
+			if (UNEXPECTED(php_stream_fill_read_buffer(stream, toread) != SUCCESS)) {
+				if (grow_mode) {
+					efree(bufstart);
+				}
+				return NULL;
+			}
 
 			if (stream->writepos - stream->readpos == 0) {
 				break;

EDIT: running a test with the leak fix too on my fork ndossche#86

@bukka

Test PR (contains leak and behaviour fix) seems to have passed.

I'm starting to doubt the expected behaviour here though. The memory leak is no doubt a real issue. The behaviour issue might be intended but unexpected behaviour... What happens is that the read buffer fills up, then errors out and marks the stream as borked. But there is still data in the read buffer at that point, so we can still get lines until the read buffer is depleted and fgets returns false. So it might be intended. cc @bukka any thoughts on this?

What happens is that the read buffer fills up, then errors out and marks the stream as borked. But there is still data in the read buffer at that point, so we can still get lines until the read buffer is depleted and fgets returns false. So it might be intended.

The thing is, the data is considered to be corrupted since the checksum failed (it is not in my example since I altered the checksum and not the data, but it will probably be the other way around in real life). Returning garbage data in fgets is not useful.

Also, even if fgets returns false at the end of the buffer, feof does return true. So if the fgets error is silenced, it can not be detected with feof later (this is the issue that made me discover this behaviour in the first place).

I don't think php_stream_get_line should fail if php_stream_fill_read_buffer returns just FAILURE. I think it could break some scenarios. I haven't verified this but wouldn't that potentially break a situation where other side of connection closes the connection after writing all data and this side of connection tries to fill more data to the buffer? In such case I would imagine the php_stream_get_line (fgets) still returns data that it has got buffered. I think there might be some other cases where it makes more sense to return what's in the buffer even if there is some fail.

It might make more sense to introduce some flag to mark the error terminating and in such situation fail. In this case PSFS_ERR_FATAL could be consider for that. It just sets eof atm moment and checking only eof might still limit some use case so maybe some new flag would make more sense. So essentially something like this

diff --git a/main/php_streams.h b/main/php_streams.h
index 31b80de986..8996ee8bcb 100644
--- a/main/php_streams.h
+++ b/main/php_streams.h
@@ -223,6 +223,9 @@ struct _php_stream  {
        /* whether stdio cast flushing is in progress */
        uint16_t fclose_stdiocast_flush_in_progress:1;
 
+       /* whether fatal error happened and all operations should terminates as soon as possible */
+       uint16_t fatal_error:1;
+
        char mode[16];                  /* "rwb" etc. ala stdio */
 
        uint32_t flags; /* PHP_STREAM_FLAG_XXX */
diff --git a/main/streams/streams.c b/main/streams/streams.c
index 9f79821f0c..e2a55e62a8 100644
--- a/main/streams/streams.c
+++ b/main/streams/streams.c
@@ -636,6 +636,7 @@ PHPAPI zend_result _php_stream_fill_read_buffer(php_stream *stream, size_t size)
                                        /* some fatal error. Theoretically, the stream is borked, so all
                                         * further reads should fail. */
                                        stream->eof = 1;
+                                       stream->fatal_error = 1;
                                        efree(chunk_buf);
                                        retval = FAILURE;
                                        goto out_is_eof;
@@ -998,7 +999,12 @@ PHPAPI char *_php_stream_get_line(php_stream *stream, char *buf, size_t maxlen,
                                }
                        }
 
-                       php_stream_fill_read_buffer(stream, toread);
+                       if (php_stream_fill_read_buffer(stream, toread) == FAILURE && stream->fatal_error) {
+                               if (grow_mode) {
+                                       efree(bufstart);
+                               }
+                               return NULL;
+                       }
 
                        if (stream->writepos - stream->readpos == 0) {
                                break;

This would be just for PHP-8.3 and master. PHP-8.2 is using uint8_t but there should be some space due to alignment and still possibly one flag free but not sure if it's not too risky for 8.2 anyway.

Ideally we could let user decide which error is terminating but then it comes to an error propagation in general. That's a bigger problem and I plan to be looking into it soon. The idea is to potentially introduce some sort of stream error handler that would be called on each error and the decision whether to continue could be done there.

sure, no probs, will handle it hopefully next week.

just a quick update that I have been really busy last and this week but should hopefully find some time on this later next week.

Fix phpGH-13264: Part 1 - Memory leak on filter failure

Part one of the fix in #13790 - memory leak fix

Fix GH-13264: Part 1 - Memory leak on filter failure

Re-opening as only part of this is fixed now.

Fix phpGH-13264: fgets() and stream_get_line() do not return false on…

The second part took me a bit longer but PR finally created: #18778

Fix GH-13264: fgets() and stream_get_line() do not return false on fi…

Navigation Menu

Repository navigation

fgets() does not return false on error from a zlib.inflate filtered stream with large content and invalid checksum #13264

Description

PHP Version

Operating System

5 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

fgets() does not return false on error from a zlib.inflate filtered stream with large content and invalid checksum #13264

Description

Description

PHP Version

Operating System

Activity

ndossche commented on Jan 29, 2024

n-peugnet commented on Jan 29, 2024

ndossche commented on Jan 29, 2024

ndossche commented on Jan 29, 2024

ndossche commented on Jan 30, 2024

n-peugnet commented on Jan 30, 2024

bukka commented on Feb 3, 2024

5 remaining items

bukka commented on Feb 22, 2024

bukka commented on Mar 9, 2024

bukka commented on Mar 23, 2024

bukka commented on Mar 29, 2024

bukka commented on Jun 5, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions