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

Curl 8.12 access violation with c-ares when HTTPS-RR enabled #16216

Closed
nono303 opened this issue Feb 6, 2025 · 24 comments
Closed

Curl 8.12 access violation with c-ares when HTTPS-RR enabled #16216

nono303 opened this issue Feb 6, 2025 · 24 comments
Labels
HTTPS-RR name lookup DNS and related tech Windows Windows-specific

Comments

@nono303
Copy link

nono303 commented Feb 6, 2025

Hi,
I've just compiled and tested curl 8.12 and have an Access violation related to c-ares.

This issue was not present on 8.11.1

⏩ use case :

curl -v https://www.google.com/
and
<?php curl_exec(curl_init("https://www.google.com/")); ?>

✅ when compiling with -DENABLE_ARES=OFF: all works

🔴 when compiling with -DENABLE_ARES=ON : I have this exception

libcurl!init_resolve_thread+0x2de
libcurl!Curl_resolv+0x9a8
libcurl!resolve_server+0x121
libcurl!create_conn+0xc26
libcurl!state_connect+0x49
libcurl!multi_runsingle+0x42f
libcurl!curl_multi_perform+0x11e
libcurl!easy_perform+0x1d5

FAULTING_SOURCE_FILE:  C:\sdk\src\curl\lib\asyn-thread.c
FAULTING_SOURCE_CODE:  
   387:     struct Curl_easy *data = td->tsd.data;
   388: #endif
   389: 
   390: #ifdef USE_HTTPSRR_ARES
>  391:     ares_destroy(data->state.async.tdata->channel);
   392: #endif
   393:     /*
   394:      * if the thread is still blocking in the resolve syscall, detach it and
   395:      * let the thread do the cleanup...
   396:      */
IMAGE_NAME:  libcurl.dll
FAILURE_BUCKET_ID:  NULL_CLASS_PTR_READ_c0000005_libcurl.dll!init_resolve_thread
OS_VERSION:  10.0.26100.1
BUILDLAB_STR:  ge_release
OSPLATFORM_TYPE:  x64
IMAGE_VERSION:  8.12.0.0
...
-- Found Cares: C:/sdk/release/vs17_x64-avx2/include (found version "1.34.4")
...
-- LTO supported and enabled
-- Protocols: dict file ftp ftps gopher gophers http https imap imaps ipfs ipns ldap ldaps mqtt pop3 pop3s rtsp scp sftp smb smbs smtp smtps telnet tftp ws wss
-- Features: alt-svc asyn-rr AsynchDNS brotli HSTS HTTP2 HTTPS-proxy HTTPSRR IDN IPv6 Kerberos Largefile libz MultiSSL NTLM SPNEGO SSL SSPI threadsafe TLS-SRP Unicode UnixSockets zstd
-- Enabled SSL backends: OpenSSL v3+ Schannel

I expected the following

No response

curl/libcurl version

8.12.0

operating system

Windows 11 24h2 x64
-- Using CMake version 3.31.5
-- curl version=[8.12.0-DEV]
-- The C compiler identification is MSVC 19.42.34438.0

@vszakats vszakats added Windows Windows-specific name lookup DNS and related tech labels Feb 6, 2025
@bagder
Copy link
Member

bagder commented Feb 6, 2025

Can you figure out why your build has USE_HTTPSRR_ARES enabled?

@bagder
Copy link
Member

bagder commented Feb 6, 2025

Ah right, because you have enabled this experimental feature. It is a bug indeed. I think I have a fix for it in #16219

@bagder bagder added the HTTPS-RR label Feb 6, 2025
@nono303
Copy link
Author

nono303 commented Feb 6, 2025

Thx @bagder for your feedback!
compiling & testing 5e4ad2e don't crash anymore but I have this error : getaddrinfo() thread failed to start

Fyi:

available to so tests and qualifications ;)

@bagder
Copy link
Member

bagder commented Feb 6, 2025

Then you're running my draft pull request code. I would not recommend it.

@nono303
Copy link
Author

nono303 commented Feb 6, 2025

Then you're running my draft pull request code. I would not recommend it.

It was only for testing purpose on my use-case ;)

@nono303
Copy link
Author

nono303 commented Feb 6, 2025

@bagder
new test compiling master tag 8.12 with -DUSE_HTTPSRR=OFF -DENABLE_ARES=ON works without crash!

But found a strange regression between 8.11:

When I invoke libcurl throught php (8.4.4) and set OPTS with curl_setopt_array():

  • ✅ When i set CURLOPT_HEADER = 1 without CURLOPT_DNS_SERVERS valued ⏩ headers are well returned
  • 🔴 When i set CURLOPT_HEADER = 1 and CURLOPT_DNS_SERVERS='127.0.0.1' (whatever valued for dns)... ⏩ headers are not any more returned

EDIT digging a bit more one last one, might be related to a PHP bug (see php/php-src#17610)
But not sure as the PHP bug had been raised before 8.12...

@bagder
Copy link
Member

bagder commented Feb 6, 2025

Either way, it is a different problem so we don't deal with it in this issue.

@nono303
Copy link
Author

nono303 commented Feb 7, 2025

Hi @bagder and sorry for some confusion as an other issue on php curl_setopt_array() is mixed in my 8.12 testing & reporting...

I found an another change (regression?) might be linked to this issue but fully related to curl 8.11.1 > 8.12 upgrade
Full use-case is described here php/php-src#17610 (comment)

To summarize when having ENABLE_ARES=ON, ENABLE_THREADED_RESOLVER=ON breaks CURLOPT_DNS_SERVERS

curl_setopt failed and whatever I set for DNS IP, it's not take in account

  • ENABLE_THREADED_RESOLVER=OFF resolve the issue
  • 8.11.1 work nice with ENABLE_THREADED_RESOLVER ON & OFF

@bagder
Copy link
Member

bagder commented Feb 7, 2025

So now you want to mention a third issue here? If you think it is a (different) curl issue I really think you should create a new issue here with all the details. You can't expect us to work on PHP issues.

@nono303
Copy link
Author

nono303 commented Feb 7, 2025

To be clear & brief: 2 regressions observed fully related to curl 8.12 (compared to 8.11.1) having ENABLE_ARES=ON on Windows (through php_curl extension as wrapper of curl C API...)

  • USE_HTTPSRR=ON ⏩ Access violation
  • ENABLE_THREADED_RESOLVER=ONcurl_easy_setopt(curl, CURLOPT_DNS_SERVERS,"X.X.X.X") doesn't return CURL_OK

@bagder
Copy link
Member

bagder commented Feb 7, 2025

Please submit one issue per problem. This particular issue is about a HTTPS-RR + threaded-resolver related problem.

@bagder
Copy link
Member

bagder commented Feb 7, 2025

On the line with the ares_destroy call that causes this violation, can you see what the problem is? Is the channel there NULL or uninitialized somehow?

@bagder
Copy link
Member

bagder commented Feb 7, 2025

BTW, libcurl returns error when CURLOPT_DNS_SERVERS it set if built with the threaded resolver because the option does not work then.

@nono303
Copy link
Author

nono303 commented Feb 7, 2025

Please submit one issue per problem. This particular issue is about a HTTPS-RR + threaded-resolver related problem.

Ok ok... I think that's the case

On the line with the ares_destroy call that causes this violation, can you see what the problem is? Is the channel there NULL or uninitialized somehow?

Yes, channel is NULL

Image

BTW, libcurl returns error when CURLOPT_DNS_SERVERS it set if built with the threaded resolver because the option does not work then.

Understand but why libcurl 8.11.1 did not returns error when CURLOPT_DNS_SERVERS it set when built with the threaded resolver ?

bagder added a commit that referenced this issue Feb 7, 2025
@bagder
Copy link
Member

bagder commented Feb 7, 2025

Does #16244 mitigate the problem for you?

@nono303
Copy link
Author

nono303 commented Feb 7, 2025

Does #16244 mitigate the problem for you?

Yes as there is no more core-dump with acces violation

  • ✅ Full Working with ENABLE_THREADED_RESOLVER=OFF
  • 🔴 Still have curl: (6) getaddrinfo() thread failed to start with ENABLE_THREADED_RESOLVER=ON

@bagder
Copy link
Member

bagder commented Feb 7, 2025

Yes as there is no more core-dump with acces violation

But now you're not longer testing with the same build config so it's not really confirming that the problem is gone. You had c-ares + threaded-resolver + httpsrr enabled when it first happened.

🔴 Still have curl: (6) getaddrinfo() thread failed to start with ENABLE_THREADED_RESOLVER=ON

I think that's a deflection and a problem with your build or something. Go back to your original build that did not cause this problem and if you apply #16244, it is not a reason for the thread to suddenly fail.

@nono303
Copy link
Author

nono303 commented Feb 7, 2025

with unchanged original build ENABLE_THREADED_RESOLVER=ON + USE_HTTPSRR=ON + ENABLE_ARES=ON here are the results

@bagder
Copy link
Member

bagder commented Feb 7, 2025

Ah, now I suspect it is the ares_init that fails. Let me extend the PR...

@bagder
Copy link
Member

bagder commented Feb 7, 2025

Try the PR again now with the extra adjustment.

@nono303
Copy link
Author

nono303 commented Feb 7, 2025

better at c190db9 but have a new core dump

Get certdata with curl!
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
Failed downloading via HTTPS with curl

Image

timediff.c l.87

timediff_t curlx_tvtoms(struct timeval *tv)
{
  return (tv->tv_sec*1000) + (timediff_t)(((double)tv->tv_usec)/1000.0);
}

identifier "tv" is undefined

in call stack asyn-ares.c l.78

int Curl_ares_getsock(struct Curl_easy *data,
                      ares_channel channel,
                      curl_socket_t *socks)

identifier "channel" is undefined

in call stack asyn-thread.c l.683

Image

@bagder
Copy link
Member

bagder commented Feb 7, 2025

Right, I need to make it deal with a "broken" c-ares channel all the way. Added a commit. Many thanks for your patience and testing.

bagder added a commit that referenced this issue Feb 8, 2025
When the c-ares setup fails and we get a NULL channel, the resolve still
continues and we just need to survive it and just not get any HTTPS RR.

Reported-by: nono303 on github
Fixes #16216
@bagder bagder changed the title Curl 8.12 Access violation with c-ares [Windows] Curl 8.12 access violation with c-ares when HTTPS-RR enabled Feb 8, 2025
@nono303
Copy link
Author

nono303 commented Feb 9, 2025

Many thx @bagder!
7b38804 ✅ all is working (and all my php tests passed)

@bagder
Copy link
Member

bagder commented Feb 9, 2025

Thanks for confirming @nono303 💚

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HTTPS-RR name lookup DNS and related tech Windows Windows-specific
Development

Successfully merging a pull request may close this issue.

3 participants