From 215926a9b5486dc284f4f62a746b1f1107010564 Mon Sep 17 00:00:00 2001 From: Maxim Sharabayko Date: Wed, 2 Sep 2020 10:34:01 +0200 Subject: [PATCH] [core] Fixed cookie contest --- docs/features/handshake.md | 21 +++++++++++++++- srtcore/core.cpp | 50 ++++++++++++++++++++++++-------------- 2 files changed, 52 insertions(+), 19 deletions(-) diff --git a/docs/features/handshake.md b/docs/features/handshake.md index ef3fcfdbe..05af2cd28 100644 --- a/docs/features/handshake.md +++ b/docs/features/handshake.md @@ -697,7 +697,26 @@ connection will not be made until new, unique cookies are generated (after a delay of up to one minute). In the case of an application "connecting to itself", the cookies will always be identical, and so the connection will never be made. -When one party's cookie value is greater than its peer's, it wins the cookie +```c++ +// Here m_ConnReq.m_iCookie is a local cookie value sent in connection request to the peer. +// m_ConnRes.m_iCookie is a cookie value sent by the peer in its connection request. +const int64_t contest = int64_t(m_ConnReq.m_iCookie) - int64_t(m_ConnRes.m_iCookie); + +if ((contest & 0xFFFFFFFF) == 0) +{ + return HSD_DRAW; +} + +if (contest & 0x80000000) +{ + return HSD_RESPONDER; +} + +return HSD_INITIATOR; +``` + +When one party's cookie value is greater than its peer's (based on 32-bit subtraction of both +with potential overflow), it wins the cookie contest and becomes Initiator (the other party becomes the Responder). At this point there are two "handshake flows" possible (at least theoretically): diff --git a/srtcore/core.cpp b/srtcore/core.cpp index cefb716c5..d66ef5e87 100644 --- a/srtcore/core.cpp +++ b/srtcore/core.cpp @@ -3758,8 +3758,10 @@ void CUDT::cookieContest() if (m_SrtHsSide != HSD_DRAW) return; - HLOGC(cnlog.Debug, log << "cookieContest: agent=" << m_ConnReq.m_iCookie << " peer=" << m_ConnRes.m_iCookie); + LOGC(cnlog.Error, log << "cookieContest: agent=" << m_ConnReq.m_iCookie << " peer=" << m_ConnRes.m_iCookie); + // Here m_ConnReq.m_iCookie is a local cookie value sent in connection request to the peer. + // m_ConnRes.m_iCookie is a cookie value sent by the peer in its connection request. if (m_ConnReq.m_iCookie == 0 || m_ConnRes.m_iCookie == 0) { // Note that it's virtually impossible that Agent's cookie is not ready, this @@ -3772,31 +3774,43 @@ void CUDT::cookieContest() // // The cookie contest must be repeated every time because it // may change the state at some point. - int better_cookie = m_ConnReq.m_iCookie - m_ConnRes.m_iCookie; - - if (better_cookie > 0) - { - m_SrtHsSide = HSD_INITIATOR; + // + // In SRT v1.4.3 and prior the below subtraction was performed in 32-bit arithmetic. + // The result of subtraction can overflow 32-bits. + // Example + // m_ConnReq.m_iCookie = -1480577720; + // m_ConnRes.m_iCookie = 811599203; + // int64_t llBetterCookie = -1480577720 - 811599203 = -2292176923 (FFFF FFFF 7760 27E5); + // int32_t iBetterCookie = 2002790373 (7760 27E5); + // + // Now 64-bit arithmetic is used to calculate the actual result of subtraction. + // The 31-st bit is then checked to check if the resulting is negative in 32-bit aritmetics. + // This way the old contest behavior is preserved, and potential compiler optimisations are avoided. + const int64_t contest = int64_t(m_ConnReq.m_iCookie) - int64_t(m_ConnRes.m_iCookie); + + if ((contest & 0xFFFFFFFF) == 0) + { + // DRAW! The only way to continue would be to force the + // cookies to be regenerated and to start over. But it's + // not worth a shot - this is an extremely rare case. + // This can simply do reject so that it can be started again. + + // Pretend then that the cookie contest wasn't done so that + // it's done again. Cookies are baked every time anew, however + // the successful initial contest remains valid no matter how + // cookies will change. + + m_SrtHsSide = HSD_DRAW; return; } - if (better_cookie < 0) + if (contest & 0x80000000) { m_SrtHsSide = HSD_RESPONDER; return; } - // DRAW! The only way to continue would be to force the - // cookies to be regenerated and to start over. But it's - // not worth a shot - this is an extremely rare case. - // This can simply do reject so that it can be started again. - - // Pretend then that the cookie contest wasn't done so that - // it's done again. Cookies are baked every time anew, however - // the successful initial contest remains valid no matter how - // cookies will change. - - m_SrtHsSide = HSD_DRAW; + m_SrtHsSide = HSD_INITIATOR; } // This function should complete the data for KMX needed for an out-of-band