Skip to content

[HttpClient] Segfault with http2 server using push #37252

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.

Sign up for GitHub

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

Jump to bottom
Closed
mpesari opened this issue Jun 12, 2020 · 3 comments
Closed

[HttpClient] Segfault with http2 server using push #37252

mpesari opened this issue Jun 12, 2020 · 3 comments
Labels
Bug HttpClient Status: Needs Review

Comments

@mpesari
Copy link
Contributor

mpesari commented Jun 12, 2020

Symfony version(s) affected: 5.1.1

Description

When using chaining HttpClient::create()->request(...) on a server which does a http2 push, some kind of memory corruption occurs. This does not occur when calling request() on an instantiated client: $client = HttpClient::create(); $client->request(...)

How to reproduce

Some tests and a Dockerfile for running them are available in my repo: https://github.com/mpesari/php-http2-push-issue

First, run composer require symfony/http-client. Then, execute this file:

<?php
require_once __DIR__.'/vendor/autoload.php';
use Symfony\Component\HttpClient\HttpClient;

$http2PushUrl = 'https://http2.golang.org/serverpush';
$response = HttpClient::create()->request('GET', $http2PushUrl);

print strlen($response->getContent()) . PHP_EOL;

Output:

PHP Warning:  Invalid callback , no array or string given in /root/vendor/symfony/http-client/Response/CurlResponse.php on line 264
PHP Warning:  curl_multi_exec(): Cannot call the CURLMOPT_PUSHFUNCTION in /root/vendor/symfony/http-client/Response/CurlResponse.php on line 264
PHP Warning:  Invalid callback , no array or string given in /root/vendor/symfony/http-client/Response/CurlResponse.php on line 264
PHP Warning:  curl_multi_exec(): Cannot call the CURLMOPT_PUSHFUNCTION in /root/vendor/symfony/http-client/Response/CurlResponse.php on line 264
PHP Warning:  Invalid callback , no array or string given in /root/vendor/symfony/http-client/Response/CurlResponse.php on line 264
PHP Warning:  curl_multi_exec(): Cannot call the CURLMOPT_PUSHFUNCTION in /root/vendor/symfony/http-client/Response/CurlResponse.php on line 264
PHP Warning:  Invalid callback , no array or string given in /root/vendor/symfony/http-client/Response/CurlResponse.php on line 264
PHP Warning:  curl_multi_exec(): Cannot call the CURLMOPT_PUSHFUNCTION in /root/vendor/symfony/http-client/Response/CurlResponse.php on line 264
67507
double free or corruption (out)
Aborted (core dumped)

Possible Solution

Not exactly a solution, but maybe a hint to resolving this... First instantiating the client prevents the corruption from happening:

<?php
require_once __DIR__.'/vendor/autoload.php';
use Symfony\Component\HttpClient\HttpClient;

$http2PushUrl = 'https://http2.golang.org/serverpush';
$client = HttpClient::create();
$response = $client->request('GET', $http2PushUrl);

print strlen($response->getContent()) . PHP_EOL;

Additional context

This seems related to https://bugs.php.net/bug.php?id=77535
@mpesari mpesari added the Bug label Jun 12, 2020
@stof
Copy link
Member

stof commented Jun 12, 2020

@nicolas-grekas could it be that the Client object is destructed too early by PHP ?

@nicolas-grekas
Copy link
Member

I think it's about having php objects (a closure) inside a resource. Refcounting has issues with that. Maybe a solution could be to also keep the closure in a private property.
@mpesari could you give it a try?

This was referenced Jun 17, 2020
Closed
Merged
@mpesari
Copy link
Contributor Author

mpesari commented Jun 17, 2020

@nicolas-grekas thanks for the suggestion! Just storing the closure didn't help, but I also refactored the CurlHttpClient::handlePush() to an instance method and it works now

nicolas-grekas added a commit that referenced this issue Jun 18, 2020
@nicolas-grekas
bug #37319 [HttpClient] Convert CurlHttpClient::handlePush() to insta…
5fb13bf 
…nce method (mpesari)

This PR was merged into the 4.4 branch.

Discussion
----------

[HttpClient] Convert CurlHttpClient::handlePush() to instance method

| Q             | A
| ------------- | ---
| Branch?       | 4.4 / 5.1
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #37252  <!-- prefix each issue number with "Fix #", if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->
<!--
Replace this notice by a short README for your feature/bugfix. This will help people
understand your PR and can be used as a start for the documentation.

Additionally (see https://symfony.com/releases):
 - Always add tests and ensure they pass.
 - Never break backward compatibility (see https://symfony.com/bc).
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too.)
 - Features and deprecations must be submitted against branch master.
-->

Commits
-------

9497972 [HttpClient] Convert CurlHttpClient::handlePush() to instance method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug HttpClient Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@nicolas-grekas @stof @xabbuh @mpesari @carsonbot