Crashes in DoH requests (consistent, unknown cause)

[arvids-kokins-bidstack]

OS: Windows (probably doesn't matter though)
libcurl version: 7.65.2-DEV (based on 40259ca with minor changes)

call stack:

• Curl_strncasecompare(const char * first, const char * second, unsigned __int64 max) Line 139
  • while(*first && *second && max) { -- first is an invalid value
• Curl_checkheaders(const connectdata * conn, const char * thisheader) Line 101
  • data->set.headers head seems to point to wrong memory (dangling pointer?) but often the memory is still usable enough to crash a bit deeper
  • conn->ip_addr_str = "1.1.1.1"
  • conn->connection_id = 18 (from one of the dumps, some are higher) so it's not a guaranteed failure
• Curl_http(connectdata * conn, bool * done) Line 2082
  • path = "/dns-query"
  • host = "1.1.1.1"
• multi_do(Curl_easy *) Line 1214
• multi_runsingle(Curl_multi * multi, curltime now, Curl_easy * data) Line 1585
• curl_multi_perform(Curl_multi * multi, int * running_handles) Line 2094

sadly the minidump does not include the entire connectdata or Curl_easy data structures (they seem to be too big) but feel free to ask for any data early in the structure, I may be able to provide some

I've also noticed one possibly related issue that is marked as a known bug: #4592

2 questions:

• is there a path in the state machine which would restart/reuse the DoH request after it calls doh_done?
  • I've tried using a proxy (set using env.var.) but ran into some other bugs which were fixed according to changelog
  • also tried redirects but those didn't seem to help either
• why are DoH request headers freed in doh_done separately, before freeing the request handles? this seems to be true even in the latest version: https://github.com/curl/curl/blob/master/lib/doh.c#L200

this seems to be a relatively rare issue but I'd still like to at least understand what's causing it since it's difficult to reproduce

[jay]

• is there a path in the state machine which would restart/reuse the DoH request after it calls doh_done

AFAICT no. If you are threading please read thread safety. Can you make a minimal self-contained program that can be used to reproduce?

[arvids-kokins-bidstack]

If you are threading please read thread safety.

Yes, we're threading and I have read that page.

• You can pass the handles around among threads, but you must never use a single handle from more than one thread at any given time. ✔️
• curl_global_* is used but only in the very beginning/end
• not using curl_share_*

One thread can make requests (configuring the easy handle and passing it off to another thread).
Another thread can perform them using the multi interface, then detaching them, reading the result and passing them back.
Handle and all associated data are passed between threads together.
Also, all the handles are reused and there's a limit on max. connections for the multi interface but it's not being hit in this case.

That said, with threading issues I'd have expected the crash to be more random, but it's always here (Curl_checkheaders(conn, "User-Agent")).

Can you make a minimal self-contained program that can be used to reproduce?

Unfortunately not at the moment as I haven't seen the crash in person, we only receive reports about them.

We're also not yet in a position to tell if it's something computer-specific or is simply rare (might get that data in a week or two) but it does seem to happen far more frequently with corporate network setups.

Are there any settings other than proxy which might be controlled by the environment/network instead of the code that uses libcurl?
In the past we've had issues with configurations where 1.1.1.1 was intercepted by the company's internal network, for example, but we didn't look too much into those at the time since they figured it out quickly.

[bagder]

We've fixed several issues in DoH (and elsewhere) since 40259ca. It seems a worthwhile investment to first bump up the version to the latest libcurl before continuing the search.

is there a path in the state machine which would restart/reuse the DoH request after it calls doh_done?

No. The easy handles used for DoH requests are never reused and those transfers are never restarted.

why are DoH request headers freed in doh_done separately, before freeing the request handles

Why not? When there's no more pending DoH request they're not needed anymore. If you think you have a better way to do it, so by all means, please provide a PR! We can always improve the code.

[arvids-kokins-bidstack]

It seems a worthwhile investment to first bump up the version to the latest libcurl before continuing the search.

I'd like that too but we're not in control of the release/update schedules. We'll try to update on our end sometime soon but it may take months to see the results.

Why not? When there's no more pending DoH request they're not needed anymore.

Sure, it just seems easier to reason about the code if all the resources were freed together, seeing as they were allocated and otherwise used together. Currently it looks like the headers are freed from one state of the last request out of two to finish and the handles are freed from another state of another request, and to someone like me in this situation it's not immediately obvious whether this works in all cases.

If you think you have a better way to do it, so by all means, please provide a PR! We can always improve the code.

Thanks, I might look into that once there's time to update.

[bagder]

I would urge you to work on creating a way to force the problem to trigger, probably with a stand-alone separate app that maybe mimics what your real-life application does. Then you can make that with the latest libcurl and once you manage to show the crash, you can share the code with us.

A reported crash that nobody can reproduce with a libcurl version with several DoH fixes missing is hard to act on.

[arvids-kokins-bidstack]

Sorry, yeah, I know it's hard without the data and I don't expect much here. Just wanted to see if you had any ideas on what could potentially be the issue (or if it was already fixed), and to ask about the headers.

I would urge you to work on creating a way to force the problem to trigger, probably with a stand-alone separate app that maybe mimics what your real-life application does.

I've been doing that and will continue, but do you have any suggestions on e.g. fuzzers and their configurations that could be used for this purpose?
Since the header freeing theory has not given any results, the next best theory is that there's a rare double free happening on the same address (though it's unclear why it only happens on some devices), but I don't think there's a practical possibility for me to guess which branches must be taken in what order to trigger the issue, hence approaching this analytically.

[bagder]

do you have any suggestions on e.g. fuzzers and their configurations that could be used for this purpose?

I think using valgrind and memory/address sanitizers are the most useful tools for this. Fuzzing this area of the code more would be awesome too and was done to trigger #4592 if I remember correctly, but I don't have anything prepared or setup to reproduce that other than what is already mentioned in that issue.

[jay]

In general we've occasionally seen weird arbitrary crashes in libcurl when users don't do thread synchronization properly. Though your crash has some consistency I would still double check that. Some kind of synchronization primitive is needed (eg critical section). volatile doesn't cut it and is too often misused.

[arvids-kokins-bidstack]

Thanks, will try fuzzing once I have time to boot into Linux.

I would still double check that.

Yep, doing that regularly.

Some kind of synchronization primitive is needed (eg critical section). volatile doesn't cut it and is too often misused.

It's a good thing that we don't use volatile for multithreading then. 😄 Mutexes/condition variables are used to transfer request objects.

---

I've started looking into a new theory (non-safe free > allocating headers in the same spot > dangling pointer freed by other code > crash), dumping all the frees that later turn into headers.
I figure the frees have to be quite close to guarantee this degree of consistency, and at least one of the frees should be always present (though possibly a free(null) usually). Also, after stress-testing with 100'000+ requests, while it's no guarantee for anything, I'm starting to think that it's more likely to be an edge case than a threading/ordering/timing issue.

P.S. Things I've found so far:

• de_cleanup being called twice on error (already fixed in master, though does not behave the same way as my crash anyway)
• https://github.com/curl/curl/blob/master/lib/url.c#L1906 - possible use-after-free?

[jay]

• https://github.com/curl/curl/blob/master/lib/url.c#L1906 - possible use-after-free?

#6613 thanks