Ticket #1974 (closed defect: fixed)

Opened 5 months ago

Last modified 5 months ago

Various fixes for DNS, primarily for IPv6

Reported by: ming Owned by: bennylp
Priority: normal Milestone: release-2.6
Component: pjlib-util Version: trunk
Keywords: Cc:
Backport to 1.x milestone: Backported: no

Description (last modified by riza) (diff)

  1. There is a race condition introduced with the IPv6 DNS patch.
    Given
    *) Thread A sending a SIP request that needs DNS resolution.
    *) Thread B handling DNS query responses in pjsip_endpt_handle_events().
    The call to pjsip_resolve() happens under Thread A which sets a dummy query:
    query->object6 = (pj_dns_async_query*)0x1;
    
    and then calls pj_dns_resolver_start_query() with the dns_a_callback().
    However, the dummy query protection is undone because the first thing that pj_dns_resolver_start_query() with the dns_a_callback() does is to set query->object6 to NULL when it is called.
    Then in thread B, the dns_a_callback() sees that there isn't an IPv6 query and attempts to send the request immediately even if there may not be any A records in the response. The pending dns_aaaa_callback() can now attempt to use resources that may be destroyed.
  1. In pjsip_resolve() the pj_dns_resolver_start_query() for the AAAA records should be guarded by a status == PJ_SUCCESS check in case the A records query start fails.
  1. In both dns_a_callback() & dns_aaaa_callback():
    if (srv->count > 0)
       (*query->cb)(PJ_SUCCESS, query->token, &query->server);
    else
       (*query->cb)(query->last_error, query->token, NULL);
    
    However, query->last_error is never set, thus in sip_util.c stateless_send_resolver_callback() it will hit the assertion:
    pj_assert(tdata->dest_info.addr.count != 0);
    
    If assertion in disabled, later in stateless_send_transport_cb() the code line:
    cont = (sent > 0) ? PJ_FALSE :
    (tdata->dest_info.cur_addr<tdata->dest_info.addr.count-1);
    
    can result in an underflow.
  1. Fixed bug of premature app callback invocation for SRV resolver by applying the same method in r5369.
  1. Remove DNS cache entry from resolver's hash table when app callback has a reference.
    In resolver.c update_res_cache(), if app callback has a reference, we only decrement the reference count of the cache entry. However, since it's not removed from the hash table, later the call to pj_hash_set_np() will overwrite this old entry (instead of using the newly allocated cache->hbuf), which will later be freed.
  1. Given
    *) A previous cached A record DNS resolution that contains either no answer records or had an error returned in the query response (PJ_STATUS_FROM_DNS_RCODE() is not PJ_SUCCESS).
    *) The next attempt to send a SIP request under Thread A will cause pj_dns_resolver_start_query() to find the cached entry and immediately call the cb callback. When the callback returns pj_dns_resolver_start_query() cannot update *p_query because where it points to may no longer be valid because the memory could be freed.
    Fixed in r5477

Thank you to Richard Mudgett (Digium) for the report.

Change History

comment:1 Changed 5 months ago by ming

  • Status changed from new to closed
  • Resolution set to fixed

In 5471:

Fixed #1974: Various fixes for DNS IPv6

comment:2 Changed 5 months ago by ming

  • Description modified (diff)

comment:3 Changed 5 months ago by ming

In 5473:

Re #1974:
If there is a pending query, set the return value to that query (instead of NULL)

Thanks to Richard Mudgett for the patch.

comment:4 Changed 5 months ago by ming

  • Description modified (diff)
  • Summary changed from Various fixes for DNS IPv6 to Various fixes for DNS, primarily for IPv6

comment:5 Changed 5 months ago by ming

In 5475:

Re #1974: Remove DNS cache entry from resolver's hash table when app callback has a reference.

Thanks to Richard Mudgett for the patch.

comment:6 Changed 5 months ago by riza

In 5477:

Re #1974: Fix DNS write on freed memory.
Thanks to Richard Mudgett for the patch.

comment:7 Changed 5 months ago by riza

  • Description modified (diff)
Note: See TracTickets for help on using tickets.