Repository navigation
Conversation
|
Nice. Does this work correctly if the |
9177fd8 to
ec8ddd8
Compare
|
Ah, it didn't: the kept handle's handlers were freed while it stayed reachable, so a later GC run could read freed memory. The callback now only clears the easy handle and drops its own reference, and |
bukka
left a comment
bukka
left a comment
There was a problem hiding this comment.
I will need to look into this more but in general the approach seems sane. It will just need a proper checking if that changed semantic is safe.
| return rval; | ||
| } | ||
|
|
||
| ch->cp = NULL; |
There was a problem hiding this comment.
I think it might be better to re-init object here as the handling between different functions (e.g. curl_reset, curl_setopt, curl_getinfo) seems quite inconsistent
There was a problem hiding this comment.
Reworked along those lines: a denied handle the callback keeps now gets a fresh easy handle with curl_init()'s defaults and its own option lists, so curl_reset(), curl_getinfo(), copy and clone behave as on a new handle. A handle the callback drops is freed as before.
| #endif | ||
|
|
||
| if (!ch->cp) { | ||
| if (!ch->clone) { |
There was a problem hiding this comment.
Couldn't this result in ch->clone != NULL no longer implying the ch->cp != NULL? If so, is that actually safe?
There was a problem hiding this comment.
Yes, but now only for a denied handle that is being released, or one kept when curl_easy_init() fails in the callback. The dtor is safe there: curl_easy_cleanup(NULL) is a no-op and _php_curl_verify_handlers() only passes the NULL handle to curl_easy_setopt(), which rejects it.
| function do_request(): void { | ||
| $callback = function ($parent_ch, $pushed_ch, array $headers) { | ||
| $GLOBALS['push_count']++; | ||
| return CURL_PUSH_DENY; | ||
| }; | ||
|
|
||
| $mh = curl_multi_init(); | ||
| curl_multi_setopt($mh, CURLMOPT_PIPELINING, CURLPIPE_MULTIPLEX); | ||
| curl_multi_setopt($mh, CURLMOPT_PUSHFUNCTION, $callback); | ||
|
|
||
| $ch = curl_init(); | ||
| curl_setopt($ch, CURLOPT_URL, "https://localhost/serverpush"); | ||
| curl_setopt($ch, CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_2_0); | ||
| curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1); | ||
| curl_multi_add_handle($mh, $ch); | ||
|
|
||
| $done = false; | ||
| do { | ||
| curl_multi_exec($mh, $active); | ||
|
|
||
| do { | ||
| $info = curl_multi_info_read($mh); | ||
| if (false !== $info && $info['msg'] == CURLMSG_DONE) { | ||
| $handle = $info['handle']; | ||
| if ($handle !== null) { | ||
| curl_multi_remove_handle($mh, $handle); | ||
| curl_close($handle); | ||
| $done = true; | ||
| } | ||
| } | ||
| } while ($info); | ||
| } while (!$done); | ||
|
|
||
| curl_multi_close($mh); | ||
| } | ||
|
|
||
| $iterations = 200; | ||
| $bytes_per_iteration_limit = 512; | ||
|
|
||
| do_request(); | ||
| gc_collect_cycles(); | ||
|
|
||
| $before = memory_get_usage(); | ||
| for ($i = 0; $i < $iterations; $i++) { | ||
| do_request(); | ||
| } | ||
| gc_collect_cycles(); | ||
| $growth = memory_get_usage() - $before; | ||
|
|
||
| if ($push_count < $iterations) { | ||
| printf("too few pushes handled: %d\n", $push_count); | ||
| } else { | ||
| echo "all pushes denied\n"; | ||
| } | ||
| if ($growth > $iterations * $bytes_per_iteration_limit) { | ||
| printf("leaked %d bytes (%d bytes/iteration)\n", $growth, (int) ($growth / $iterations)); | ||
| } else { | ||
| echo "no leak detected\n"; | ||
| } |
There was a problem hiding this comment.
This seems a bit flaky or at least a bit hacky. Does it even test the push? This might need some re-thinking
There was a problem hiding this comment.
Rewrote it: the callback records that the push arrived and keeps only a WeakReference, and the test asserts the reference is dead once the transfer completes.
When the push callback denies the push or throws, libcurl destroys the pushed easy handle. The CurlHandle created for it was never released, so it leaked along with its share of the parent's free lists, and a callback that kept it was left with a handle that had no easy handle, on which curl_reset(), curl_copy_handle() and clone crashed. Release the wrapper when the callback dropped it; otherwise give it a new easy handle with curl_init() defaults and its own free lists, detached from the parent. A share set from the callback is detached from the doomed easy handle first so it can still be cleaned up, and curl_free_obj() tests ch->clone for the constructor-failure case, since a released wrapper has no easy handle.
ec8ddd8 to
114ba3f
Compare
When a CURLMOPT_PUSHFUNCTION callback denies the push or throws, libcurl destroys the pushed easy handle itself, but the CurlHandle created for it was never released, so each denied push leaked it along with its share of the parent's option lists. A handle the callback drops is now released; one it keeps gets a fresh easy handle with curl_init()'s defaults and its own option lists, so curl_reset(), curl_copy_handle() and clone work on it instead of crashing on a NULL easy handle. The new tests need Caddy, so they only run in the lanes that have it.