Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect behavior in message mode when too small buffer supplied #2809

Draft
wants to merge 38 commits into
base: master
Choose a base branch
from
Draft
Changes from 1 commit
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
b53a091
[FIX] Calculate correctly max payload size per UDP packet
Mar 1, 2023
a8b9104
Applied verification of the payload size fit. Added tests. Updated do…
Mar 3, 2023
56fdfdb
Withdrawn quotes for option names
Mar 3, 2023
d7f32d0
Fixed links in the socket option table
Mar 3, 2023
70de763
Fixed minimum MSS to 116. Fixed some other bux
Mar 6, 2023
2eb1159
Replaced rand_r with std c++ random
Mar 6, 2023
9f48d3c
Fixed usage of C++14 literals in the test (build failures)
Mar 6, 2023
edbb608
[MAINT] Upgraded CI: ubuntu to version 20.04
Mar 6, 2023
26a7be6
Fixed test logics (printing after closing)
Mar 6, 2023
3d387a2
Attempted fix for a deadlock in test, added some tracking
Mar 6, 2023
03717f3
Merge branch 'dev-upgrade-ci-ubuntu' into dev-fix-ipv6-payloadsize
Mar 6, 2023
3eef592
Added expect and tracking to close socket in ReuseAddr test (Travis p…
Mar 6, 2023
e2ab5f6
Used relaxed signaling for the sake of Travis
Mar 6, 2023
d081e50
Lock debug fix for tests
ethouris Mar 7, 2023
ba5f962
Added timeout for lock-CV to avoid Travis problem
ethouris Mar 7, 2023
320a79b
Attempted more debug for test ipv6 for Travis
ethouris Mar 7, 2023
1b3ccd7
More debug for Travis
ethouris Mar 7, 2023
90b13b1
Fixed test ipv6 to use promise-future for synchronization
Mar 7, 2023
bc4059c
Fixed filtering-out IPv6 tests for Travis
Mar 7, 2023
11574a0
Fixed wrong comment
Mar 9, 2023
fabb85f
Updated with upstream and fixed
Apr 28, 2023
7cc801f
Updated and fixed
Sep 8, 2023
7658145
Fixed a bug introduced during upstream merge
Sep 11, 2023
41a86bf
Fixed tests that should require IPv6 enabled
Sep 11, 2023
45809bd
Pre-refax of change-independent parts for 2677
Sep 18, 2023
c782bf7
Merge branch 'dev-fix-ipv6-payloadsize-prefax' into dev-fix-ipv6-payl…
Sep 18, 2023
848857b
Merge branch 'master' into dev-fix-ipv6-payloadsize
Sep 18, 2023
0c3abe0
A fix from code review
Sep 18, 2023
ce2a043
Apply SOME suggestions from the doc review (others pending)
ethouris Sep 19, 2023
ccfb0b6
Updated and fixed
Sep 19, 2023
422cbed
Merge branch 'master' into dev-fix-ipv6-payloadsize
Sep 19, 2023
fa40417
Some more explanatory comments to enforce checkin
Sep 19, 2023
5c2c876
Apply suggestions from doc review (still pending)
ethouris Sep 19, 2023
0403820
Update doc review (still pending)
ethouris Sep 19, 2023
e02d85a
Apply suggestions from doc review (complete)
ethouris Sep 19, 2023
6074f21
Added extra v6-to-v6 test to check correct payload size
Oct 2, 2023
43d1a9c
Merge branch 'dev-fix-ipv6-payloadsize' into dev-fix-messageapi-error
Oct 6, 2023
9c6e5b5
Added a test that demonstrates the bug
Oct 6, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Updated and fixed
Mikołaj Małecki committed Sep 8, 2023
commit 7cc801fecb37105009c2ced1ef2f5a953d9d3f9b
1 change: 0 additions & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
@@ -71,7 +71,6 @@ matrix:
- BUILD_TYPE=Release
- BUILD_OPTS='-DENABLE_MONOTONIC_CLOCK=ON'
script:
- TESTS_IPv6="TestMuxer.IPv4_and_IPv6:TestIPv6.*6:ReuseAddr.ProtocolVersion:ReuseAddr.*6" ; # Tests to skip due to lack of IPv6 support
- if [ "$TRAVIS_COMPILER" == "x86_64-w64-mingw32-g++" ]; then
export CC="x86_64-w64-mingw32-gcc";
export CXX="x86_64-w64-mingw32-g++";
2 changes: 1 addition & 1 deletion docs/API/API-functions.md
Original file line number Diff line number Diff line change
@@ -171,7 +171,7 @@ Since SRT v1.5.0.
| [SRT_REJ_FILTER](#SRT_REJ_FILTER) | 1.3.4 | The [`SRTO_PACKETFILTER`](API-socket-options.md#SRTO_PACKETFILTER) option has been set differently on both connection parties |
| [SRT_REJ_GROUP](#SRT_REJ_GROUP) | 1.4.2 | The group type or some group settings are incompatible for both connection parties |
| [SRT_REJ_TIMEOUT](#SRT_REJ_TIMEOUT) | 1.4.2 | The connection wasn't rejected, but it timed out |
| [SRT_REJ_CRYPTO](#SRT_REJ_CRYPTO) | 1.6.0-dev | The connection was rejected due to an unsupported or mismatching encryption mode |
| [SRT_REJ_CRYPTO](#SRT_REJ_CRYPTO) | 1.5.2 | The connection was rejected due to an unsupported or mismatching encryption mode |
| [SRT_REJ_SETTINGS](#SRT_REJ_SETTINGS) | 1.6.0 | The connection was rejected because settings on both parties are in collision and cannot negotiate common values |
| <img width=290px height=1px/> | | |

134 changes: 68 additions & 66 deletions docs/API/API-socket-options.md

Large diffs are not rendered by default.

16 changes: 6 additions & 10 deletions srtcore/buffer_tools.cpp
Original file line number Diff line number Diff line change
@@ -143,7 +143,7 @@ void CRateEstimator::updateInputRate(const time_point& time, int pkts, int bytes
return;

// Required Byte/sec rate (payload + headers)
m_iInRateBytesCount += (m_iInRatePktsCount * CPacket::SRT_DATA_HDR_SIZE);
m_iInRateBytesCount += (m_iInRatePktsCount * m_iFullHeaderSize);
m_iInRateBps = (int)(((int64_t)m_iInRateBytesCount * 1000000) / period_us);
HLOGC(bslog.Debug,
log << "updateInputRate: pkts:" << m_iInRateBytesCount << " bytes:" << m_iInRatePktsCount
@@ -173,15 +173,11 @@ void CSndRateEstimator::addSample(const time_point& ts, int pkts, size_t bytes)

if (iSampleDeltaIdx >= 2 * NUM_PERIODS)
{
// Required Byte/sec rate (payload + headers)
m_iInRateBytesCount += (m_iInRatePktsCount * m_iFullHeaderSize);
m_iInRateBps = (int)(((int64_t)m_iInRateBytesCount * 1000000) / period_us);
HLOGC(bslog.Debug,
log << "updateInputRate: pkts:" << m_iInRateBytesCount << " bytes:" << m_iInRatePktsCount
<< " rate=" << (m_iInRateBps * 8) / 1000 << "kbps interval=" << period_us);
m_iInRatePktsCount = 0;
m_iInRateBytesCount = 0;
m_tsInRateStartTime = time;
// Just reset the estimator and start like if new.
for (int i = 0; i < NUM_PERIODS; ++i)
{
const int idx = incSampleIdx(m_iFirstSampleIdx, i);
m_Samples[idx].reset();

if (idx == m_iCurSampleIdx)
break;
4 changes: 2 additions & 2 deletions srtcore/buffer_tools.h
Original file line number Diff line number Diff line change
@@ -122,8 +122,8 @@ class CRateEstimator
int m_iInRatePktsCount; // number of payload packets added since InRateStartTime.
int m_iInRateBytesCount; // number of payload bytes added since InRateStartTime.
time_point m_tsInRateStartTime;
uint64_t m_InRatePeriod; // usec
int m_iInRateBps; // Input Rate in Bytes/sec
uint64_t m_InRatePeriod; // usec
int m_iInRateBps; // Input Rate in Bytes/sec
int m_iFullHeaderSize;
};

21 changes: 15 additions & 6 deletions srtcore/core.cpp
Original file line number Diff line number Diff line change
@@ -308,6 +308,11 @@ void srt::CUDT::construct()

srt::CUDT::CUDT(CUDTSocket* parent)
: m_parent(parent)
#ifdef ENABLE_MAXREXMITBW
, m_SndRexmitRate(sync::steady_clock::now())
#endif
, m_iISN(-1)
, m_iPeerISN(-1)
{
construct();

@@ -332,6 +337,11 @@ srt::CUDT::CUDT(CUDTSocket* parent)

srt::CUDT::CUDT(CUDTSocket* parent, const CUDT& ancestor)
: m_parent(parent)
#ifdef ENABLE_MAXREXMITBW
, m_SndRexmitRate(sync::steady_clock::now())
#endif
, m_iISN(-1)
, m_iPeerISN(-1)
{
construct();

@@ -5668,10 +5678,9 @@ bool srt::CUDT::prepareBuffers(CUDTException* eout)
<< (m_TransferIPVersion == AF_INET6 ? "6" : m_TransferIPVersion == AF_INET ? "4" : "???")
<< " authtag=" << authtag);

m_pSndBuffer = new CSndBuffer(m_TransferIPVersion, 32, snd_payload_size, authtag);
SRT_ASSERT(m_iISN != -1);
m_pRcvBuffer = new srt::CRcvBuffer(m_iISN, m_config.iRcvBufSize, m_pRcvQueue->m_pUnitQueue, m_config.bMessageAPI);
// after introducing lite ACK, the sndlosslist may not be cleared in time, so it requires twice space.
SRT_ASSERT(m_iPeerISN != -1);
m_pRcvBuffer = new srt::CRcvBuffer(m_iPeerISN, m_config.iRcvBufSize, m_pRcvQueue->m_pUnitQueue, m_config.bMessageAPI);
// After introducing lite ACK, the sndlosslist may not be cleared in time, so it requires twice a space.
m_pSndLossList = new CSndLossList(m_iFlowWindowSize * 2);
m_pRcvLossList = new CRcvLossList(m_config.iFlightFlagSize);
}
@@ -10117,8 +10126,8 @@ int srt::CUDT::handleSocketPacketReception(const vector<CUnit*>& incoming, bool&

const steady_clock::time_point tnow = steady_clock::now();
ScopedLock lg(m_StatsLock);
m_stats.rcvr.dropped.count(stats::BytesPackets(iDropCnt* rpkt.getLength(), iDropCnt));
m_stats.rcvr.undecrypted.count(stats::BytesPackets(rpkt.getLength(), 1));
m_stats.rcvr.dropped.count(stats::BytesPacketsCount(iDropCnt* rpkt.getLength(), iDropCnt));
m_stats.rcvr.undecrypted.count(stats::BytesPacketsCount(rpkt.getLength(), 1));
if (frequentLogAllowed(tnow))
{
LOGC(qrlog.Warn, log << CONID() << "Packet not encrypted (seqno %" << u->m_Packet.getSeqNo() << "), dropped "
2 changes: 2 additions & 0 deletions test/test_file_transmission.cpp
Original file line number Diff line number Diff line change
@@ -201,6 +201,8 @@ TEST(FileTransmission, Upload)

TEST(FileTransmission, Setup46)
{
SRTST_REQUIRES(IPv6);

using namespace srt;

srt_startup();
5 changes: 4 additions & 1 deletion test/test_ipv6.cpp
Original file line number Diff line number Diff line change
@@ -2,6 +2,9 @@
#include <chrono>
#include <string>
#include <future>
#include "gtest/gtest.h"
#include "test_env.h"

#include "srt.h"
#include "sync.h"
#include "netinet_any.h"
@@ -207,7 +210,7 @@ TEST_F(TestIPv6, v4_calls_v6_mapped)
client.join();
}

TEST_F(TestIPv6, v6_calls_mapped_v6)
TEST_F(TestIPv6, v6_calls_v6_mapped)
{
SRTST_REQUIRES(IPv6);

You are viewing a condensed version of this merge commit. You can view the full changes here.