Skip to content

Close the upload stream under a parked curl read - #296

Merged
EdmondDantes merged 2 commits into
mainfrom
295-curl-read-state-lifetime
Sep 10, 2026
Merged

EdmondDantes merged 2 commits into
mainfrom
295-curl-read-state-lifetime

Conversation

@EdmondDantes

Copy link
Copy Markdown
Contributor

Covers #295. Pairs with true-async/php-src#34, which carries the fix.

The test closes the handle curl uploads from, from a second coroutine, between two
of curl's reads. CURLOPT_MAX_SEND_SPEED_LARGE spreads the upload over seconds so
that the close lands with no read in flight — the state in which the reactor holds
no pin of its own on the stream's IO and the subscription has to keep it alive by
itself. Before the php-src change:

==6146==ERROR: AddressSanitizer: heap-use-after-free
    #0 curl_async_read_state_free ext/curl/curl_async.c:1425
  freed by thread T0 here:
    #3 libuv_io_event_dispose ext/async/libuv_reactor.c:4727
    #4 php_stdiop_close main/streams/plain_wrapper.c:962
    #6 zif_fclose ext/standard/file.c:779

Ordered by the progress callback rather than by a count of scheduler turns, and
bounded by a deadline that says out loud when it gave up, following
070-read_takes_only_its_completion.phpt.

The ASAN run needs USE_ZEND_ALLOC=0: the io is freed through ZendMM, and without
that the sanitizer never sees the free.

The second half of the php-src change, #294, has no test here. The dangling event
pointer of a CURLFile read state was observed directly, with a read in flight, by
instrumenting the event's destructor, but no script made a completion land in the
window between that destructor and libcurl's free_cb — ten attempts, including a
fifo source, which does not work for CURLFile because the part's length comes from
stat, and a single-thread UV_THREADPOOL_SIZE with eight concurrent uploads. The
CHANGELOG entry says so.

Checks

  • ext/async/tests/curl under ASAN — 68 passed, 0 failed.
  • ext/async/tests — 1194 passed, 0 failed.
  • fuzzy-tests/_generated — 724 passed, 0 failed.
  • This test on a rebuild of php-src with the fix reverted: fails. With it: passes.

The test closes the handle curl uploads from, from another coroutine, between
two of curl's reads: the transfer is throttled so that no read is in flight at
that moment, which is the state in which the reactor holds no pin of its own and
the subscription has to keep the io alive by itself. Before the php-src change it
is a heap-use-after-free under ASAN in curl_async_read_state_free().

The close is ordered by the progress callback rather than by a count of
scheduler turns, and the wait is bounded so that a build whose progress callback
never reports says so instead of hanging.

#295
@codecov

codecov Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Below libcurl 8.11.1 curl_async_read does a synchronous read(2): curl_exec pushes
the whole body before the closing coroutine gets a turn, so the close lands after
the transfer and the assertion reads "completed" instead of "aborted". The
subscription the test covers is not built there either -- it sits behind the same
version guard. Measured against libcurl 8.5.0, the version the Linux CI builds
from source, and 7.87.0, the declared minimum.

#295
@EdmondDantes
EdmondDantes merged commit 57e1219 into main Sep 10, 2026
9 checks passed
@EdmondDantes
EdmondDantes deleted the 295-curl-read-state-lifetime branch September 10, 2026 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant