Fix GH-17345: Bug #35916 was not completely fixed

Change the reproducer code in `bug35916.phpt` from `stream_bucket_append` to
`stream_bucket_prepend` and you have the same bug.
Furthermore, even in the append case the check is incorrect because the bucket
can already be in the brigade at a position other than the tail.
To solve this properly, unlink the brigade first and also use that as a
condition to manage the refcount.

Closes GH-18973.

NEWS

@@ -55,6 +55,7 @@ PHP                                                                        NEWS
 - Streams:
   . Fixed bug GH-19248 (Use strerror_r instead of strerror in main).
     (Jakub Zelenka)
+  . Fixed bug GH-17345 (Bug #35916 was not completely fixed). (nielsdos)
 
 - XMLReader:
   . Fixed bug GH-20009 (XMLReader leak on RelaxNG schema failure). (nielsdos)

ext/standard/tests/filters/gh17345.phpt

@@ -0,0 +1,49 @@
+--TEST--
+GH-17345 (Bug #35916 was not completely fixed)
+--FILE--
+<?php
+$file = __DIR__ . "/gh17345.txt";
+@unlink($file);
+
+class strtoupper_filter extends php_user_filter
+{
+    function filter($in, $out, &$consumed, $closing): int
+    {
+        while ($bucket=stream_bucket_make_writeable($in)) {
+            $bucket->data = strtoupper($bucket->data);
+            $consumed += $bucket->datalen;
+            stream_bucket_prepend($out, $bucket);
+            // Interleave new bucket
+            stream_bucket_prepend($out, clone $bucket);
+            stream_bucket_prepend($out, $bucket);
+        }
+        return PSFS_PASS_ON;
+    }
+
+    function onCreate(): bool
+    {
+        echo "fffffffffff\n";
+        return true;
+    }
+
+    function onClose(): void
+    {
+        echo "hello\n";
+    }
+}
+
+stream_filter_register("strtoupper", "strtoupper_filter");
+$fp=fopen($file, "w");
+stream_filter_append($fp,  "strtoupper");
+fread($fp, 1024);
+fwrite($fp, "Thank you\n");
+fclose($fp);
+readfile($file);
+unlink($file);
+?>
+--EXPECTF--
+fffffffffff
+
+Notice: fread(): Read of 8192 bytes failed with errno=9 Bad file descriptor in %s on line %d
+hello
+THANK YOU

ext/standard/user_filters.c

@@ -404,17 +404,19 @@ static void php_stream_bucket_attach(int append, INTERNAL_FUNCTION_PARAMETERS)
 		memcpy(bucket->buf, Z_STRVAL_P(pzdata), bucket->buflen);
 	}
 
+	/* If the bucket is already on a brigade we have to unlink it first to keep the
+	 * linked list consistent. Furthermore, we can transfer the refcount in that case. */
+	if (bucket->brigade) {
+		php_stream_bucket_unlink(bucket);
+	} else {
+		bucket->refcount++;
+	}
+
 	if (append) {
 		php_stream_bucket_append(brigade, bucket);
 	} else {
 		php_stream_bucket_prepend(brigade, bucket);
 	}
-	/* This is a hack necessary to accommodate situations where bucket is appended to the stream
- 	 * multiple times. See bug35916.phpt for reference.
-	 */
-	if (bucket->refcount == 1) {
-		bucket->refcount++;
-	}
 }
 /* }}} */
 

main/streams/filter.c

@@ -173,6 +173,7 @@ PHPAPI void php_stream_bucket_prepend(php_stream_bucket_brigade *brigade, php_st
 
 PHPAPI void php_stream_bucket_append(php_stream_bucket_brigade *brigade, php_stream_bucket *bucket)
 {
+	/* TODO: this was added as a bad workaround for bug #35916 and should be removed in the future. */
 	if (brigade->tail == bucket) {
 		return;
 	}