[core] Fixed cookie contest #1517

maxsharabayko · 2020-09-02T08:40:39Z

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;

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 · 2020-09-02T13:42:05Z

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 · 2020-09-02T14:25:29Z

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 · 2020-09-04T09:01:54Z

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

maxsharabayko · 2020-09-07T15:06:41Z

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 · 2020-09-07T15:12:02Z

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 · 2020-09-07T15:12:46Z

Testing Environment

Compiler: gcc 7.5.0
Build Type: Release

Result: connection established (responder + initiator).

maxsharabayko · 2020-09-07T15:32:27Z

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 · 2020-09-08T11:25:13Z

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

maxsharabayko added Type: Bug Indicates an unexpected problem or unintended behavior [core] Area: Changes in SRT library core labels Sep 2, 2020

maxsharabayko added this to the v1.5.0 - Sprint 22 milestone Sep 2, 2020

maxsharabayko self-assigned this Sep 2, 2020

maxsharabayko modified the milestones: v1.5.0 - Sprint 22, v1.5.0 - Sprint 23 Sep 7, 2020

maxsharabayko force-pushed the hotfix/hscookie branch from c5621f6 to 929567d Compare September 9, 2020 12:05

maxsharabayko modified the milestones: v1.5.0 - Sprint 23, v1.5.0 Sep 21, 2020

maxsharabayko modified the milestones: v1.5.0, v1.4.3 Oct 22, 2020

mbakholdina added the Priority: Medium label Oct 22, 2020

maxsharabayko modified the milestones: v1.4.3, v1.4.4 Mar 17, 2021

maxsharabayko force-pushed the hotfix/hscookie branch from 929567d to 0b1925d Compare April 28, 2021 11:36

[core] Fixed cookie contest

215926a

maxsharabayko force-pushed the hotfix/hscookie branch from 0b1925d to 215926a Compare April 28, 2021 11:53

maxsharabayko marked this pull request as ready for review April 28, 2021 11:54

maxsharabayko merged commit d9eefd8 into Haivision:master May 4, 2021

maxsharabayko deleted the hotfix/hscookie branch May 4, 2021 10:57

maxsharabayko mentioned this pull request May 4, 2021

[BUG] Cookie contest behavior does not match documentation #1267

Closed

maxsharabayko mentioned this pull request Mar 22, 2022

Fix the cookie contest description Haivision/srt-rfc#112

Closed

maxsharabayko mentioned this pull request Jun 16, 2022

[core] Cookie contest log msg downgraded to Debug #2388

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Fixed cookie contest #1517

[core] Fixed cookie contest #1517

maxsharabayko commented Sep 2, 2020 •

edited

Loading

maxsharabayko commented Sep 2, 2020 •

edited

Loading

maxsharabayko commented Sep 2, 2020

maxsharabayko commented Sep 4, 2020

maxsharabayko commented Sep 7, 2020

maxsharabayko commented Sep 7, 2020

maxsharabayko commented Sep 7, 2020

maxsharabayko commented Sep 7, 2020

maxsharabayko commented Sep 8, 2020 •

edited

Loading

[core] Fixed cookie contest #1517

[core] Fixed cookie contest #1517

Conversation

maxsharabayko commented Sep 2, 2020 • edited Loading

Example Cookie Contest

Proposed Change

TODO:

maxsharabayko commented Sep 2, 2020 • edited Loading

Reproduction Example

Debug Build Sets HSD_INITIATOR

Release Build Sets HSD_RESPONDER

Sample

maxsharabayko commented Sep 2, 2020

Solution 1. Let it overflow

Solution 2. Cast to int64_t

maxsharabayko commented Sep 4, 2020

maxsharabayko commented Sep 7, 2020

Testing Environment:

srt-xtransmit (release build)

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

Network Capture

maxsharabayko commented Sep 7, 2020

Testing Environment:

srt-xtransmit (release build)

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

maxsharabayko commented Sep 7, 2020

Testing Environment

maxsharabayko commented Sep 7, 2020

maxsharabayko commented Sep 8, 2020 • edited Loading

maxsharabayko commented Sep 2, 2020 •

edited

Loading

maxsharabayko commented Sep 2, 2020 •

edited

Loading

maxsharabayko commented Sep 8, 2020 •

edited

Loading