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

Memory leak in SecureConnector #134

Closed
laszlovl opened this issue Jun 20, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@laszlovl
Copy link

commented Jun 20, 2018

I'm not sure if this issue is http-client's fault, a PHP bug, or a coding error but let's start here and see where it goes.

I use http-client to poll a REST API in a script that's constantly running. I observed that PHP's memory footprint is ever increasing up to the point where the OOM killer kicks in. After adding some memory_get_usage(true) debug statements, I noticed that the amount reported by PHP is static, even if ps/top show it's constantly increasing.

Eventually I used gdb to dump the PHP process' entire memory space, over 1GB at the time. What I found is millions of repeated SSL references, specifically snippets from the requested URL's SSL certificate.

After creating a minimal scenario for reproduction, it appears that the problem does not manifest when using HTTP instead of HTTPS urls. I tried rewriting the script to a addPeriodicTimer() setup but the problem still exists.

This is on Ubuntu 16.04 LTS, with the latest PHP (7.0.30) and http-client (0.5.9) versions.

<?php

require __DIR__ . '/vendor/autoload.php';

class Tester
{
    public function test()
    {
        $this->loop = \React\EventLoop\Factory::create();

        $reactConnector = new React\Socket\Connector($this->loop);

        $this->httpClient = new React\HttpClient\Client($this->loop, $reactConnector);

        $config = ['url' => 'https://url-on-a-local-or-nearby-host'];
        $this->makeCall($config);

        gc_disable();

        $this->loop->run();
    }

    private function makeCall($rest)
    {
        $request = $this->httpClient->request('GET', $rest['url']);

        $request->on('response', function ($response) use ($rest) {
            $response->on('data', function ($chunk) use ($rest) {
                echo "Received response " . memory_get_usage(true) / 1024 . PHP_EOL;
            });
        });

        $request->on('close', function () use ($rest) {
            $this->loop->addTimer(0.01, function () use ($rest) {
                $this->makeCall($rest);
            });

            gc_collect_cycles();
        });

        $request->end();
    }
}

$tester = new Tester;
$tester->test();
@clue

This comment has been minimized.

Copy link
Member

commented Jun 20, 2018

@laszlovl Thank you for reporting!

I've just tried to reproduce this locally, but can not confirm any memory issues.

Here's the client side script I've kept running for a few minutes:

<?php

use React\HttpClient\Client;
use React\Socket\Connector;

require __DIR__ . '/../vendor/autoload.php';

$url = isset($argv[1]) ? $argv[1] : 'https://127.0.0.1:8080/';

$loop = React\EventLoop\Factory::create();
$connector = new Connector($loop, array('tls' => array('verify_peer' => false)));
$client = new Client($loop, $connector);

$send = function () use ($client, $url, &$send){
    $request = $client->request('GET', $url);

    $request->on('error', function (\Exception $e) {
        echo $e;
    });

    $request->on('close', function () use ($send) {
        echo '.';
        $send();
    });

    $request->end();
};
$send();

$loop->addPeriodicTimer(1.0, function () {
    echo PHP_EOL . round(memory_get_usage() / 1024 / 1024, 1) . 'M ' . gc_collect_cycles();
});

$loop->run();

For the server side, I've used our own server examples from https://github.com/reactphp/http/blob/master/examples/11-hello-world-https.php and https://github.com/reactphp/http/blob/master/examples/01-hello-world.php.

I've kept this running for a few minutes and both the memory reported from within PHP and from top output seem to be rather constant at a few megabytes only.

I can confirm that updating dependencies has a very significant effect, as we've recently introduced some significant memory improvements for react/event-loop and react/promise. Can you verify that composer outdated reports you're using all the latest components?

@laszlovl

This comment has been minimized.

Copy link
Author

commented Jun 20, 2018

Well this was fun :)

For those who prefer to skip to the conclusion right away: there's a memory leak in PHP (7.0)'s validation of SAN fields in SSL certificates.

I started by testing against the example server you created and indeed, no memory leak. Back to testing with a real server though and the leak was immediately obvious. A few hours later and a few miles down the rabbit hole it clicked: connecting to an SSL certificate that only provides a commonName (the default when you create a self-signed certificate for development) everything is fine. But if the SSL certificate contains SAN entries (like pretty much every certificate in the wild) there's a leak.

Reproduction is simple. Create a new SSL certificate with two SAN entries and let your server use it:

openssl req \
    -newkey rsa:2048 \
    -x509 \
    -nodes \
    -keyout server.key \
    -new \
    -out server.crt \
    -subj /CN=127.0.0.1 \
    -reqexts SAN \
    -extensions SAN \
    -config <(cat /etc/ssl/openssl.cnf \
        <(printf '[SAN]\nsubjectAltName=DNS:127.0.0.1,DNS:127.0.0.2')) \
    -sha256 \
    -days 3650

cat server.crt server.key > localhost-san.pem

php server.php 127.0.0.1:8000 ./localhost-san.pem 

Then test with this client and the memory leak is back:

$options = [
    'ssl' => [
        'verify_peer' => false
    ]
];

while (true) {
    $response = file_get_contents('https://127.0.0.1:8000', false, stream_context_create($options));
}

Adding the verify_peer_name => false context option makes it go away again.

Alas, since the problem is in PHP itself I'll have to take this elsewhere. I'll submit a bugreport there once I can confirm the problem still exists in PHP 7.2.6. You can probably close this, thanks for pointing me in the right direction @clue :)

@clue

This comment has been minimized.

Copy link
Member

commented Jun 21, 2018

@laszlovl Thank you very much for reporting back and letting us know! I'm happy to hear both that you've been able to track this down and also that this is not a bug here :-)

If you file an upstream issue, make sure to link to it from here so we can follow along 👍

I believe this has been answered, so I'm closing this for now. Please come back with more details if this problem persists and we can reopen this 👍

@clue clue closed this Jun 21, 2018

@laszlovl

This comment has been minimized.

Copy link
Author

commented Jun 28, 2018

FYI, the problem still exists in PHP 7.2.7. I reported an issue here: https://bugs.php.net/bug.php?id=76542

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.