Skip to content

Commit 6a0d512

Browse files
afrindfacebook-github-bot
afrind
authored and
facebook-github-bot
committed
Fix params (#56)
Summary: Pull Request resolved: #56 There were several problems here: v11 integer params have no length includeParam was wrong for setup Reviewed By: sharmafb Differential Revision: D74500602 fbshipit-source-id: 70aa633ebac1d8694e90a584dac3d739df0e1b5a
1 parent eeb796a commit 6a0d512

File tree

2 files changed

+102
-34
lines changed

2 files changed

+102
-34
lines changed

‎moxygen/MoQFramer.cpp‎

Lines changed: 58 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ folly::Expected<std::string, ErrorCode> parseFixedString(
5555
folly::Expected<folly::Unit, ErrorCode> parseSetupParams(
5656
folly::io::Cursor& cursor,
5757
size_t& length,
58+
uint64_t version,
5859
size_t numParams,
5960
std::vector<SetupParameter>& params);
6061
bool datagramTypeHasExtensions(uint64_t version, StreamType streamType);
@@ -142,6 +143,7 @@ folly::Expected<std::string, ErrorCode> parseFixedString(
142143
folly::Expected<folly::Unit, ErrorCode> parseSetupParams(
143144
folly::io::Cursor& cursor,
144145
size_t& length,
146+
uint64_t version,
145147
size_t numParams,
146148
std::vector<SetupParameter>& params) {
147149
for (auto i = 0u; i < numParams; i++) {
@@ -158,10 +160,13 @@ folly::Expected<folly::Unit, ErrorCode> parseSetupParams(
158160
if (!res) {
159161
return folly::makeUnexpected(ErrorCode::PARSE_UNDERFLOW);
160162
}
161-
length -= res->second;
162-
res = quic::decodeQuicInteger(cursor, res->first);
163-
if (!res) {
164-
return folly::makeUnexpected(ErrorCode::PARSE_UNDERFLOW);
163+
if (getDraftMajorVersion(version) < 11) {
164+
length -= res->second;
165+
res = quic::decodeQuicInteger(cursor, res->first);
166+
if (!res) {
167+
XLOG(ERR) << "Failed to decode parameter value";
168+
return folly::makeUnexpected(ErrorCode::PARSE_UNDERFLOW);
169+
}
165170
}
166171
p.asUint64 = res->first;
167172
length -= res->second;
@@ -189,11 +194,15 @@ folly::Expected<ClientSetup, ErrorCode> parseClientSetup(
189194
return folly::makeUnexpected(ErrorCode::PARSE_UNDERFLOW);
190195
}
191196
length -= numVersions->second;
197+
bool v11Plus = true;
192198
for (auto i = 0ul; i < numVersions->first; i++) {
193199
auto version = quic::decodeQuicInteger(cursor, length);
194200
if (!version) {
195201
return folly::makeUnexpected(ErrorCode::PARSE_UNDERFLOW);
196202
}
203+
if (getDraftMajorVersion(version->first) < 11) {
204+
v11Plus = false;
205+
}
197206
clientSetup.supportedVersions.push_back(version->first);
198207
length -= version->second;
199208
}
@@ -202,8 +211,12 @@ folly::Expected<ClientSetup, ErrorCode> parseClientSetup(
202211
return folly::makeUnexpected(ErrorCode::PARSE_UNDERFLOW);
203212
}
204213
length -= numParams->second;
205-
auto res =
206-
parseSetupParams(cursor, length, numParams->first, clientSetup.params);
214+
auto res = parseSetupParams(
215+
cursor,
216+
length,
217+
v11Plus ? kVersionDraft11 : kVersionDraft10,
218+
numParams->first,
219+
clientSetup.params);
207220
if (res.hasError()) {
208221
return folly::makeUnexpected(res.error());
209222
}
@@ -228,8 +241,12 @@ folly::Expected<ServerSetup, ErrorCode> parseServerSetup(
228241
return folly::makeUnexpected(ErrorCode::PARSE_UNDERFLOW);
229242
}
230243
length -= numParams->second;
231-
auto res =
232-
parseSetupParams(cursor, length, numParams->first, serverSetup.params);
244+
auto res = parseSetupParams(
245+
cursor,
246+
length,
247+
serverSetup.selectedVersion,
248+
numParams->first,
249+
serverSetup.params);
233250
if (res.hasError()) {
234251
return folly::makeUnexpected(res.error());
235252
}
@@ -624,11 +641,13 @@ folly::Expected<folly::Unit, ErrorCode> MoQFrameParser::parseTrackRequestParams(
624641
p.asAuthToken = std::move(*tokenRes.value());
625642
} else {
626643
auto res = quic::decodeQuicInteger(cursor, length);
627-
if (!res) {
628-
return folly::makeUnexpected(ErrorCode::PARSE_UNDERFLOW);
644+
if (getDraftMajorVersion(*version_) < 11) {
645+
if (!res) {
646+
return folly::makeUnexpected(ErrorCode::PARSE_UNDERFLOW);
647+
}
648+
length -= res->second;
649+
res = quic::decodeQuicInteger(cursor, res->first);
629650
}
630-
length -= res->second;
631-
res = quic::decodeQuicInteger(cursor, res->first);
632651
if (!res) {
633652
return folly::makeUnexpected(ErrorCode::PARSE_UNDERFLOW);
634653
}
@@ -1791,9 +1810,10 @@ std::string MoQFrameWriter::encodeTokenValue(
17911810
}
17921811

17931812
bool includeParam(uint64_t version, uint64_t key) {
1794-
return key == getAuthorizationParamKey(version) ||
1795-
key == getMaxCacheDurationParamKey(version) ||
1796-
key == getDeliveryTimeoutParamKey(version);
1813+
return key == folly::to_underlying(SetupKey::MAX_REQUEST_ID) ||
1814+
key == folly::to_underlying(SetupKey::PATH) ||
1815+
(getDraftMajorVersion(version) >= 11 &&
1816+
key == folly::to_underlying(SetupKey::MAX_AUTH_TOKEN_CACHE_SIZE));
17971817
}
17981818

17991819
WriteResult writeClientSetup(
@@ -1834,12 +1854,15 @@ WriteResult writeClientSetup(
18341854
if (param.key == folly::to_underlying(SetupKey::MAX_REQUEST_ID) ||
18351855
param.key ==
18361856
folly::to_underlying(SetupKey::MAX_AUTH_TOKEN_CACHE_SIZE)) {
1837-
auto ret = quic::getQuicIntegerSize(param.asUint64);
1838-
if (ret.hasError()) {
1839-
XLOG(ERR) << "Invalid max requestID: " << param.asUint64;
1840-
return folly::makeUnexpected(quic::TransportErrorCode::INTERNAL_ERROR);
1857+
if (!v11Plus) {
1858+
auto ret = quic::getQuicIntegerSize(param.asUint64);
1859+
if (ret.hasError()) {
1860+
XLOG(ERR) << "Invalid max requestID: " << param.asUint64;
1861+
return folly::makeUnexpected(
1862+
quic::TransportErrorCode::INTERNAL_ERROR);
1863+
}
1864+
writeVarint(writeBuf, ret.value(), size, error);
18411865
}
1842-
writeVarint(writeBuf, ret.value(), size, error);
18431866
writeVarint(writeBuf, param.asUint64, size, error);
18441867
} else {
18451868
writeFixedString(writeBuf, param.asString, size, error);
@@ -1882,12 +1905,15 @@ WriteResult writeServerSetup(
18821905
if (param.key == folly::to_underlying(SetupKey::MAX_REQUEST_ID) ||
18831906
(param.key ==
18841907
folly::to_underlying(SetupKey::MAX_AUTH_TOKEN_CACHE_SIZE))) {
1885-
auto ret = quic::getQuicIntegerSize(param.asUint64);
1886-
if (ret.hasError()) {
1887-
XLOG(ERR) << "Invalid max requestID: " << param.asUint64;
1888-
return folly::makeUnexpected(quic::TransportErrorCode::INTERNAL_ERROR);
1908+
if (getDraftMajorVersion(serverSetup.selectedVersion) < 11) {
1909+
auto ret = quic::getQuicIntegerSize(param.asUint64);
1910+
if (ret.hasError()) {
1911+
XLOG(ERR) << "Invalid max requestID: " << param.asUint64;
1912+
return folly::makeUnexpected(
1913+
quic::TransportErrorCode::INTERNAL_ERROR);
1914+
}
1915+
writeVarint(writeBuf, ret.value(), size, error);
18891916
}
1890-
writeVarint(writeBuf, ret.value(), size, error);
18911917
writeVarint(writeBuf, param.asUint64, size, error);
18921918
} else {
18931919
writeFixedString(writeBuf, param.asString, size, error);
@@ -2066,12 +2092,14 @@ void MoQFrameWriter::writeTrackRequestParams(
20662092
} else if (
20672093
param.key == getDeliveryTimeoutParamKey(*version_) ||
20682094
param.key == getMaxCacheDurationParamKey(*version_)) {
2069-
auto res = quic::getQuicIntegerSize(param.asUint64);
2070-
if (!res) {
2071-
error = true;
2072-
return;
2095+
if (getDraftMajorVersion(*version_) < 11) {
2096+
auto res = quic::getQuicIntegerSize(param.asUint64);
2097+
if (!res) {
2098+
error = true;
2099+
return;
2100+
}
2101+
writeVarint(writeBuf, res.value(), size, error);
20732102
}
2074-
writeVarint(writeBuf, res.value(), size, error);
20752103
writeVarint(writeBuf, param.asUint64, size, error);
20762104
}
20772105
}

‎moxygen/test/MoQSessionTest.cpp‎

Lines changed: 44 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -97,12 +97,14 @@ class MoQSessionTest : public testing::TestWithParam<VersionParams>,
9797
}
9898

9999
EXPECT_EQ(setup.supportedVersions[0], getClientSupportedVersions()[0]);
100+
EXPECT_EQ(setup.params.at(0).key, folly::to_underlying(SetupKey::PATH));
101+
EXPECT_EQ(setup.params.at(0).asString, "/foo");
100102
EXPECT_EQ(
101-
setup.params.at(0).key, folly::to_underlying(SetupKey::MAX_REQUEST_ID));
102-
EXPECT_EQ(setup.params.at(0).asUint64, initialMaxRequestID_);
103-
if (setup.params.size() > 1) {
103+
setup.params.at(1).key, folly::to_underlying(SetupKey::MAX_REQUEST_ID));
104+
EXPECT_EQ(setup.params.at(1).asUint64, initialMaxRequestID_);
105+
if (setup.params.size() > 2) {
104106
EXPECT_EQ(
105-
setup.params.at(1).key,
107+
setup.params.at(2).key,
106108
folly::to_underlying(SetupKey::MAX_AUTH_TOKEN_CACHE_SIZE));
107109
} else {
108110
EXPECT_LT(setup.supportedVersions[0], kVersionDraft11);
@@ -193,6 +195,7 @@ class MoQSessionTest : public testing::TestWithParam<VersionParams>,
193195
return ClientSetup{
194196
.supportedVersions = getClientSupportedVersions(),
195197
.params = {
198+
SetupParameter{folly::to_underlying(SetupKey::PATH), "/foo", 0, {}},
196199
SetupParameter{
197200
folly::to_underlying(SetupKey::MAX_REQUEST_ID),
198201
"",
@@ -1435,6 +1438,43 @@ CO_TEST_P_X(V11OnlyTests, TrackStatusWithAuthorizationToken) {
14351438
clientSession_->close(SessionCloseErrorCode::NO_ERROR);
14361439
}
14371440

1441+
CO_TEST_P_X(MoQSessionTest, SubscribeWithParams) {
1442+
co_await setupMoQSession();
1443+
1444+
expectSubscribe([this](auto sub, auto pub) -> TaskSubscribeResult {
1445+
EXPECT_EQ(sub.params.size(), 2);
1446+
EXPECT_EQ(
1447+
sub.params.at(0).key,
1448+
getDeliveryTimeoutParamKey(getServerSelectedVersion()));
1449+
EXPECT_EQ(sub.params.at(0).asUint64, 5000);
1450+
EXPECT_EQ(
1451+
sub.params.at(1).key,
1452+
getAuthorizationParamKey(getServerSelectedVersion()));
1453+
if (getServerSelectedVersion() < kVersionDraft11) {
1454+
EXPECT_EQ(sub.params.at(1).asString, "auth_token_value");
1455+
} else {
1456+
EXPECT_EQ(sub.params.at(1).asAuthToken.tokenValue, "auth_token_value");
1457+
}
1458+
1459+
pub->subscribeDone(getTrackEndedSubscribeDone(sub.requestID));
1460+
co_return makeSubscribeOkResult(sub);
1461+
});
1462+
1463+
expectSubscribeDone();
1464+
1465+
SubscribeRequest subscribeRequest = getSubscribe(kTestTrackName);
1466+
subscribeRequest.params.push_back(
1467+
{getDeliveryTimeoutParamKey(getServerSelectedVersion()), "", 5000, {}});
1468+
subscribeRequest.params.push_back(
1469+
getAuthParam(getServerSelectedVersion(), "auth_token_value"));
1470+
1471+
auto res =
1472+
co_await clientSession_->subscribe(subscribeRequest, subscribeCallback_);
1473+
EXPECT_FALSE(res.hasError());
1474+
co_await subscribeDone_;
1475+
clientSession_->close(SessionCloseErrorCode::NO_ERROR);
1476+
}
1477+
14381478
// Missing Test Cases
14391479
// ===
14401480
// receive bidi stream on client

0 commit comments

Comments
 (0)