Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent cURL transport from leaking on Exception #319

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

soulseekah
Copy link
Contributor

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.

@soulseekah
Copy link
Contributor Author

soulseekah commented Feb 17, 2018

Calling curl_close has been chased time (#39) and time (#59) again. Found another one while investigating #257

Requesting a bad URL, even if it's a mere certificate error, will not call the destructor and eventually curl_close. Why?

Requests_Transport_cURL::process_response can throw an Exception. This happens BEFORE the CURLOPT_HEADERFUNCTION and CURLOPT_WRITEFUNCTION are freed of the instance callbacks, so the garbage collector will not collect $this resulting in an out of memory error.

Breaking case:

while(true) {                                                                                                                                                                                  
    try {
        $response = Requests::post('http://localhost:43992', array(), array(), array('transport'=>'Requests_Transport_cURL'));
    } catch (Exception $e) {}
}

Where http://localhost:43992 is a nonexistent local endpoint (to error out faster).

@gorlovka
Copy link

gorlovka commented Jun 3, 2020

Leaks exception for me too:
http://i.kagda.ru/780166178436_03-06-2020-23:52:00_7801.png

Trying to catch is unsuccessful

@silviucpp
Copy link

There is one big problem with the fix proposed here: curl_setopt called before process_response will reset the curl_errno (used in process_response) state. And the library will trigger an exception with "Missing header/body separator" instead real cause " Connection refused".

A better fix will be:

try
{
    $this->process_response($response, $options);
}
finally
{
    if (true === $options['blocking'])
    {
        // Need to remove the $this reference from the curl handle.
        // Otherwise 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);
    }
}

@jrfnl jrfnl changed the base branch from stable to develop June 18, 2021 09:56
@schlessera
Copy link
Member

As we have now moved to PHP 5.6 as a minimum requirement, it seems to make sense to take a fresh look at the execution flow, and use the finally keyword to close the cURL handle safely no matter what.

@jrfnl
Copy link
Member

jrfnl commented Sep 12, 2021

Note for the next iteration of this PR:

For testing the correct functioning of this change, the PHPUnit 9.3+ Assert::assertIsClosedResource() assertion can be used. This assertion is polyfilled by the PHPUnit Polyfills, so can be safely used.

Also note: tests are only relevant for PHP < 8.0 and should be skipped on PHP 8.0+ as Curl was changed from using resources to using objects.

@schlessera schlessera added this to the 2.0.0 milestone Nov 5, 2021
@schlessera schlessera force-pushed the fix-curl-leak-exception branch 2 times, most recently from 9771abb to 3d91340 Compare November 10, 2021 16:28
@schlessera
Copy link
Member

Tests are failing because PHP 7 does check for active references to a resource before closing it, even if you explicitly use curl_close(). In that case, it does not actually close the resource as requested.

In our case, the very fact that we want to test whether the resource is closed properly creates a reference to the resource. So while there is no memory leak in the normal code path, there is one in the test - because of the test.

The only way I can think of for solving this is by using weak references, but those have only been introduced with PHP 7.2.

Options right now:

  • Don't test for the closed resource.
  • Use a weak reference to test for the closed resource, but skip these tests on PHP 7.0 and PHP 7.1.

@schlessera schlessera modified the milestones: 2.0.0, 2.1.0 Nov 24, 2021
soulseekah and others added 7 commits September 11, 2023 09:57
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.
@jrfnl
Copy link
Member

jrfnl commented Nov 6, 2023

Okay, so I have spend some time today trying to create a better/working test for this. Current status can be seen here: https://github.com/WordPress/Requests/tree/319/better-test-attempt-take-two

Tentative conclusions:

  • The current fix doesn't solve the problem. The Curl object instance still doesn't get destroyed.
    It may well be a step in the right direction, but it doesn't fix the issue yet.
  • There seems to be a reference somewhere to the $options array (containing a reference to the $transport class) which will need to be unset, which we haven't been able to trace back yet. This needs further investigation to figure out where the $options arrays gets stored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants