[lwip-devel] [bug #61837] DNS Table State Deadlock On NTP Retry

[Hamish Banham]

URL:
  <https://savannah.nongnu.org/bugs/?61837>

                 Summary: DNS Table State Deadlock On NTP Retry
                 Project: lwIP - A Lightweight TCP/IP stack
            Submitted by: bigbummer
            Submitted on: Mon 17 Jan 2022 05:58:05 AM UTC
                Category: DNS
                Severity: 3 - Normal
              Item Group: None
                  Status: None
                 Privacy: Public
             Assigned to: None
             Open/Closed: Open
         Discussion Lock: Any
         Planned Release: None
            lwIP version: 2.1.2

    _______________________________________________________

Details:

When using DNS & NTP, if the DNS server reaches max retries, NTP will enqueue
a DNS request before the state of the DNS table is updated to DNS_UNUSED. As
the DNS table reports the state is unused and a request is generated for NTP
(via dns_call_found(...)), NTP is now deadlocked.

This results in sntp_request(...) unable to create a new request as
dns_gethostbyname(...) reports the DNS state is currently ERR_INPROGRESS
(valid). However, as the state of the DNS table is now UNUSED, the request
will never get handled.

Now, I am new to LWIP, I might be missing an important step to reset DNS
caches when the DNS server is not reachable (either no PHY or server down).

/**
 * dns_check_entry() - see if entry has not yet been queried and, if so, sends
out a query.
 * Check an entry in the dns_table:
 * - send out query for new entries
 * - retry old pending entries on timeout (also with different servers)
 * - remove completed entries from the table if their TTL has expired
 *
 * @param i index of the dns_table entry to check
 */
static void
dns_check_entry(u8_t i)

err_t err;
  struct dns_table_entry *entry = &dns_table[i];

  LWIP_ASSERT("array index out of bounds", i < DNS_TABLE_SIZE);

  switch (entry->state) {

  ...

  case DNS_STATE_ASKING:
      if (--entry->tmr == 0) {
        if (++entry->retries == DNS_MAX_RETRIES) {
          if (dns_backupserver_available(entry)
#if LWIP_DNS_SUPPORT_MDNS_QUERIES
              && !entry->is_mdns
#endif /* LWIP_DNS_SUPPORT_MDNS_QUERIES */
             ) {
            /* change of server */
            entry->server_idx++;
            entry->tmr = 1;
            entry->retries = 0;
          } else {
            LWIP_DEBUGF(DNS_DEBUG, ("dns_check_entry: \"%s\": timeout\n",
entry->name));
            /* call specified callback function if provided */
            dns_call_found(i, NULL); // <- Can create a request which will
deadlock DNS table
            /* flush this entry */
            entry->state = DNS_STATE_UNUSED;
            break;
          }
        } else {
          /* wait longer for the next retry */
          entry->tmr = entry->retries;
        }
        ...
}


Proposed Fix:

LWIP_DEBUGF(DNS_DEBUG, ("dns_check_entry: \"%s\": timeout\n", entry->name));
/* call specified callback function if provided */
dns_call_found(i, NULL);

/* Check to see if new DNS request was enqueued by resulting call back
    * if so, we don't want to change the state to unused, doing so would brick
the DNS
    * entry as the state would be set as unused, but a request would exist. As
the request
    * exits, we cannot enqueue a new request, forcing the block to stay
unused.
    * */
u8_t callback_enqueued_request = 0;
for (u8_t idx = 0; idx < DNS_MAX_REQUESTS; idx++) {
    if (dns_requests[idx].found != NULL && dns_requests[idx].dns_table_idx ==
i) {
    callback_enqueued_request = 1;
    }
}

if (!callback_enqueued_request) {
    /* Flush Request */
    entry->state = DNS_STATE_UNUSED;
} else {
    /* Reset State */
    entry->state = DNS_STATE_NEW;
}





    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/bugs/?61837>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/