[BUG] Crash in srt::CUDT::craftKmResponse from a null pointer in m_pCryptoControl

[leio]

Describe the bug
Crash in srt::CUDT::craftKmResponse from a null pointer in m_pCryptoControl when under some atypical situations

To Reproduce
Very rare crash case, can't be sure how exactly it happened. However the machine where this can be gdb debugged from full core file can remain running for a week to do more debugging in, if needed.

Expected behavior
No crashes

Desktop (please provide the following information):

• OS: Fedora 35
• SRT Version / commit ID: 1.4.4 from Fedora package

Additional context

Backtrace:

#0  CCryptoControl::getKmMsg_size(unsigned long) const (ki=0, this=0x0) at /usr/src/debug/srt-1.4.4-1.fc35.aarch64/srtcore/crypto.h:137
        msgsize = <optimized out>
        hs_flags = 7
#1  srt::CUDT::craftKmResponse(unsigned int*, unsigned long&) (this=0xffff9ba03420, aw_kmdata=0xffffa6fddea0, w_kmdatasize=@0xffffa6fdde40: 26) at /usr/src/debug/srt-1.4.4-1.fc35.aarch64/srtcore/core.cpp:3915
        msgsize = <optimized out>
        hs_flags = 7
#2  0x0000ffffaee30d04 in srt::CUDT::processConnectRequest(sockaddr_any const&, srt::CPacket&) (this=0x1e76280, addr=..., packet=...) at /usr/src/debug/srt-1.4.4-1.fc35.aarch64/srtcore/core.cpp:10638
        kmdata = 
          {4, 0, 927805440, 4129124264, 2801655504, 65535, 45082, 0, 4987808, 0, 927805440, 4129124264, 2801655520, 65535, 2932267136, 65535, 2801655712, 65535, 2932270692, 65535, 2550136864, 65535, 2801661360, 65535, 24, 0}
        kmdatasize = 26
        conn = CONN_ACCEPT
        error = 6
        acpu = 0xffff9ba03420
        result = 0
        exp_len = 48
        hs = 
          {static m_iContentSize = 48, static HS_EXT_HSREQ = 1, static HS_EXT_KMREQ = 2, static HS_EXT_CONFIG = 4, m_iVersion = 5, m_iType = 7, m_iISN = 157897213, m_iMSS = 1500, m_iFlightFlagSize = 8192, m_iReqType = URQ_CONCLUSION, m_iID = 377572497, m_iCookie = 1999797760, m_piPeerIP = {1646402732, 0, 0, 0}, m_extension = true}
        cookie_val = <optimized out>
        id = 182198432
        accepted_hs = <optimized out>
#3  0x0000ffffaee4c520 in srt::CRcvQueue::worker_ProcessConnectionRequest(srt::CUnit*, sockaddr_any const&) (this=this@entry=0x1e4b800, unit=0x1e7ccf8, addr=...)
    at /usr/src/debug/srt-1.4.4-1.fc35.aarch64/srtcore/queue.cpp:1421
        cg = {m_mutex = @0x1e4b868}
        listener_ret = 0
        have_listener = false
#4  0x0000ffffaee4cb30 in srt::CRcvQueue::worker(void*) (param=0x1e4b800) at /usr/src/debug/srt-1.4.4-1.fc35.aarch64/srtcore/queue.cpp:1255
        have_received = false
        rst = RST_OK
        curtime_minus_syn = {m_timestamp = <optimized out>}
        ul = <optimized out>
        self = 0x1e4b800
        sa = 
          {{sin = {sin_family = 2, sin_port = 62858, sin_addr = {s_addr = 1646402732}, sin_zero = "\000\000\000\000\000\000\000"}, sin6 = {sin6_family = 2, sin6_port = 62858, sin6_flowinfo = 1646402732, sin6_addr = {__in6_u = {__u6_addr8 = '\000' <repeats 15 times>, __u6_addr16 = {0, 0, 0, 0, 0, 0, 0, 0}, __u6_addr32 = {0, 0, 0, 0}}}, sin6_scope_id = 0}, sa = {sa_family = 2, sa_data = "\212\365\254\034\"b\000\000\000\000\000\000\000"}}, len = 16}
        id = 0
        unit = 0x1e7ccf8
        cst = CONN_CONTINUE
#5  0x0000ffffaec5dfb4 in start_thread (arg=<optimized out>) at pthread_create.c:435
        ret = <optimized out>
        pd = <optimized out>
        unwind_buf = 
              {cancel_jmp_buf = {{jmp_buf = {281473483402784, 281474078689184, 281474078689182, 8447632, 281474078689183, 0, 281473474953216, 281473621367728, 281473474953216, 8447632, 281473483400560, 16132077304594628293, 0, 16132077304464066169, 0, 0, 0, 0, 0, 0, 0, 0}, mask_was_saved = 0}}, priv = {pad = {0x0, 0x0, 0x0, 0x0}, data = {prev = 0x0, cleanup = 0x0, canceltype = 0}}}
        not_first_call = 0
#6  0x0000ffffaecc8f5c in thread_start () at ../sysdeps/unix/sysv/linux/aarch64/clone.S:79

Initial extra gdb debugging:

(gdb) frame 1
#1  srt::CUDT::craftKmResponse (this=0xffff9ba03420, aw_kmdata=0xffffa6fddea0, w_kmdatasize=@0xffffa6fdde40: 26) at /usr/src/debug/srt-1.4.4-1.fc35.aarch64/srtcore/core.cpp:3915
3915	        size_t msgsize = m_pCryptoControl->getKmMsg_size(0);
(gdb) print m_pCryptoControl
$1 = std::unique_ptr<CCryptoControl> = {get() = 0x0}

[maxsharabayko]

Hi @leio. Thank for reporting!

There seem to be a small breach on the listener side processing HS Conclusion Response described below.

// Listener gets incoming conclusion request v5 handshake.
int srt::CUDT::processConnectRequest(..) {
    // (...)
    CUDT* acpu = NULL;
    int result = uglobal().newConnection(.., acpu); 
    // If an associated socket already exists (seems to be the case according to the backtrace)
    // returns result = 0 and the existing socket in `acpu`. But `acpu` can be in the Closed state,
    // meaning m_pCryptoControl is already NULL.
    
    // Generate HS response (repeated response on successful connection or rejection reason).
    // m_pCryptoControl can be NULL, but not checked inside the function.
    acpu->craftKmResponse((kmdata), (kmdatasize));

    // (..)
}

Places to Fix

• In CUDT::craftKmResponse(..) check if m_pCryptoControl is not NULL, otherwise return CONN_REJECT.
• If the corresponding connection already exists, the socket is only checked to be broken. But it can be as well closed, having m_pCryptoControl = NULL. (see the code sample below)

int srt::CUDTUnited::newConnection(.., CUDT*& w_acpu)
{
   w_acpu = NULL;
   CUDTSocket* ls = locateSocket(listen);
   // (..)

   // Check if this connection already exists.
   if ((ns = locatePeer(peer, w_hs.m_iID, w_hs.m_iISN)) != NULL)
   {
      if (ns->core().m_bBroken) // TOFIX: Also check if ns has already been closed!
      {
         // (..)
      }
      else
      {
         // (..)
         w_acpu = &ns->core();
         return 0;
      }
   }
   // (...)
}