Hold the shared-handle test to the upload, and cover a failed read

CHANGELOG.md

@@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
-- **A curl upload took another coroutine's completion and freed it** (#291). `curl_async_read` submits `ZEND_ASYNC_IO_READ` and parks on `io->event`, the event shared by every operation on that descriptor, and `curl_async_read_complete` took whatever request the notification carried: the next `curl_async_read` copied that request's buffer into curl's upload buffer and called `req->dispose(req)`, while `php_stdiop_write` freed the same request as its owner. A file handle passed as `CURLOPT_READDATA` and written by another coroutine ended the process with `zend_mm_heap corrupted (double free)`: five runs of five on the script the issue carries, four naming the double free and one going straight to SIGSEGV, with a gdb trace on `libuv_io_req_dispose` showing one address freed by `php_stdiop_write` (main/streams/plain_wrapper.c), then five times by `curl_async_read`, then by `php_stdiop_write` again. The upload also carries the foreign write's bytes rather than the file's, since `req->buf` of a write request is the source of that write — that one follows from the code and was not observed on the wire. php-src now has the read state remember the request it submitted, and a completion for any other request returns without touching the state. A notification with no result is the handle closing, and the reactor sends it in two senses: with an exception every request has ended, without one the descriptor stays open for the requests still in the thread pool and they report themselves later — a read waiting for one of those keeps waiting instead of failing its upload. Three requests that had nobody to release them are released with it: the one in flight when the handle fails, the one in flight when the read state is freed, and the one whose read ended with an exception, which `curl_async_read` took for the end of the file and turned into a silently truncated body. A cancelled state also drops its subscription before freeing itself, so a later notification on that descriptor does not read through it. Of the php-src consumers of this event only curl went unfiltered; `php_stdiop_read` and `php_stdiop_write` have their own `while (!req->completed)` loop. Evidence: `tests/curl/070-read_takes_only_its_completion.phpt`, SIGSEGV in 5 runs of 5 before the engine change and 10 green runs of 10 after. Needs php-src `8f25e009bef`.
+- **A curl upload took another coroutine's completion and freed it** (#291). `curl_async_read` submits `ZEND_ASYNC_IO_READ` and parks on `io->event`, the event shared by every operation on that descriptor, and `curl_async_read_complete` took whatever request the notification carried: the next `curl_async_read` copied that request's buffer into curl's upload buffer and called `req->dispose(req)`, while `php_stdiop_write` freed the same request as its owner. A file handle passed as `CURLOPT_READDATA` and written by another coroutine ended the process with `zend_mm_heap corrupted (double free)`: five runs of five on the script the issue carries, four naming the double free and one going straight to SIGSEGV, with a gdb trace on `libuv_io_req_dispose` showing one address freed by `php_stdiop_write` (main/streams/plain_wrapper.c), then five times by `curl_async_read`, then by `php_stdiop_write` again. The upload also carries the foreign write's bytes rather than the file's, since `req->buf` of a write request is the source of that write — that one follows from the code and was not observed on the wire. php-src now has the read state remember the request it submitted, and a completion for any other request returns without touching the state. For the io a stream hands out, a notification with no result is the handle closing, and the reactor sends it in two senses (elsewhere the same shape means something else: a successful `fs_open` reports it too): with an exception every request has ended, without one the descriptor stays open for the requests still in the thread pool and they report themselves later — a read waiting for one of those keeps waiting instead of failing its upload. Four requests that had nobody to release them are released with it: the one in flight when the handle fails, the one in flight when the read state is freed, the one whose read failed — that one arrives with the request as the result and its own exception beside it, so it reaches the failure branch and is released there; and the one a cancelled state was given by the completion that frees it. A cancelled state also drops its subscription before freeing itself, so a later notification on that descriptor does not read through it. Of the php-src consumers of this event only curl went unfiltered; `php_stdiop_read` and `php_stdiop_write` have their own `while (!req->completed)` loop. A read that fails is notified with the request as the result and the request's own exception beside it, so it reaches the branch that handles a failure of the event layer, and that branch is where it is released: an upload from a handle opened for writing leaked 528 bytes of request per read on that path, where the read fails at submit and the buffer is never taken; a read that fails later carries the buffer with it, which a debug build reports against `libuv_reactor.c`. Evidence: `tests/curl/070-read_takes_only_its_completion.phpt`, SIGSEGV in 5 runs of 5 before the engine change and 5 green runs of 5 after, with the writer held back until curl reports bytes on the wire, by the clock rather than by a count of scheduler turns, and saying so in the output when it gave up waiting; the leak has no test of its own: an upload from a write-only handle fails the read on this machine and is accepted on the CI runner, so what a test could assert there is nothing, and the evidence stays the debug build's leak report against `libuv_reactor.c(6072)`, counted by hand. Two of those follow from the filter rather than standing on their own, and neither is reproduced by a script. The read callback answers PAUSE while a read of its own is in flight: libcurl asks again whenever the transfer is unpaused, and once a request is told from a stranger by its address, a second request turns the first into a stranger — freed by nobody, its bytes missing from the body. And a state whose handle closed under it ends the upload rather than asking the reactor for a read on a descriptor that is gone, which is what the early return on a close leaves it to do. Needs php-src `06198885ee7`.
 
 - **Two coroutines decompressing entries of one phar wrote over each other** (#283). Every compressed entry is decompressed into one stream shared by the archive, at the offset the entry takes from that stream before it starts writing, and every call of that span parks: seek to the end, remember the offset, append the decompression filter, copy from the archive stream, flush, remove the filter. A second entry entering the span appended its own filter to the same write chain and put its bytes at the offset the first had remembered, so the first read back a length that matched nothing: four gzipped entries of one archive, read by four coroutines, answered `internal corruption of phar "..." (actual filesize mismatch on file "...")` for three of the four on every run. php-src holds the span as one unit now - both sides of the archive stream around it, both sides of the uncompressed-file stream inside them, taken in the order the stream layer itself takes them - and holds an archive reference for the call, because `phar_get_entry_data()` takes its own only after the entry has been opened: without it the coroutine that finished first dropped the last reference and `phar_archive_delref()` closed the archive stream the others were queued on. Reading an archive is serialized per archive as a result, stored entries included. Evidence: `tests/io/101-phar_entries_decompressed_at_once.phpt`, three broken entries of four before the change and 10 green runs of 10 after. Needs php-src `353ccf7b070`.
 

tests/curl/070-read_takes_only_its_completion.phpt

@@ -7,6 +7,7 @@ curl
 
 use function Async\spawn;
 use function Async\await_all;
+use function Async\suspend;
 
 include __DIR__ . '/../../../../ext/curl/tests/server.inc';
 $host = curl_cli_server_start();
@@ -33,13 +34,39 @@ curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
 curl_setopt($ch, CURLOPT_READDATA, $handle);
 curl_setopt($ch, CURLOPT_HTTPHEADER, ['Expect:', 'Content-Length: 200000']);
 
+/* The writes have to fall inside the upload, and nothing else orders the two:
+ * the writer waits until curl reports bytes on the wire. */
+$uploading = false;
+curl_setopt($ch, CURLOPT_NOPROGRESS, false);
+curl_setopt($ch, CURLOPT_PROGRESSFUNCTION,
+    function ($resource, $downloadSize, $downloaded, $uploadSize, $uploaded) use (&$uploading) {
+        if ($uploaded > 0) {
+            $uploading = true;
+        }
+
+        return 0;
+    });
+
 [$results, $errors] = await_all([
     spawn(function () use ($ch) {
         $response = curl_exec($ch);
 
         return 'response: ' . (is_string($response) ? $response : 'error #' . curl_errno($ch));
     }),
-    spawn(function () use ($handle) {
+    spawn(function () use ($handle, &$uploading) {
+        /* Waits by the clock rather than by a count of turns: an empty
+         * scheduler spins through thousands of them while curl is still
+         * connecting. The deadline is there so that a build whose progress
+         * callback never reports runs the writes instead of hanging. */
+        $deadline = microtime(true) + 5.0;
+        while (!$uploading && microtime(true) < $deadline) {
+            suspend();
+        }
+
+        if (!$uploading) {
+            echo "the upload never reported a byte\n";
+        }
+
         $chunk = str_repeat('w', 8192);
         for ($i = 0; $i < 100; $i++) {
             @fwrite($handle, $chunk);