buglwIP - A Lightweight TCP/IP stack - Bugs: bug #61837, DNS Table State Deadlock On NTP...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #61837: DNS Table State Deadlock On NTP Retry

Submitted by:  HB <bigbummer>
Submitted on:  Mon 17 Jan 2022 05:58:05 AM UTC
Votes:  100  
 
Category:  DNS Severity:  3 - Normal
Item Group:  None Status:  None
Privacy:  Public Assigned to:  None
Open/Closed:  Open Planned Release:  None
lwIP version:  * 2.1.2
* Mandatory Fields

Add a New Comment (Rich Markup)
   

Comment Type & Canned Response:
       No canned response available

 

Tue 22 Feb 2022 09:27:06 AM UTC, comment #2: 

Next time, please provide a patch file instead of describing your proposed change in words. Reading a patch file is far easier.

Anyway, I see the problem:
- dns_call_found() can result in a nested call into the dns code
- the entry in question is re-requested
- we set the entry to DNS_STATE_UNUSED afterwards, effectively cancelling the re-request

Simon Goldschmidt <goldsimon>
Project Administrator
Sun 13 Feb 2022 11:24:24 PM UTC, comment #1: 

Forgot to allocate a PCB when an enqueued request is found, resulting in the dns send to fail. Updated as such:


static void
dns_check_entry(u8_t i)
{

    ...

    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);

            /* 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
            * exists, we cannot enqueue a new request, forcing the block to stay unused.
            */

            u8_t enqueued_request = dns_check_for_request(i);

            /* Flush or reset based on enqueued response */
            if (enqueued_request) {
                entry->state = DNS_STATE_NEW;
                entry->pcb_idx = dns_alloc_pcb();
            } else {
                entry->state = DNS_STATE_UNUSED;
            }

          }
        }

    ...

}

/**
 * dns_check_entries() - Checks if there is a DNS request enqueued for dns_table
 * @param i - dns_table index
 * @return 1 for request found, 0 for no request found.
 */
static u8_t
dns_check_for_request(u8_t i)
{
  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) {
      return 1;
    }
  }

  return 0;
}

HB <bigbummer>
Mon 17 Jan 2022 05:58:05 AM UTC, original submission:  

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;
}

HB <bigbummer>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by storky123 (Voted in favor of this item)
  • -email is unavailable- added by bigbummer (Submitted the item)
  •  

    There are 100 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

     

     

     

    Follows 1 latest change.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-02-21 storky123 Carbon-Copy- => Added storky123

    Back to the top


    Powered by Savane 3.9