[core] Fixed cookie contest

[maxsharabayko]

The following condition in CUDT::cookieContest() can overflow the 32-bit signed integer:

int better_cookie = m_ConnReq.m_iCookie - m_ConnRes.m_iCookie;

The value of better_cookie is then checked against zero. A positive value is resolved to INITIATOR, a negative value is resolved to RESPONDER. Otherwise, DRAW and try again.

However, checking whether better_cookie has a negative or a positive value is not very reliable, as some compilers may optimize it just by comparing m_ConnReq.m_iCookie and m_ConnRes.m_iCookie instead.

The example input provided below results in HSD_DRAW when SRT is linked with -O3 and C++14 by clang.
A simplified example provided in this comment provides different results when built under Release or Debug configuration.

Probably further checks are optimized by the compiler somehow?

Example Cookie Contest

In this example, the actual result of a cookie contest should be INITIATOR, but due to casting to int32 the actual result is RESPONDER. It is the most likely result of the cookie contest, however, due to compiler optimizations, other results are also possible. See a simplified example provided in this comment.

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); // Used by SRT

if (iBetterCookie > 0)
{
    g_hs_side = HSD_INITIATOR;
    return;
}

if (iBetterCookie < 0)
{
    g_hs_side = HSD_RESPONDER;
    return;
}

g_hs_side = HSD_DRAW;

See also: #1267

Proposed Change

Instead of relying on integer overflow, do 64-bit arithmetic and check the 31-st bit of the result.

const int64_t contest = int64_t(REQ) - int64_t(RSP);

if ((contest & 0xFFFFFFFF) == 0)
    return HSD_DRAW;

if (contest & 0x80000000) // was (contest > 0)
    return HSD_RESPONDER;

return HSD_INITIATOR;

Both 32-bit cast and checking the 31-st bit provide the same results:

REQ	RSP	REQ - RSP	INT32 (REQ-RSP)	31st bit
-1480577720	811599203	-2292176923 (0xffffffff776027e5)	2002790373 (0x776027e5)	0
811599203	-1480577720	2292176923 (0x889fd81b)	-2002790373 (0x889fd81b)	1
2147483647	-2147483648	4294967295 (0xffffffff)	-1 (0xffffffff)	1
-2147483648	2147483647	-4294967295 (0xffffffff00000001)	1 (0x1)	0

TODO:

• Provide reliable cookie contest version (without integer overflow, backward compatible)
• Update handshake.md
• Write unit tests

[maxsharabayko]

Reproduction Example

To find the proper backward-compatible way to fix the cookie contest the minimum reproducible example given below was created.

Tests were performed using:

• Comiler: Apple clang version 11.0.3 (clang-1103.0.32.62)
• Target: x86_64-apple-darwin19.6.0

Note: This example input provided below results in HSD_DRAW when SRT is linked with -O3 and C++14 by clang.
This sample returns a different value, but still shows that the behavior is probably undefined.

Debug Build Sets HSD_INITIATOR

$ cmake . -DCMAKE_BUILD_TYPE=Debug
$ make
$ ./contest -1480577720 811599203
CookieReq = -1480577720 CookieRsp = 811599203 better_cookie = 2002790373
HS Side = 1

Release Build Sets HSD_RESPONDER

$ cmake . -DCMAKE_BUILD_TYPE=Release
$ make
$ ./contest -1480577720 811599203
CookieReq = -1480577720 CookieRsp = 811599203 better_cookie = 2002790373
HS Side = 2

Sample

contest.cpp:

#include <cstdint>
#include <iostream>
#include <string>

using namespace std;

enum HandshakeSide
{
    HSD_DRAW,
    HSD_INITIATOR,
    HSD_RESPONDER
};

HandshakeSide g_hs_side = HSD_DRAW;

void cookieContest(int32_t cookie_req, int32_t cookie_rsp)
{
    if (g_hs_side != HSD_DRAW) return;

    if (cookie_req == 0 || cookie_rsp == 0)
    {
        g_hs_side = HSD_DRAW;
        return;
    }

    const int better_cookie = cookie_req - cookie_rsp;
    // Actually removing this log line results in a similar result (1: HSD_INITIATOR)
    cout << "CookieReq = " << cookie_req << " CookieRsp = " << cookie_rsp
         << " better_cookie = " << better_cookie << '\n';

    if (better_cookie > 0)
    {
        g_hs_side = HSD_INITIATOR;
        return;
    }

    if (better_cookie < 0)
    {
        g_hs_side = HSD_RESPONDER;
        return;
    }

    g_hs_side = HSD_DRAW;
}

int main(int argc, char* argv[])
{
    if (argc != 3)
        return 1;

    const int32_t cookie_req = stoi(argv[1]);
    const int32_t cookie_rsp = stoi(argv[2]);
    cookieContest(cookie_req, cookie_rsp);
    cout << "HS Side = " << g_hs_side << '\n';

    return 0;
}

CMakeLists.txt:

cmake_minimum_required(VERSION 3.10 FATAL_ERROR)
project(contest C CXX)
message(STATUS "BUILD TYPE: ${CMAKE_BUILD_TYPE}")
add_executable(contest ./contest.cpp)

[maxsharabayko]

Changing the condition in release build.

Solution 1. Let it overflow

const long long better_cookie = cookie_req - cookie_rsp;

CookieReq = -1480577720 CookieRsp = 811599203 better_cookie = 2002790373
HS Side = 1

Result: at least compatible with debug build of the current master version.

Solution 2. Cast to int64_t

const long long better_cookie = static_cast<int64_t>(cookie_req) - static_cast<int64_t>(cookie_rsp);

CookieReq = -1480577720 CookieRsp = 811599203 better_cookie = -2292176923
HS Side = 2

Result: not compatible with the debug build of the current master version.

[maxsharabayko]

Looks like PR #76 introduced the cookieContest() function with HSv5, so this behavior is at least from v1.3.0.

[maxsharabayko]

Testing Environment:

• Comiler: Apple clang version 11.0.3 (clang-1103.0.32.62)
• Target: x86_64-apple-darwin19.6.0
• Release build

Result: two initiators.

srt-xtransmit (release build)

./bin/srt-xtransmit generate "srt://127.0.0.1:4201?mode=rendezvous&bind=:4200" --sendrate 1Mbps --duration 5s -v
16:56:17.394768 [I] SOCKET::SRT srt://127.0.0.1:4201: bound to ':4200'.
16:56:17.395312 [D] SOCKET::SRT 0x2A35723 ASYNC Connecting to srt://127.0.0.1:4201
16:56:17.395734/T0x70000ca44000*E:SRT.cn: COOKIE CONTEST:::
16:56:17.395773/T0x70000ca44000!W:SRT.cn: cookieContest: Entering agent=811599203 peer=-1480577720
16:56:17.395780/T0x70000ca44000!W:SRT.cn: cookieContest: INITIATOR agent=811599203 peer=-1480577720
16:56:17.396824/T0x70000ca44000*E:SRT.cn: COOKIE CONTEST:::
16:56:17.396837/T0x70000ca44000!W:SRT.cn: cookieContest: Entering agent=811599203 peer=-1480577720
16:56:17.407857 [D] SOCKET::SRT 0x2A35723 ASYNC Connected to srt://127.0.0.1:4201

srt-live-transmit (release build from srt master)

./srt-live-transmit "srt://127.0.0.1:4200?port=4201&mode=rendezvous" udp://127.0.0.1:4501 -v -loglevel warn
Media path: 'srt://127.0.0.1:4200?port=4201&mode=rendezvous' --> 'udp://127.0.0.1:4501'
SRT parameters specified:

	mode = 'rendezvous'
	port = '4201'
Opening SRT source rendezvous on 127.0.0.1:4200
Binding a server on :4201
Connecting to 127.0.0.1:4200
16:56:17.396153/T0x70000f11a000*E:SRT.cn: cookieContest: agent req = -1480577720 peer rsp = 811599203 better_cookie = 2002790373
16:56:17.396452/T0x70000f11a000*E:SRT.cn: cookieContest: result INITIATOR
SRT source connected

Network Capture

two-initiators.pcapng.zip

[maxsharabayko]

Testing Environment:

• Comiler: VC++ 2019
• Target: x86_64 Windows 10
• Release build

Result: connection established (responder + initiator).

srt-xtransmit (release build)

srt-live-transmit "srt://127.0.0.1:4200?port=4201&mode=rendezvous" udp://127.0.0.1:4501 -v -loglevel warn
Media path: 'srt://127.0.0.1:4200?port=4201&mode=rendezvous' --> 'udp://127.0.0.1:4501'
SRT parameters specified:

        mode = 'rendezvous'
        port = '4201'
Opening SRT source rendezvous on 127.0.0.1:4200
Binding a server on :4201
Connecting to 127.0.0.1:4200
15:55:22.462000/T15856*E:SRT.cn: cookieContest: agent req = -1480577720 peer rsp = 811599203 better_cookie = 2002790373
15:55:22.462000/T15856*E:SRT.cn: cookieContest: result INITIATOR

srt-live-transmit (release build from srt-xtransmit)

srt-xtransmit receive "srt://127.0.0.1:4201?mode=rendezvous&bind=:4200" -v
15:55:22.456760 [I] SOCKET::SRT srt://127.0.0.1:4201: bound to ':4200'.
15:55:22.458131 [D] SOCKET::SRT 0x17C02F8 ASYNC Connecting to srt://127.0.0.1:4201
15:55:22.459000/T14700*E:SRT.cn: cookieContest: agent req = 811599203 peer rsp = -1480577720 better_cookie = -2002790373
15:55:22.460000/T14700*E:SRT.cn: cookieContest: result RESPONDER
15:55:22.470132 [D] SOCKET::SRT 0x17C02F8 ASYNC Connected to srt://127.0.0.1:4201

[maxsharabayko]

Testing Environment

• Compiler: gcc 7.5.0
• Build Type: Release

Result: connection established (responder + initiator).

[maxsharabayko]

Proposed cookie contest with regard to the existing bug-drivven behavior:

const int64_t better_cookie =
    (static_cast<int64_t>(m_ConnReq.m_iCookie)
    - static_cast<int64_t>(m_ConnRes.m_iCookie))
    & 0xFFFFFFFF;


if (better_cookie > 0)
{
    m_SrtHsSide = HSD_INITIATOR;
    return;
}

if (better_cookie < 0)
{
    m_SrtHsSide = HSD_RESPONDER;
    return;
}

m_SrtHsSide = HSD_DRAW;

[maxsharabayko]

The proposed cookie contest fix is to check the 31-st bit of the resulting int64_t difference of request and response cookies instead of relying on overflowing of a signed 32-bit integer:

const int64_t contest = int64_t(REQ) - int64_t(RSP);

if ((contest & 0xFFFFFFFF) == 0)
    return HSD_DRAW;

if (contest & 0x80000000) // was (contest > 0)
    return HSD_RESPONDER;

return HSD_INITIATOR;

REQ	RSP	REQ - RSP	INT32 (REQ-RSP)	31st bit
-1480577720	811599203	-2292176923 (0xffffffff776027e5)	2002790373 (0x776027e5)	0
811599203	-1480577720	2292176923 (0x889fd81b)	-2002790373 (0x889fd81b)	1
2147483647	-2147483648	4294967295 (0xffffffff)	-1 (0xffffffff)	1
-2147483648	2147483647	-4294967295 (0xffffffff00000001)	1 (0x1)	0