Windows DNS resolution: Curl crash when GetAddrInfoExW callback invoked on shutdown about curl HOT 7 CLOSED

It looks to me like Curl_resolver_kill is supposed to wait for threads to terminate and doesn't do that for the GetAddrInfoExW threads.

I am not sure how quick exit works in this circumstance. If we don't wait for thread cleanup in resolver_kill then our getaddrinfo thread or the winsock getaddrinfo thread (ie the GetAddrInfoExW async dns thread) can call back into winsock, even after the user calls curl_global_cleanup?

from curl.

@pps83 any ideas?

from curl.

The best way for me to fix is to reproduce the error. @Ch40zz is it easy for you to provide some minimal example to repro the issue?

Overall, it was hard to understand how threading/asynch/dns logic worked in the curl, as some things were quite confusing. Basically, I wrote all that code making a logical assumption that winapi creates these threads. Then, for all places where libcurl manages the async data I needed to add equivalent code for winapi.

So, overall, thread_hnd access needs corresponding/equivalent access to complete_ev. Either I didn't do something right, or, libcurl had a bug dealing with thread_hnd and the bug got exposed when equivalent winapi code was used.

Here's the commit that was merged: a6bbc87
I did that code quite a wile ago, I'll just try to analyze the diff.

If you look for all the places complete_ev is accessed you'll see that it's touched at the sample places where thread_hnd gets handled:

• line 549 Curl_thread_destroy(thread_hnd); internally simply calls CloseHandle(thread_hnd); - I added CloseHandle(complete_ev);
• line 558 Curl_thread_join(thread_hnd) waits AND closes thread handler (this part is confusing). Equivalent code is WaitForSingleObject(complete_ev) and CloseHandle(complete_ev); Unlike regular threading code, async dns api has an option to cancel the request via Curl_GetAddrInfoExCancel(w8.cancel_ev) which is called before "closing the thread" part.
• line 707 similar to line 558 above, it simply waits and closes complete_ev.

However, as @jay pointed out, it looks like I had to add code to handle complete_ev case where thread_hnd was touched to make sure thread_wait_resolv was called inside Curl_resolver_kill. This leads me to this PR: #13517

from curl.

Sorry for the lengthy iterative "debugging". I don't know this code, and I didn't touch curl since that PR was merged.

Perhaps, all the quick_exit part is irrelevant for the winapi DNS request, as the cancel request supposedly should cancel the outstanding DNS request without waiting.

@bagder before merging the PR I think a call to cancel the request needs to be added, as it's not logically clear what needs to be added there. What do you think? Here's the alternative impl that ignores quick_exit and cancels the request before waiting: #13518

@Ch40zz can you try these changes with your code to see if the issue gets fixed?

from curl.

#13518 sadly did not fix the issue yet, I posted more info on the PR.
Needs a bit more investigation, we'll try to provide a repro asap.

from curl.

Quick update from us after testing for a day:
PR #13518 is correct and fixes a bug in curl. Our initial bug is still occuring when building with ASAN, however it is not a curl issue anymore. It also occurs when pasting the msdn sample code for GetAddrInfoExW(). We suspect that either ASAN has a bug (false positive) or the API itself is buggy and causes a heap-use-after-free. We will report the bug to Microsoft and hope it gets fixed soon.
So long people building with ASAN will run into the error on shutdown. We will link the bug in the main issue when it has been created.
Thanks for your help!

EDIT: repro with either curl or pure WinAPI generating the exact same stack trace when compiled with ASAN and MSVC can be found here: https://gist.github.com/Ch40zz/f3a33139f35fd608d71db5e4085e0bee

EDIT2: Created a bug on MS Developer Community: https://developercommunity.visualstudio.com/t/ASAN:-heap-use-after-free-in-NdrFullPoin/10654169

from curl.

Thanks. We'll consider the remaining issue a Windows bug until proven otherwise.

from curl.