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

resolve option (DNS cache) not honored with persistent handles #45

Closed
afflerbach opened this issue Jun 14, 2016 · 20 comments
Closed

resolve option (DNS cache) not honored with persistent handles #45

afflerbach opened this issue Jun 14, 2016 · 20 comments

Comments

@afflerbach
Copy link

A client with persistent handle sends requests to unexpected hosts when using the pre-populated DNS cache via the resolve option:

<?php

$request = new http\Client\Request('GET', 'http://example.com/');

$client = new http\Client('curl', 'persistentID');
$client->enqueue($request);
$client->send(); // opens persistent connection to example.com

$request2 = new http\Client\Request('GET', 'http://example.com/');
$options = [ 'resolve' => ['example.com:80:127.0.0.1'] ];
$request2->setOptions($options); // remap example.com to 127.0.0.1
$client->enqueue($request2);
$client->send(); // Unexpectedly re-uses handle, issues 2nd request to example.com
print(substr($client->getResponse($request2)->getBody(), 0, 150));

I expected it to open a fresh connection when the resolve option is actually effective.

@m6w6
Copy link
Owner

m6w6 commented Jun 15, 2016

Thanks four your report!

We cannot add the IP to the persistent handle ID, because we do not know it beforehand.

We'd rather have to look how to handle issues with the DNS cache.

I'll first try to determine the intended behavior of related options within libcurl.

@filex
Copy link

filex commented Jun 15, 2016

We cannot add the IP to the persistent handle ID, because we do not know it beforehand.

Is the resolve option (or better: its mapped IP address) already known when handle ID is generated?

If so, we could seed the handle with the resolve-option's IP for a particular domain/port pair. Normally, without a resolve map (or one that doesn't match), that seed would be null. Would this work to separate the persistent handle IDs?

@m6w6
Copy link
Owner

m6w6 commented Jun 15, 2016

resolve actually works like a pre-population of the DNS cache, so if the connection already exists for host example.com, the DNS (cache) won't even be consulted.

At a first glance, everything is working as expected: https://curl.haxx.se/libcurl/c/CURLOPT_RESOLVE.html

Implementing CURLOPT_FRESH_CONNECT/CURLOPT_FORBID_REUSE might be an option.

@afflerbach
Copy link
Author

afflerbach commented Jun 17, 2016

Let me try to explain the issue in a different way: I am using the resolve option to map a hostname X to an IP address Y. Persistent connections are enabled.

What I observe seems to be the following:
A HTTP request to host X is issued. Due to the DNS cache, a TCP connection to Y is openend. The HTTP response body contains a resource from Y. The connection is kept persistent for later reuse. So far, so good.

If later another HTTP request to X is issued, and at the same time the persistent TCP connection is about to be closed (netstat -t indicates CLOSE_WAIT) it seems, that that connection is still considered to be ready for reuse.

However, a fresh connection will be opened - this time without taking the resolve option into account. Therefore, the next HTTP request will go to X and not, as expected, to Y.

Unfortunately, in this way, the resolve option is not reliably usable together with persistent handles.

@m6w6
Copy link
Owner

m6w6 commented Jun 20, 2016

I'm still not sure I understand correctly.

However, a fresh connection will be opened - this time without taking the resolve option into account. Therefore, the next HTTP request will go to X and not, as expected, to Y.

This sounds like we should check for a bug in libcurl.

Do you set ["resolve" => ["X:80:Y"]] for every request?

@filex
Copy link

filex commented Jun 20, 2016

Do you set ["resolve" => ["X:80:Y"]] for every request?

Yes, we do! However, it is complicated to reproduce :)

I use this script in PHP-FPM:

$url = 'http://example.com/';
$request = new http\Client\Request('GET', $url);
$options = [ 'resolve' => ['example.com:80:127.0.0.1'] ];
$request->setOptions($options);

$client = new http\Client('curl', 'foo');
$client->enqueue($request);
$client->send(); // opens persistent connection to localhost

print('<pre>');
print($client->getResponse($request)->getBody());
print("PID: " . getmypid() ."\n");
print("Port: " . $client->getTransferInfo($request)->local_port . "\n");

restart php-fpm to clear any connections and then call the script a couple of times. all requests go to localhost:80 where we have an apache running.

From the PID and Port output, I see that I switch between two FPM children when I reload, each of them has a persistent connection (local port not changing).

Then comes the boring part: netstat -nt |grep 127.0.0.1:80 until one of the connections changes from ESTABLISHED to CLOSE_WAIT:

$  netstat -nt |grep 127.0.0.1:80
tcp        0      0 127.0.0.1:41939         127.0.0.1:80            VERBUNDEN  
tcp        1      0 127.0.0.1:41936         127.0.0.1:80            CLOSE_WAIT 
tcp6       0      0 127.0.0.1:80            127.0.0.1:41936         FIN_WAIT2  
tcp6       0      0 127.0.0.1:80            127.0.0.1:41939         VERBUNDEN  

When I now hit reload in the browser, my request goes to the real example.com and not to localhost.

This sounds like we should check for a bug in libcurl.

Maybe :) We use the CentOS 7 system package libcurl-7.29.0-25.el7.centos.x86_64.

@m6w6
Copy link
Owner

m6w6 commented Jun 20, 2016

Oh, somehow I thought you were using libcurl >= 7.41 -- no idea where I got his from :-/

Does the CentOS 7 system package use an asynchronous resolver (c-ares or threaded; check with curl-config --features | grep AsynchDNS)? If so, IIRC I fixed cache lookups for async resolvers in libcurl-7.38.

Would you mind to check whether your issue disappears with a libcurl >= 7.38?

Thank you!

@m6w6
Copy link
Owner

m6w6 commented Jun 20, 2016

I'm still unable to reproduce this with libcurl built from tag curl-7_29_0. Neither with system, threaded nor c-ares resolver.

Restarting the target webserver, shuts down the socket on the server side and puts it in CLOSE_WAIT state. For me, libcurl detects the dead connection and correctly creates a new one with the information found in the DNS cache.

@filex
Copy link

filex commented Jun 20, 2016

Does the CentOS 7 system package use an asynchronous resolver (c-ares or threaded; check with curl-config --features | grep AsynchDNS)?

yes, it does.

If so, IIRC I fixed cache lookups for async resolvers in libcurl-7.38.

Would you mind to check whether your issue disappears with a libcurl >= 7.38?

I can reproduce the error with 7.29.0 (the base for the centos 7.2 libcurl) with this configuration

./configure --enable-threaded-resolver --prefix=/tmp/curl-test-7.29.0

and LD_PRELOAD'ing the .so from /tmp/curl-test-7.29.0/lib/ before restarting my php-fpm.

I cannot reproduce it with curl >= 7.35.

@filex
Copy link

filex commented Jun 20, 2016

Restarting the target webserver, shuts down the socket on the server side and puts it in CLOSE_WAIT state. For me, libcurl detects the dead connection and correctly creates a new one with the information found in the DNS cache.

At first, I have experimented with restarting the webserver, too. But that doesn't seem to fail reliably.

After establishing the connection to localhost I have to wait for about a minute before the "server" part of the connection disappears from netstat. Then, the re-connect goes to example.com (reliably).

I have not yet figured out which timeout is responsible. Lowering tcp_fin_timeout didn't make my day less boring :)

@m6w6
Copy link
Owner

m6w6 commented Jun 20, 2016

Okay, I've been able to reproduce it with 7.29 and setting dns_cache_timeout to 1 second.

So, it looks like an old libcurl bug to me where those entries were expiring, while they shouldn't.

I couldn't find an explicit change log entry about such an issue, but, ugh, there've been more than 20 releases since.

@filex
Copy link

filex commented Jun 20, 2016

Okay, I've been able to reproduce it with 7.29 and setting dns_cache_timeout to 1 second.

you're debugging 60 times faster than me :)

So, it looks like an old libcurl bug to me where those entries were expiring, while they shouldn't.

does that mean, that any resolve-option will be useless after the first dns_cache_timeout has passed, until the fpm-child restarts?

I couldn't find an explicit change log entry about such an issue, but, ugh, there've been more than 20 releases since.

hm. do you know any workaround that would avoid this issue with 7.29/centos7?

@m6w6
Copy link
Owner

m6w6 commented Jun 20, 2016

That's a hard one.

Maybe a dns_cache_timeout higher than the maximum time of a php-fpm process cycle?

A better solution might be to run and control your own DNS forwarder/cache to be used by libcurl https://mdref.m6w6.name/http/Client/Curl#$dns_servers.

@m6w6
Copy link
Owner

m6w6 commented Jun 20, 2016

Gosh, that's only available with the c-ares resolver (unless you want the whole host to use the custom DNS cache/forwarder).

@m6w6 m6w6 changed the title Persistent cURL handle ID must include IP address from DNS cache resolve option (DNS cache) not honored with persistent handles Jun 21, 2016
@afflerbach
Copy link
Author

Maybe a dns_cache_timeout higher than the maximum time of a php-fpm process cycle?

We'll try that one.

somehow I thought you were using libcurl >= 7.41 -- no idea where I got his from :-/

Might be because of @rcanavans comment in #37. We support multiple distributions and therefore link against different versions of libcurl. 7.29 currently is the oldest.

@filex
Copy link

filex commented Jun 23, 2016

Does the $dns_cache_timeout option only affect the TTL of the DNS Cache entries that are created by the particular request with this options? Therefore, can I use a high TTL for all requests with resolve option, and no $dns_cache_timeout for all other requests to have the default TTL there?

@m6w6
Copy link
Owner

m6w6 commented Jun 23, 2016

Quite a good question, actually.

My wild guess is "no, you can't". All requests (curl easy handles) share the DNS cache of the client (curl multi handle), so I think each request with different options affects the shared DNS cache.

@filex
Copy link

filex commented Jun 30, 2016

As you suggested, our workaround is now setting the TTL to a day on requests that use pre-resolved DNS. Thus, the entries stay long enough in curl's DNS cache to be still available when a stale persistent connection is reconnected. (Problem solved).

In our setup a domain is either always or never pre-resolved. So we shouldn't have problems with conflicting DNS entries. As our FPM processes have a life span that is usually much short than a day, I don't expect side effects of the long TTL. (Hopefully: No new problem opened).

I'd say, let's close this as "documented" :)

Thanks a lot for your help!

@m6w6
Copy link
Owner

m6w6 commented Jul 11, 2016

Alright, re-open at will! Thanks.

@m6w6 m6w6 closed this as completed Jul 11, 2016
m6w6 added a commit that referenced this issue Oct 4, 2016
+ Added http\Client\Curl\User interface for userland event loops
+ Added http\Url::IGNORE_ERRORS, http\Url::SILENT_ERRORS and
http\Url::STDFLAGS
+ Added http\Client::setDebug(callable $debug)
+ Added http\Client\Curl\FEATURES constants and namespace
+ Added http\Client\Curl\VERSIONS constants and namespace
+ Added share_cookies and share_ssl (libcurl >= 7.23.0) options to
http\Client::configure()
+ http\Client uses curl_share handles to properly share cookies and
SSL/TLS sessions between requests
+ Improved configure checks for default CA bundles
+ Improved negotiation precision
* Fixed regression introduced by http\Params::PARSE_RFC5987: negotiation
using the params parser would receive param keys without the trailing
asterisk, stripped by http\Params::PARSE_RFC5987.
* Fix gh-issue #50: http\Client::dequeue() within
http\Client::setDebug() causes segfault (Mike, Maik Wagner)
* Fix gh-issue #47: http\Url: Null pointer deref in sanitize_value()
(Mike, @rc0r)
* Fix gh-issue #45: HTTP/2 response message parsing broken with libcurl
>= 7.49.1 (Mike)
* Fix gh-issue #43: Joining query with empty original variable in query
(Mike, Sander Backus)
* Fix gh-issue #42: fatal error when using punycode in URLs (Mike,
Sebastian Thielen)
* Fix gh-issue #41: Use curl_version_info_data.features when
initializing options (Mike)
* Fix gh-issue #40: determinde the SSL backend used by curl at runtime
(Mike, @rcanavan)
* Fix gh-issue #39: Notice: http\Client::enqueue(): Could not set option
proxy_service_name (Mike, @rcanavan)
* Fix gh-issue #38: Persistent curl handles: error code not properly
reset (Mike, @afflerbach)
* Fix gh-issue #36: Unexpected cookies sent if persistent_handle_id is
used (Mike, @rcanavan, @afflerbach)
* Fix gh-issue #34: allow setting multiple headers with the same name
(Mike, @rcanavan)
* Fix gh-issue #33: allow setting prodyhost request option to NULL
(Mike, @rcanavan)
* Fix gh-issue #31: add/improve configure checks for default CA
bundle/path (Mike, @rcanavan)

Changes from beta1:
* Fixed PHP-5.3 compatibility
* Fixed recursive calls to the event loop dispatcher

Changes from beta2:
* Fix bug #73055: crash in http\QueryString (Mike, @rc0r)
(CVE-2016-7398)
* Fix bug #73185: Buffer overflow in HTTP parse_hostinfo() (Mike, @rc0r)
* Fix HTTP/2 version parser for older libcurl versions (Mike)
m6w6 added a commit that referenced this issue Oct 4, 2016
+ Added http\Client\Curl\User interface for userland event loops
+ Added http\Url::IGNORE_ERRORS, http\Url::SILENT_ERRORS and
http\Url::STDFLAGS
+ Added http\Client::setDebug(callable $debug)
+ Added http\Client\Curl\FEATURES constants and namespace
+ Added http\Client\Curl\VERSIONS constants and namespace
+ Added share_cookies and share_ssl (libcurl >= 7.23.0) options to
http\Client::configure()
+ http\Client uses curl_share handles to properly share cookies and
SSL/TLS sessions between requests
+ Improved configure checks for default CA bundles
+ Improved negotiation precision
* Fixed regression introduced by http\Params::PARSE_RFC5987: negotiation
using the params parser would receive param keys without the trailing
asterisk, stripped by http\Params::PARSE_RFC5987.
* Fix gh-issue #50: http\Client::dequeue() within
http\Client::setDebug() causes segfault (Mike, Maik Wagner)
* Fix gh-issue #47: http\Url: Null pointer deref in sanitize_value()
(Mike, @rc0r)
* Fix gh-issue #45: HTTP/2 response message parsing broken with libcurl
>= 7.49.1 (Mike)
* Fix gh-issue #43: Joining query with empty original variable in query
(Mike, Sander Backus)
* Fix gh-issue #42: fatal error when using punycode in URLs (Mike,
Sebastian Thielen)
* Fix gh-issue #41: Use curl_version_info_data.features when
initializing options (Mike)
* Fix gh-issue #40: determinde the SSL backend used by curl at runtime
(Mike, @rcanavan)
* Fix gh-issue #39: Notice: http\Client::enqueue(): Could not set option
proxy_service_name (Mike, @rcanavan)
* Fix gh-issue #38: Persistent curl handles: error code not properly
reset (Mike, @afflerbach)
* Fix gh-issue #36: Unexpected cookies sent if persistent_handle_id is
used (Mike, @rcanavan, @afflerbach)
* Fix gh-issue #34: allow setting multiple headers with the same name
(Mike, @rcanavan)
* Fix gh-issue #33: allow setting prodyhost request option to NULL
(Mike, @rcanavan)
* Fix gh-issue #31: add/improve configure checks for default CA
bundle/path (Mike, @rcanavan)

Changes from beta1:
* Fixed recursive calls to the event loop dispatcher

Changes from beta2:
+ Improved configure checks for IDNA libraries (added
--with-http-libicu-dir, --with-http-libidnkit{,2}-dir,
--with-http-libidn2-dir)
* Fix bug #73055: crash in http\QueryString (Mike, @rc0r)
(CVE-2016-7398)
* Fix bug #73185: Buffer overflow in HTTP parse_hostinfo() (Mike, @rc0r)
* Fix HTTP/2 version parser for older libcurl versions (Mike)
* Fix gh-issue #52: Underscores in host names: libidn Failed to parse
IDN (Mike, @canavan)
m6w6 added a commit that referenced this issue Dec 12, 2016
+ Added http\Client\Curl\User interface for userland event loops
+ Added http\Url::IGNORE_ERRORS, http\Url::SILENT_ERRORS and
  http\Url::STDFLAGS
+ Added http\Client::setDebug(callable $debug)
+ Added http\Client\Curl\FEATURES constants and namespace
+ Added http\Client\Curl\VERSIONS constants and namespace
+ Added share_cookies and share_ssl (libcurl >= 7.23.0) options to
  http\Client::configure()
+ http\Client uses curl_share handles to properly share cookies and
  SSL/TLS sessions between requests
+ Improved configure checks for default CA bundles
+ Improved negotiation precision
* Fixed regression introduced by http\Params::PARSE_RFC5987: negotiation
  using the params parser would receive param keys without the trailing
  asterisk, stripped by http\Params::PARSE_RFC5987.
* Fix gh-issue #50: http\Client::dequeue() within
  http\Client::setDebug() causes segfault (Mike, Maik Wagner)
* Fix gh-issue #47: http\Url: Null pointer deref in sanitize_value()
  (Mike, @rc0r)
* Fix gh-issue #45: HTTP/2 response message parsing broken with
  libcurl >= 7.49.1 (Mike)
* Fix gh-issue #43: Joining query with empty original variable in query
  (Mike, Sander Backus)
* Fix gh-issue #42: fatal error when using punycode in URLs
  (Mike, Sebastian Thielen)
* Fix gh-issue #41: Use curl_version_info_data.features when
  initializing options (Mike)
* Fix gh-issue #40: determinde the SSL backend used by curl at runtime
  (Mike, @rcanavan)
* Fix gh-issue #39: Notice: http\Client::enqueue(): Could not set option
  proxy_service_name (Mike, @rcanavan)
* Fix gh-issue #38: Persistent curl handles: error code not properly
  reset (Mike, @afflerbach)
* Fix gh-issue #36: Unexpected cookies sent if persistent_handle_id is
  used (Mike, @rcanavan, @afflerbach)
* Fix gh-issue #34: allow setting multiple headers with the same name
  (Mike, @rcanavan)
* Fix gh-issue #33: allow setting prodyhost request option to NULL
  (Mike, @rcanavan)
* Fix gh-issue #31: add/improve configure checks for default
  CA bundle/path (Mike, @rcanavan)

Changes from beta1:
* Fixed PHP-5.3 compatibility
* Fixed recursive calls to the event loop dispatcher

Changes from beta2:
* Fix bug #73055: crash in http\QueryString (Mike, @rc0r)
  (CVE-2016-7398)
* Fix bug #73185: Buffer overflow in HTTP parse_hostinfo() (Mike, @rc0r)
  (CVE-2016-7961)
* Fix HTTP/2 version parser for older libcurl versions (Mike)
m6w6 added a commit that referenced this issue Dec 12, 2016
+ Added http\Client\Curl\User interface for userland event loops
+ Added http\Url::IGNORE_ERRORS, http\Url::SILENT_ERRORS and
  http\Url::STDFLAGS
+ Added http\Client::setDebug(callable $debug)
+ Added http\Client\Curl\FEATURES constants and namespace
+ Added http\Client\Curl\VERSIONS constants and namespace
+ Added share_cookies and share_ssl (libcurl >= 7.23.0) options to
  http\Client::configure()
+ http\Client uses curl_share handles to properly share cookies and
  SSL/TLS sessions between requests
+ Improved configure checks for default CA bundles
+ Improved negotiation precision
* Fixed regression introduced by http\Params::PARSE_RFC5987: negotiation
  using the params parser would receive param keys without the trailing
  asterisk, stripped by http\Params::PARSE_RFC5987.
* Fix gh-issue #50: http\Client::dequeue() within
  http\Client::setDebug() causes segfault (Mike, Maik Wagner)
* Fix gh-issue #47: http\Url: Null pointer deref in sanitize_value()
  (Mike, @rc0r)
* Fix gh-issue #45: HTTP/2 response message parsing broken with
  libcurl >= 7.49.1 (Mike)
* Fix gh-issue #43: Joining query with empty original variable in query
  (Mike, Sander Backus)
* Fix gh-issue #42: fatal error when using punycode in URLs
  (Mike, Sebastian Thielen)
* Fix gh-issue #41: Use curl_version_info_data.features when
  initializing options (Mike)
* Fix gh-issue #40: determinde the SSL backend used by curl at runtime
  (Mike, @rcanavan)
* Fix gh-issue #39: Notice: http\Client::enqueue(): Could not set option
  proxy_service_name (Mike, @rcanavan)
* Fix gh-issue #38: Persistent curl handles: error code not properly
  reset (Mike, @afflerbach)
* Fix gh-issue #36: Unexpected cookies sent if persistent_handle_id is
  used (Mike, @rcanavan, @afflerbach)
* Fix gh-issue #34: allow setting multiple headers with the same name
  (Mike, @rcanavan)
* Fix gh-issue #33: allow setting prodyhost request option to NULL
  (Mike, @rcanavan)
* Fix gh-issue #31: add/improve configure checks for default
  CA bundle/path (Mike, @rcanavan)

Changes from beta1:
* Fixed recursive calls to the event loop dispatcher

Changes from beta2:
+ Improved configure checks for IDNA libraries (added
  --with-http-libicu-dir, --with-http-libidnkit{,2}-dir,
  --with-http-libidn2-dir)
* Fix bug #73055: crash in http\QueryString (Mike, @rc0r)
  (CVE-2016-7398)
* Fix bug #73185: Buffer overflow in HTTP parse_hostinfo() (Mike, @rc0r)
  (CVE-2016-7961)
* Fix HTTP/2 version parser for older libcurl versions (Mike)
* Fix gh-issue #52: Underscores in host names: libidn Failed to parse
  IDN (Mike, @canavan)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants