Prevent cURL transport from leaking on Exception

[soulseekah]

trafficstars

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]

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]

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

Trying to catch is unsuccessful

[silviucpp]

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);
    }
}

[schlessera]

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]

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]

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.

[jrfnl]

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.