From a039381ee132e63ba5a667690e019deebaca4dc7 Mon Sep 17 00:00:00 2001 From: Gennady Kovshenin Date: Sat, 17 Feb 2018 19:57:25 +0500 Subject: [PATCH 01/10] Prevent cURL transport from leaking on Exception The `CURLOPT_HEADERFUNCTION` and `CURLOPT_WRITEFUNCTION` options are not being reset if a cURL handle error occurs (which throws a `Requests_Exception` and stops execution). The destructor is not called due to lingering instance method callbacks in the two options. Move `CURLOPT_HEADERFUNCTION` and `CURLOPT_WRITEFUNCTION` cleanup before any exceptions can happen. --- src/Transport/Curl.php | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/Transport/Curl.php b/src/Transport/Curl.php index 6291ed63f..bf0d6e13e 100644 --- a/src/Transport/Curl.php +++ b/src/Transport/Curl.php @@ -232,12 +232,14 @@ public function request($url, $headers = [], $data = [], $options = []) { $response = $this->response_data; } - $this->process_response($response, $options); + if (true === $options['blocking']) { + // Need to remove the $this reference from the curl handle. + // Otherwise \WpOrg\Requests\Transport\Curl wont be garbage collected and the curl_close() will never be called. + curl_setopt($this->handle, CURLOPT_HEADERFUNCTION, null); + curl_setopt($this->handle, CURLOPT_WRITEFUNCTION, null); + } - // Need to remove the $this reference from the curl handle. - // Otherwise \WpOrg\Requests\Transport\Curl won't be garbage collected and the curl_close() will never be called. - curl_setopt($this->handle, CURLOPT_HEADERFUNCTION, null); - curl_setopt($this->handle, CURLOPT_WRITEFUNCTION, null); + $this->process_response($response, $options); return $this->headers; } From 76be0b7631d6532e94a45e67b6e10d593393221a Mon Sep 17 00:00:00 2001 From: Alain Schlesser Date: Wed, 10 Nov 2021 17:27:02 +0100 Subject: [PATCH 02/10] Assert that cURL handles are correctly closed --- src/Transport/Curl.php | 4 +- tests/Transport/Curl/CurlTest.php | 82 +++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 2 deletions(-) diff --git a/src/Transport/Curl.php b/src/Transport/Curl.php index bf0d6e13e..6238565f5 100644 --- a/src/Transport/Curl.php +++ b/src/Transport/Curl.php @@ -232,9 +232,9 @@ public function request($url, $headers = [], $data = [], $options = []) { $response = $this->response_data; } - if (true === $options['blocking']) { + if (! isset($options['blocking']) || $options['blocking'] !== false) { // Need to remove the $this reference from the curl handle. - // Otherwise \WpOrg\Requests\Transport\Curl wont be garbage collected and the curl_close() will never be called. + // Otherwise \WpOrg\Requests\Transport\Curl won't be garbage collected and the curl_close() will never be called. curl_setopt($this->handle, CURLOPT_HEADERFUNCTION, null); curl_setopt($this->handle, CURLOPT_WRITEFUNCTION, null); } diff --git a/tests/Transport/Curl/CurlTest.php b/tests/Transport/Curl/CurlTest.php index b7a2442d4..1f3385e7a 100644 --- a/tests/Transport/Curl/CurlTest.php +++ b/tests/Transport/Curl/CurlTest.php @@ -3,6 +3,7 @@ namespace WpOrg\Requests\Tests\Transport\Curl; use WpOrg\Requests\Exception; +use WpOrg\Requests\Hooks; use WpOrg\Requests\Requests; use WpOrg\Requests\Tests\Transport\BaseTestCase; use WpOrg\Requests\Transport\Curl; @@ -10,6 +11,55 @@ final class CurlTest extends BaseTestCase { protected $transport = Curl::class; + /** + * Temporary storage of the cURL handle to assert against. + * + * @var null|resource|\CurlHandle + */ + protected $curl_handle; + + /** + * Get the options to use for the cURL tests. + * + * This adds a hook on curl.before_request to store the cURL handle. This is + * needed for asserting after the test scenarios that the cURL handle was + * correctly closed. + * + * @param array $other + * @return array + */ + protected function getOptions($other = array()) { + $options = parent::getOptions($other); + + $this->curl_handle = null; + + $hooks = new Hooks(); + $hooks->register( + 'curl.before_request', + function ($handle) { + $this->curl_handle = $handle; + } + ); + + $options['hooks'] = $hooks; + + return $options; + } + + /** + * Post condition asserts to run after each scenario. + * + * This is used for asserting that cURL handles are not leaking memory. + */ + protected function assert_post_conditions( ) { + if ($this->curl_handle === null) { + // No cURL handle was used during this particular test scenario. + return; + } + + $this->assertCurlHandleIsClosed($this->curl_handle); + } + public function testBadIP() { $this->expectException(Exception::class); $this->expectExceptionMessage('t resolve host'); @@ -136,4 +186,36 @@ public function testSetsEmptyExpectHeaderIfBodySmallerThan1Mb() { $this->assertFalse(isset($result['headers']['Expect'])); } + + /** + * Assert that a provided cURL handle was properly closed. + * + * For PHP 8.0+, the cURL handle is not a resource that needs to be closed, + * but rather a \CurlHandle object. The assertion just skips these. + * + * @param resource|\CurlHandle $handle CURL handle to check. + */ + private function assertCurlHandleIsClosed($handle) { + if ($handle instanceof \CurlHandle) { + // CURL handles have been changed from resources into CurlHandle + // objects starting with PHP 8.0, which don;t need to be closed. + return; + } + + self::assertThat( + in_array( + gettype($handle), + array('resource', 'resource (closed)'), + true + ), + self::isTrue(), + 'Failed asserting that stored cURL handle was a resource.' + ); + + self::assertThat( + is_resource($handle) === false, + self::isTrue(), + 'Failed asserting that stored cURL handle was properly closed.' + ); + } } From 7a56b5c22f3b1c2ce6f98ac04a9c82d01d92ee39 Mon Sep 17 00:00:00 2001 From: Alain Schlesser Date: Wed, 10 Nov 2021 17:48:40 +0100 Subject: [PATCH 03/10] Use assertIsClosedResource from PHPUnit polyfills --- tests/Transport/Curl/CurlTest.php | 40 ++++++------------------------- 1 file changed, 7 insertions(+), 33 deletions(-) diff --git a/tests/Transport/Curl/CurlTest.php b/tests/Transport/Curl/CurlTest.php index 1f3385e7a..165e1fcb7 100644 --- a/tests/Transport/Curl/CurlTest.php +++ b/tests/Transport/Curl/CurlTest.php @@ -57,7 +57,13 @@ protected function assert_post_conditions( ) { return; } - $this->assertCurlHandleIsClosed($this->curl_handle); + if ($this->curl_handle instanceof \CurlHandle) { + // CURL handles have been changed from resources into CurlHandle + // objects starting with PHP 8.0, which don;t need to be closed. + return; + } + + $this->assertIsClosedResource($this->curl_handle); } public function testBadIP() { @@ -186,36 +192,4 @@ public function testSetsEmptyExpectHeaderIfBodySmallerThan1Mb() { $this->assertFalse(isset($result['headers']['Expect'])); } - - /** - * Assert that a provided cURL handle was properly closed. - * - * For PHP 8.0+, the cURL handle is not a resource that needs to be closed, - * but rather a \CurlHandle object. The assertion just skips these. - * - * @param resource|\CurlHandle $handle CURL handle to check. - */ - private function assertCurlHandleIsClosed($handle) { - if ($handle instanceof \CurlHandle) { - // CURL handles have been changed from resources into CurlHandle - // objects starting with PHP 8.0, which don;t need to be closed. - return; - } - - self::assertThat( - in_array( - gettype($handle), - array('resource', 'resource (closed)'), - true - ), - self::isTrue(), - 'Failed asserting that stored cURL handle was a resource.' - ); - - self::assertThat( - is_resource($handle) === false, - self::isTrue(), - 'Failed asserting that stored cURL handle was properly closed.' - ); - } } From 846ea8f3af09a7b26db24371925b78ec4aa2f2d3 Mon Sep 17 00:00:00 2001 From: Alain Schlesser Date: Wed, 10 Nov 2021 17:52:36 +0100 Subject: [PATCH 04/10] Use try-finally clause --- src/Transport/Curl.php | 16 +++++++++------- tests/Transport/Curl/CurlTest.php | 2 +- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/Transport/Curl.php b/src/Transport/Curl.php index 6238565f5..0e35d8760 100644 --- a/src/Transport/Curl.php +++ b/src/Transport/Curl.php @@ -232,15 +232,17 @@ public function request($url, $headers = [], $data = [], $options = []) { $response = $this->response_data; } - if (! isset($options['blocking']) || $options['blocking'] !== false) { - // Need to remove the $this reference from the curl handle. - // Otherwise \WpOrg\Requests\Transport\Curl won't be garbage collected and the curl_close() will never be called. - curl_setopt($this->handle, CURLOPT_HEADERFUNCTION, null); - curl_setopt($this->handle, CURLOPT_WRITEFUNCTION, null); + try { + $this->process_response($response, $options); + } finally { + if (!isset($options['blocking']) || $options['blocking'] !== false) { + // Need to remove the $this reference from the curl handle. + // Otherwise \WpOrg\Requests\Transport\Curl won't be garbage collected and the curl_close() will never be called. + curl_setopt($this->handle, CURLOPT_HEADERFUNCTION, null); + curl_setopt($this->handle, CURLOPT_WRITEFUNCTION, null); + } } - $this->process_response($response, $options); - return $this->headers; } diff --git a/tests/Transport/Curl/CurlTest.php b/tests/Transport/Curl/CurlTest.php index 165e1fcb7..3aed868cb 100644 --- a/tests/Transport/Curl/CurlTest.php +++ b/tests/Transport/Curl/CurlTest.php @@ -51,7 +51,7 @@ function ($handle) { * * This is used for asserting that cURL handles are not leaking memory. */ - protected function assert_post_conditions( ) { + protected function assert_post_conditions() { if ($this->curl_handle === null) { // No cURL handle was used during this particular test scenario. return; From 4589dba8f3f16fb0041b2fa87391d2938c529b2f Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 10 Nov 2021 17:51:30 +0100 Subject: [PATCH 05/10] Minor tweaks --- tests/Transport/Curl/CurlTest.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/Transport/Curl/CurlTest.php b/tests/Transport/Curl/CurlTest.php index 3aed868cb..7d4b0c8fa 100644 --- a/tests/Transport/Curl/CurlTest.php +++ b/tests/Transport/Curl/CurlTest.php @@ -2,6 +2,7 @@ namespace WpOrg\Requests\Tests\Transport\Curl; +use CurlHandle; use WpOrg\Requests\Exception; use WpOrg\Requests\Hooks; use WpOrg\Requests\Requests; @@ -57,13 +58,15 @@ protected function assert_post_conditions() { return; } - if ($this->curl_handle instanceof \CurlHandle) { + if ($this->curl_handle instanceof CurlHandle) { // CURL handles have been changed from resources into CurlHandle // objects starting with PHP 8.0, which don;t need to be closed. return; } - $this->assertIsClosedResource($this->curl_handle); + if ($this->shouldClosedResourceAssertionBeSkipped($this->curl_handle) === false) { + $this->assertIsClosedResource($this->curl_handle); + } } public function testBadIP() { From e67a47871e259a54382fc7118ec1feed58dc831a Mon Sep 17 00:00:00 2001 From: Alain Schlesser Date: Wed, 10 Nov 2021 18:42:10 +0100 Subject: [PATCH 06/10] Fix cURL handle locking when file stream is broken --- src/Transport/Curl.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/Transport/Curl.php b/src/Transport/Curl.php index 0e35d8760..bdf764c34 100644 --- a/src/Transport/Curl.php +++ b/src/Transport/Curl.php @@ -187,6 +187,12 @@ public function request($url, $headers = [], $data = [], $options = []) { } } + $this->hooks = $options['hooks']; + + $this->setup_handle($url, $headers, $data, $options); + + $options['hooks']->dispatch('curl.before_send', array(&$this->handle)); + $this->response_data = ''; $this->response_bytes = 0; $this->response_byte_limit = false; From 30afc43786732e3d6a4cff569faf7720004c62ae Mon Sep 17 00:00:00 2001 From: Alain Schlesser Date: Wed, 10 Nov 2021 18:42:41 +0100 Subject: [PATCH 07/10] Make blocking check more consistent with rest of the code --- src/Transport/Curl.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Transport/Curl.php b/src/Transport/Curl.php index bdf764c34..532883b4c 100644 --- a/src/Transport/Curl.php +++ b/src/Transport/Curl.php @@ -241,7 +241,7 @@ public function request($url, $headers = [], $data = [], $options = []) { try { $this->process_response($response, $options); } finally { - if (!isset($options['blocking']) || $options['blocking'] !== false) { + if (isset($options['blocking']) && $options['blocking'] === true) { // Need to remove the $this reference from the curl handle. // Otherwise \WpOrg\Requests\Transport\Curl won't be garbage collected and the curl_close() will never be called. curl_setopt($this->handle, CURLOPT_HEADERFUNCTION, null); From dd91131079c34b938828af83bdd165259e4b7daa Mon Sep 17 00:00:00 2001 From: Alain Schlesser Date: Wed, 10 Nov 2021 18:51:52 +0100 Subject: [PATCH 08/10] Fix hooks processing in cURL tests --- src/Transport/Curl.php | 2 +- tests/Transport/Curl/CurlTest.php | 11 ++++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/Transport/Curl.php b/src/Transport/Curl.php index 532883b4c..bb85345aa 100644 --- a/src/Transport/Curl.php +++ b/src/Transport/Curl.php @@ -191,7 +191,7 @@ public function request($url, $headers = [], $data = [], $options = []) { $this->setup_handle($url, $headers, $data, $options); - $options['hooks']->dispatch('curl.before_send', array(&$this->handle)); + $options['hooks']->dispatch('curl.before_send', [&$this->handle]); $this->response_data = ''; $this->response_bytes = 0; diff --git a/tests/Transport/Curl/CurlTest.php b/tests/Transport/Curl/CurlTest.php index 7d4b0c8fa..709efd866 100644 --- a/tests/Transport/Curl/CurlTest.php +++ b/tests/Transport/Curl/CurlTest.php @@ -29,21 +29,22 @@ final class CurlTest extends BaseTestCase { * @param array $other * @return array */ - protected function getOptions($other = array()) { + protected function getOptions($other = []) { $options = parent::getOptions($other); $this->curl_handle = null; - $hooks = new Hooks(); - $hooks->register( + if (!array_key_exists('hooks', $options)) { + $options['hooks'] = new Hooks(); + } + + $options['hooks']->register( 'curl.before_request', function ($handle) { $this->curl_handle = $handle; } ); - $options['hooks'] = $hooks; - return $options; } From 61b3b14180d8f551ab5e20c262102162f73644ca Mon Sep 17 00:00:00 2001 From: Alain Schlesser Date: Mon, 11 Sep 2023 10:17:48 +0200 Subject: [PATCH 09/10] Use weak references to avoid the test causing a leak --- src/Transport/Curl.php | 6 ------ tests/Transport/Curl/CurlTest.php | 22 ++++++++++++++++------ 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/src/Transport/Curl.php b/src/Transport/Curl.php index bb85345aa..5cce806c5 100644 --- a/src/Transport/Curl.php +++ b/src/Transport/Curl.php @@ -187,12 +187,6 @@ public function request($url, $headers = [], $data = [], $options = []) { } } - $this->hooks = $options['hooks']; - - $this->setup_handle($url, $headers, $data, $options); - - $options['hooks']->dispatch('curl.before_send', [&$this->handle]); - $this->response_data = ''; $this->response_bytes = 0; $this->response_byte_limit = false; diff --git a/tests/Transport/Curl/CurlTest.php b/tests/Transport/Curl/CurlTest.php index 709efd866..0f07a1db8 100644 --- a/tests/Transport/Curl/CurlTest.php +++ b/tests/Transport/Curl/CurlTest.php @@ -3,6 +3,7 @@ namespace WpOrg\Requests\Tests\Transport\Curl; use CurlHandle; +use WeakReference; use WpOrg\Requests\Exception; use WpOrg\Requests\Hooks; use WpOrg\Requests\Requests; @@ -15,7 +16,10 @@ final class CurlTest extends BaseTestCase { /** * Temporary storage of the cURL handle to assert against. * - * @var null|resource|\CurlHandle + * The handle is stored as a weak reference in order to avoid the test itself + * becoming the source of the memory leak due to locking the resource. + * + * @var null|WeakReference */ protected $curl_handle; @@ -34,6 +38,12 @@ protected function getOptions($other = []) { $this->curl_handle = null; + // On PHP < 7.2, we lack the capability to store weak references. + // In this case, we just skip the memory leak testing. + if (version_compare(PHP_VERSION, '7.2.0') < 0) { + return $options; + } + if (!array_key_exists('hooks', $options)) { $options['hooks'] = new Hooks(); } @@ -41,7 +51,7 @@ protected function getOptions($other = []) { $options['hooks']->register( 'curl.before_request', function ($handle) { - $this->curl_handle = $handle; + $this->curl_handle = WeakReference::create($handle); } ); @@ -54,19 +64,19 @@ function ($handle) { * This is used for asserting that cURL handles are not leaking memory. */ protected function assert_post_conditions() { - if ($this->curl_handle === null) { + if (version_compare(PHP_VERSION, '7.2.0') < 0 || !$this->curl_handle instanceof WeakReference) { // No cURL handle was used during this particular test scenario. return; } - if ($this->curl_handle instanceof CurlHandle) { + if ($this->curl_handle->get() instanceof CurlHandle) { // CURL handles have been changed from resources into CurlHandle // objects starting with PHP 8.0, which don;t need to be closed. return; } - if ($this->shouldClosedResourceAssertionBeSkipped($this->curl_handle) === false) { - $this->assertIsClosedResource($this->curl_handle); + if ($this->shouldClosedResourceAssertionBeSkipped($this->curl_handle->get()) === false) { + $this->assertIsClosedResource($this->curl_handle->get()); } } From e1cd380e2d18cd0eae86c00f85e41a159c206f4a Mon Sep 17 00:00:00 2001 From: Alain Schlesser Date: Mon, 11 Sep 2023 10:26:24 +0200 Subject: [PATCH 10/10] Bump use of weak references to PHP 7.4 --- tests/Transport/Curl/CurlTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Transport/Curl/CurlTest.php b/tests/Transport/Curl/CurlTest.php index 0f07a1db8..1eea7a4f7 100644 --- a/tests/Transport/Curl/CurlTest.php +++ b/tests/Transport/Curl/CurlTest.php @@ -40,7 +40,7 @@ protected function getOptions($other = []) { // On PHP < 7.2, we lack the capability to store weak references. // In this case, we just skip the memory leak testing. - if (version_compare(PHP_VERSION, '7.2.0') < 0) { + if (version_compare(PHP_VERSION, '7.4.0') < 0) { return $options; } @@ -64,7 +64,7 @@ function ($handle) { * This is used for asserting that cURL handles are not leaking memory. */ protected function assert_post_conditions() { - if (version_compare(PHP_VERSION, '7.2.0') < 0 || !$this->curl_handle instanceof WeakReference) { + if (version_compare(PHP_VERSION, '7.4.0') < 0 || !$this->curl_handle instanceof WeakReference) { // No cURL handle was used during this particular test scenario. return; }