Skip to content

pjlib-util: forget the cached DNS errors when the nameservers' state is reset #5328

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.

Sign up for GitHub

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

Open
vlaushkin wants to merge 1 commit into
base: master
Choose a base branch
from
+104 −15
Open

pjlib-util: forget the cached DNS errors when the nameservers' state is reset #5328

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions pjlib-util/include/pjlib-util/resolver.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,8 +362,9 @@ PJ_DECL(pj_status_t) pj_dns_resolver_set_ns(pj_dns_resolver *resolver,
* Reset the state of the name servers, so that they are all tried again
* as when they were set with #pj_dns_resolver_set_ns(), e.g. after a
* network change: a name server marked as bad while the previous network
* was going down may answer on the new one. Unlike setting them again,
* this keeps the cache and the pending queries untouched.
* was going down may answer on the new one. Their cached errors, e.g. a

@sauwming sauwming Oct 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NXDOMAIN is also an error status (PJ_STATUS_FROM_DNS_RCODE) but is kept, and error entries the application added without TTL are kept as well. Could the doc say exactly what's dropped (rcodes other than NOERROR and NXDOMAIN) and that entries added without TTL stay, like the doc of pj_dns_resolver_clear_cache() does?

* refusal, are dropped too. The rest of the cache and the pending queries
* are kept.
*
* @param resolver The resolver instance.
*
Expand Down
81 changes: 76 additions & 5 deletions pjlib-util/src/pjlib-util-test/resolver_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2725,15 +2725,27 @@ static int wait_pkt_count(unsigned count)
#define SYS_FAIL(code) { rc = (code); goto on_return; }

/* After the reset, a nameserver marked as bad is tried again, while the
* cache and the pending queries are kept.
* cache and the pending queries are kept, but for the cached errors.
*/
static int dns_reset_ns_state_test(void)
{
pj_str_t name1 = pj_str("name_reset1");
pj_str_t name2 = pj_str("name_reset2");
pj_str_t name3 = pj_str("name_reset3");
static const int err_rcode[] = {
PJ_DNS_RCODE_FORMERR, PJ_DNS_RCODE_SERVFAIL,
PJ_DNS_RCODE_NOTIMPL, PJ_DNS_RCODE_NOTAUTH
};
static const char *err_name[] = {
"name_reset_err1", "name_reset_err2",
"name_reset_err3", "name_reset_err4"
};
pj_dns_async_query *err_q[PJ_ARRAY_SIZE(err_rcode) + 1] = { NULL };
unsigned i;
pj_str_t ns_addr = pj_str("127.0.0.1");
pj_uint16_t port = g_server[0].port;
pj_dns_parsed_packet *r;
pj_dns_parsed_packet *r, pkt;
pj_dns_parsed_query question;
pj_dns_resolver *res = NULL;
pj_dns_settings lset;
pj_dns_async_query *q;
Expand Down Expand Up @@ -2819,13 +2831,72 @@ static int dns_reset_ns_state_test(void)
PJ_TEST_EQ(g_server[0].pkt_count + g_server[1].pkt_count, sent, NULL,
SYS_FAIL(-918));

pj_dns_resolver_destroy(res, PJ_FALSE);
res = NULL;
/* But the cached errors are dropped, while NXDOMAIN stays */

@sauwming sauwming Oct 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing tests that an error entry added by the application without TTL (pj_dns_resolver_add_entry(..., PJ_FALSE)) survives this reset. If the 0x7FFFFFFF check in drop_cache() were moved after the errors_only branch, such an entry would be dropped and no test would fail. Could you add one?

pj_bzero(&pkt, sizeof(pkt));
pj_bzero(&question, sizeof(question));
pkt.hdr.qdcount = 1;
pkt.q = &question;
question.type = PJ_DNS_TYPE_A;
question.dnsclass = 1;
for (i = 0; i < PJ_ARRAY_SIZE(err_rcode); ++i) {
pkt.hdr.flags = PJ_DNS_SET_QR(1) | PJ_DNS_SET_RCODE(err_rcode[i]);
question.name = pj_str((char*)err_name[i]);
PJ_TEST_SUCCESS(pj_dns_resolver_add_entry(res, &pkt, PJ_TRUE), NULL,

@sauwming sauwming Oct 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs PJ_DNS_RESOLVER_INVALID_TTL to be non-zero, but config.h documents 0 as valid. With 0, add_entry() with an error rcode and set_ttl=PJ_TRUE caches nothing, start_query() returns a pending query and the test fails at -921. Could the entries be added with an explicit TTL, or the check skipped when the macro is 0?

SYS_FAIL(-919));
reset_ns_cb_called = PJ_FALSE;
PJ_TEST_SUCCESS(pj_dns_resolver_start_query(res, &question.name,
PJ_DNS_TYPE_A, 0,
&dns_callback_reset_ns,
NULL, &q),
NULL, SYS_FAIL(-920));
PJ_TEST_TRUE(q == NULL && reset_ns_cb_called, NULL, SYS_FAIL(-921));
PJ_TEST_EQ(reset_ns_cb_status, PJ_STATUS_FROM_DNS_RCODE(err_rcode[i]),
NULL, SYS_FAIL(-922));
}
pkt.hdr.flags = PJ_DNS_SET_QR(1) | PJ_DNS_SET_RCODE(PJ_DNS_RCODE_NXDOMAIN);
question.name = name3;
PJ_TEST_SUCCESS(pj_dns_resolver_add_entry(res, &pkt, PJ_TRUE), NULL,
SYS_FAIL(-923));

PJ_TEST_SUCCESS(pj_dns_resolver_reset_ns_state(res), NULL, SYS_FAIL(-924));

/* Left unanswered until cancelled at the end */
g_server[0].action = ACTION_IGNORE;
lset.qretr_delay = 10000;
pj_dns_resolver_set_settings(res, &lset);
sent = g_server[0].pkt_count + g_server[1].pkt_count;
for (i = 0; i < PJ_ARRAY_SIZE(err_q); ++i) {
pj_str_t name = i < PJ_ARRAY_SIZE(err_rcode) ?
pj_str((char*)err_name[i]) : name1;

@sauwming sauwming Oct 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name1 check doesn't test the reset at -924: name1's REFUSED entry, cached at -902, was already dropped by the reset at -906. The checks for name1 here would pass even if the reset at -924 did nothing, so the only REFUSED that was cached from a real reply isn't tied to the reset this part tests.


PJ_TEST_SUCCESS(pj_dns_resolver_start_query(res, &name,
PJ_DNS_TYPE_A, 0,
&dns_callback_reset_ns,
NULL, &err_q[i]),
NULL, SYS_FAIL(-925));
PJ_TEST_NOT_NULL(err_q[i], NULL, SYS_FAIL(-926));
}
PJ_TEST_EQ(wait_pkt_count(sent + PJ_ARRAY_SIZE(err_q)), 0, NULL,
SYS_FAIL(-927));

reset_ns_cb_called = PJ_FALSE;
PJ_TEST_SUCCESS(pj_dns_resolver_start_query(res, &name3, PJ_DNS_TYPE_A, 0,
&dns_callback_reset_ns, NULL,
&q),
NULL, SYS_FAIL(-928));
PJ_TEST_TRUE(q == NULL && reset_ns_cb_called, NULL, SYS_FAIL(-929));
PJ_TEST_EQ(reset_ns_cb_status,
PJ_STATUS_FROM_DNS_RCODE(PJ_DNS_RCODE_NXDOMAIN), NULL,
SYS_FAIL(-930));

on_return:
if (res)
if (res) {
for (i = 0; i < PJ_ARRAY_SIZE(err_q); ++i) {
if (err_q[i])
pj_dns_resolver_cancel_query(err_q[i], PJ_FALSE);
}
pj_dns_resolver_destroy(res, PJ_FALSE);
}
return rc;
}

Expand Down
33 changes: 25 additions & 8 deletions pjlib-util/src/pjlib-util/resolver.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,8 @@ static void sys_collect(pj_dns_resolver *resolver, struct query_head *list);

static void report_sys(pj_dns_resolver *resolver, pj_dns_async_query *q);

static void drop_cache(pj_dns_resolver *resolver, pj_bool_t errors_only);

static void update_res_cache(pj_dns_resolver *resolver,
const struct res_key *key,
pj_status_t status,
Expand Down Expand Up @@ -799,6 +801,9 @@ PJ_DEF(pj_status_t) pj_dns_resolver_reset_ns_state(pj_dns_resolver *resolver)
resolver->ns[i].state = STATE_ACTIVE;
resolver->ns[i].state_expiry = now;
}

/* Their errors may not hold on the new network either */
drop_cache(resolver, PJ_TRUE);

@sauwming sauwming Oct 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lookup in flight during the reset puts the old network's error back in the cache right after it's dropped: a sys-fallback getaddrinfo() that fails after the reset caches the error again via update_res_cache() (line 1529), and a late reply does the same at line 2723. This race is most likely exactly at a network change. A cheap fix: keep a reset counter in the resolver, stamp it on each query, and don't cache an error if it has changed since the query started.

if (resolver->ns_count)
PJ_LOG(4,(resolver->name.ptr, "Nameserver state reset"));

Expand Down Expand Up @@ -2820,27 +2825,28 @@ PJ_DEF(unsigned) pj_dns_resolver_get_cached_count(pj_dns_resolver *resolver)
}


/*
* Clear the response cache.
/* Remove the cached entries, or only the errors. Entries added by the
* application without TTL are kept. Called with the lock held.
*/
PJ_DEF(pj_status_t) pj_dns_resolver_clear_cache(pj_dns_resolver *resolver)
static void drop_cache(pj_dns_resolver *resolver, pj_bool_t errors_only)
{
pj_hash_iterator_t it_buf, *it;

PJ_ASSERT_RETURN(resolver, PJ_EINVAL);

pj_grp_lock_acquire(resolver->grp_lock);

it = pj_hash_first(resolver->hrescache, &it_buf);
while (it) {
struct cached_res *cache;

cache = (struct cached_res*) pj_hash_this(resolver->hrescache, it);
it = pj_hash_next(resolver->hrescache, it);

/* Keep the entries added by the application without TTL */
if (cache->expiry_time.sec == 0x7FFFFFFFL)
continue;
if (errors_only) {
int rcode = PJ_DNS_GET_RCODE(cache->pkt->hdr.flags);

if (rcode == 0 || rcode == PJ_DNS_RCODE_NXDOMAIN)

@sauwming sauwming Oct 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The system-fallback answers survive this. They're cached with rcode 0 for bad_ns_ttl, and they only exist because the nameservers weren't trusted, which is the state the reset clears. E.g. with sys_fallback on, a captive portal refuses DNS, getaddrinfo() returns the portal's address and it's cached for 60s as NOERROR. After the reset that entry is kept, so for up to bad_ns_ttl the name keeps resolving to the stale address without asking the nameservers again. Should the reset also drop the fallback entries?

@sauwming sauwming Oct 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NXDOMAIN and empty NOERROR (no-data) answers from the previous network are kept too. Captive portals and split-horizon DNS often return these, and they cause the same symptom this PR fixes: e.g. an empty or NXDOMAIN answer to a _sip._udp SRV query before login is cached for PJ_DNS_RESOLVER_INVALID_TTL (60s), so after the reset the SIP SRV lookup still falls back to the domain's address for up to a minute. A cached NXDOMAIN for A also makes AAAA lookups fail through name_is_unknown(). Is keeping them intentional?

continue;
}

pj_hash_set(NULL, resolver->hrescache, &cache->key,
sizeof(cache->key), 0, NULL);
Expand All @@ -2849,7 +2855,18 @@ PJ_DEF(pj_status_t) pj_dns_resolver_clear_cache(pj_dns_resolver *resolver)
if (--cache->ref_cnt <= 0)
free_entry(resolver, cache);
}
}


/*
* Clear the response cache.
*/
PJ_DEF(pj_status_t) pj_dns_resolver_clear_cache(pj_dns_resolver *resolver)
{
PJ_ASSERT_RETURN(resolver, PJ_EINVAL);

pj_grp_lock_acquire(resolver->grp_lock);
drop_cache(resolver, PJ_FALSE);
pj_grp_lock_release(resolver->grp_lock);
return PJ_SUCCESS;
}
Expand Down
Loading