[EnzoH8321]

Motivation

Fixes #90.

A query that fails synchronously crashes instead of throwing. Our iOS app hit this three times across two devices (iOS 26.4.2 and 26.7.1): twice as SIGSEGV in swift_release at DNSResolver_c-ares.swift:209, once as a libmalloc invalid-free abort at :210. All three had the same stack through Ares.query, and all occurred when a device lost its route to the configured resolvers.

Ares.query frees the QueryReplyHandler box when ares_query_dnsrec returns a non-success status, on the premise
stated in the comment: "On failure the callback will not fire." That premise doesn't hold — c-ares invokes the
callback and returns the error on every reachable synchronous failure path:

if (ares_slist_len(channel->servers) == 0) {
    callback(arg, ARES_ENOSERVER, 0, NULL);   // ares_send.c:123
    return ARES_ENOSERVER;                    // ares_send.c:124
}

Same shape at ares_send.c:141-142, :162-163 and :172-174. The callback's own defer
(DNSResolver_c-ares.swift:184-187) has already released the handler and deallocated the box, so the cleanup on the
error return is a second release of freed memory.

This is a regression in 0.8.0 (1bd427a, #85), which introduced the block as part of moving from the deprecated
ares_query to ares_query_dnsrec. The void-returning ares_query had no synchronous status to misinterpret.

Modifications

Removed the deinitialize/deallocate pair from the status != ARES_SUCCESS branch and corrected the comment.

handler.handle(status:dnsrec:) is deliberately kept. ares_query_dnsrec returns ARES_EFORMERR without calling the
callback when the channel is null (ares_query.c:123-125), and that is the only path where this call is what resumes
the continuation — removing it would turn the crash into a hang. On the paths that do call back it is a no-op, since
QueryReplyHandler enforces resume-once by clearing resume to nil under its lock.

Added CAresSynchronousFailureTests.

Result

A synchronous failure from ares_query_dnsrec surfaces as a thrown AsyncDNSResolver.Error instead of terminating the
process. No change to the success path or to asynchronous failures.

Test Plan

The new test reaches a synchronous failure offline, with no network, routing changes or privileges. An empty server list
is joined into an empty CSV and passed to ares_set_servers_ports_csv, which treats it as "blank all servers" and
succeeds (ares_update_servers.c:1280-1286), leaving the channel with none — so every subsequent query takes the
ares_slist_len(channel->servers) == 0 path above. It repeats 50 times, because a single over-release can land on
memory that hasn't been reused and pass unnoticed.

Without the patch, the test target aborts on the first test:

Test Case '-[AsyncDNSResolverTests.CAresSynchronousFailureTests
  test_synchronousFailure_throwsAndDoesNotDoubleFreeTheQueryHandler]' started.
error: Process '.../xctest ...' exited with unexpected signal code 6

Signal 6 is libmalloc's invalid-free check on the second deallocate. With the patch, both tests pass.

Two of the three reported crashes took the other route — SIGSEGV in swift_release — because the freed box had already
been reused, so the first deinitialize read a recycled pointer and followed it. The third was the same abort this test
produces. Which symptom appears depends only on allocator timing.

Our app worked around this by switching to the dnssd backend, so the patched c-ares path hasn't been exercised in production — the evidence for the fix itself is the test above.

[josephnoir]

Thank you for working on the fix! Two notes:

• The comments are a bit verbose and I left some suggestions to focus them more.
• Please double check that test_synchronousFailure_throwsForEveryQueryType provides additional value.

[josephnoir]

This could be a bit more straightforward. Since there is no code here to explicitly address a bug, we should write this to the explain that code that exists, not the one we removed:

ares_query_dnsrec reports a synchronous status and will invoke the callback on every reachable failure path, which frees handlerPointer. handler` resumes exactly once, so this report is a no-op on those paths."

[EnzoH8321]

Good call, updated.

[josephnoir]

Suggested change

	///
	/// c-ares invokes the query callback *and* returns the error on every reachable synchronous
	/// failure path, so the returned status is not evidence that the callback was skipped. The
	/// callback owns the handler box and frees it, which means a second free on the error return
	/// is a release of freed memory: either a `swift_release` on a recycled allocation or a
	/// `POINTER_BEING_FREED_WAS_NOT_ALLOCATED` abort, both of which kill the process rather than
	/// failing an assertion.

[EnzoH8321]

Done.

[josephnoir]

Suggested change

	/// An empty server list is the simplest way to reach a synchronous failure offline, and it
	/// needs no network, no routing changes and no privileges. `AresChannel` joins the list into
	/// a CSV and hands it to `ares_set_servers_ports_csv`, which treats an empty string as
	/// "blank all servers" and succeeds, leaving the channel with none. Every subsequent query
	/// then takes the `ares_slist_len(channel->servers) == 0` path in `ares_send.c`, which calls
	/// the callback with `ARES_ENOSERVER` and returns the same status.
	/// An empty server list is the simplest way to reach a synchronous failure offline, and it
	/// needs no network, no routing changes and no privileges.

[EnzoH8321]

Done

[josephnoir]

"exactly once" is a strong claim that these tests do not cover. There is an existing test test_cancelledQueryReleasesResourcesAndDoesNotLeak that should cover the leak, so we should focus on not freeing more than once.

[EnzoH8321]

Correct, these only cover the upper bound. I've updated to "A synchronous failure from ares_query_dnsrec must surface as a thrown error without freeing the query handler more than once."

[josephnoir]

Does the query type make a difference for the failure path? I expect that there is no reply since the server list is empty.

[EnzoH8321]

No, it makes no difference. Test ran the same code path four times, hit the "no servers configured" check before record type was even read. In this case the query type would not even matter. I'll go ahead and remove the tests.

[josephnoir]

LGTM. Thank you!