aarond10 / https_dns_proxy

A lightweight DNS-over-HTTPS proxy.
MIT License
775 stars 114 forks source link

The workaround for the libcurl bug maybe too much #95

Closed nirui closed 3 years ago

nirui commented 3 years ago

When a HTTP query sent by https_dns_proxy has timed out or closed due to unexpected network errors (which is actually very usual), https_dns_proxy will enter a special mode where it outputs error message:

[E] 1608912134.350407 https_client.c:293 libcurl bug detected: socket closed without ever being read. [E] 1608912134.350471 https_client.c:294 Activating workaround. PERFORMANCE WILL BE GREATLY DEGRADED!

And then eats up all the CPU resource without releasing them in a reasonable time, causing ... well, performance degradation.

There is how to reproduce the problem:

  1. Block traffics in and out of your localhost
  2. Start https_dns_proxy on your localhost and send DNS query to it

Demo (Watch for the CPU usage):

https://user-images.githubusercontent.com/30656734/103138667-d1826f80-470f-11eb-8266-bf0216821b50.mp4

This maybe the cause of https://github.com/openwrt/packages/issues/11410

Merry Christmas BTW :)

nirui commented 3 years ago

So I've dug this a little bit deeper, and discovered that the problem is caused by the loop calling of curl_multi_perform function.

The function keeps returning error code 4 (seems to indicate a CURLM_INTERNAL_ERROR), while the value of c->still_running remain unchanged (3 in my case). So the loop keep executing indefinitely or at least for a very long time.

This was my libcurl installation for the test:

$ curl --version curl 7.68.0 (x86_64-pc-linux-gnu) libcurl/7.68.0 OpenSSL/1.1.1f zlib/1.2.11 brotli/1.0.7 libidn2/2.2.0 libpsl/0.21.0 (+libidn2/2.2.0) libssh/0.9.3/openssl/zlib nghttp2/1.40.0 librtmp/2.3 Release-Date: 2020-01-08

aarond10 commented 3 years ago

Ah, awesome! I've been "holding it wrong" this whole time.

For MULTI_SOCKET "curl_multi_socket_action is then used instead of curl_multi_perform."

The workaround for the socket disconnect bug uses both to ill effect it seems. :/ I'm going to remove the workaround as 7.20 is 10 years old now and this seems the cleanest way forward.

aarond10 commented 3 years ago

Done in 37511cc0.

stangri commented 3 years ago

@aarond10 is the -x option still supported then? PS. Is the polling interval in seconds?

aarond10 commented 3 years ago

Yeah, this doesn't change HTTP_1.1 support but it may not actually be necessary as a mitigation. The libcurl bug fix was actually just buggy. I've been running without it for 24 hours now and tried both good and bad server addresses without issue.

On Mon, 18 Jan 2021 at 12:28, Stan Grishin notifications@github.com wrote:

@aarond10 https://github.com/aarond10 is the -x option still supported then?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/aarond10/https_dns_proxy/issues/95#issuecomment-761923926, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABTOXWYPTNVEADWIZY6CXLS2OFDVANCNFSM4VJHEJ6Q .