admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Revert "ice server parsing: return RTCError with more details"

Browse Source 
This reverts commit c4b0bde7f2daabc4e0667fb73d096d1cf0819226.

Reason for revert: Breaks downstream tests.

Basically, ParseIceServers() and other functions have changed
the return type, and this breaks tests at compile time.

Is it possible to reland with backwards compatible versions that return
the previous type? Then they can be removed afterwards.

Original change's description:
> ice server parsing: return RTCError with more details
>
> surfacing those errors to the API (without specific details) instead of just the logging.
>
> BUG=webrtc:14539
>
> Change-Id: Id907ebeb08b96b0e4225a016a37a12d58889091b
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/278340
> Reviewed-by: Henrik Boström <hbos@webrtc.org>
> Reviewed-by: Harald Alvestrand <hta@webrtc.org>
> Commit-Queue: Philipp Hancke <phancke@microsoft.com>
> Cr-Commit-Position: refs/heads/main@{#38356}

Bug: webrtc:14539
Change-Id: I4df936ff865f87759936c5d0425478fe51051dc8
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/278960
Owners-Override: Mirko Bonadei <mbonadei@webrtc.org>
Auto-Submit: Mirko Bonadei <mbonadei@webrtc.org>
Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#38359}
This commit is contained in:
Mirko Bonadei 2022-10-12 06:51:54 +00:00 committed by WebRTC LUCI CQ
parent 84fcc269f6
commit 4d47e0b2be
4 changed files with 49 additions and 64 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

86
pc/ice_server_parsing.cc
View File

@ -155,7 +155,7 @@ std::tuple<bool, absl::string_view, int> ParseHostnameAndPortFromString(
// Adds a STUN or TURN server to the appropriate list,
// by parsing `url` and using the username/password in `server`.
RTCError ParseIceServerUrl(
RTCErrorType ParseIceServerUrl(
const PeerConnectionInterface::IceServer& server,
absl::string_view url,
cricket::ServerAddresses* stun_servers,
@ -186,24 +186,20 @@ RTCError ParseIceServerUrl(
std::vector<absl::string_view> transport_tokens =
rtc::split(tokens[1], '=');
if (transport_tokens[0] != kTransport) {
LOG_AND_RETURN_ERROR(
RTCErrorType::SYNTAX_ERROR,
"ICE server parsing failed: Invalid transport parameter key.");
RTC_LOG(LS_WARNING) << "Invalid transport parameter key.";
return RTCErrorType::SYNTAX_ERROR;
}
if (transport_tokens.size() < 2) {
LOG_AND_RETURN_ERROR(
RTCErrorType::SYNTAX_ERROR,
"ICE server parsing failed: Transport parameter missing value.");
RTC_LOG(LS_WARNING) << "Transport parameter missing value.";
return RTCErrorType::SYNTAX_ERROR;
}
absl::optional<cricket::ProtocolType> proto =
cricket::StringToProto(transport_tokens[1]);
if (!proto ||
(*proto != cricket::PROTO_UDP && *proto != cricket::PROTO_TCP)) {
LOG_AND_RETURN_ERROR(
RTCErrorType::SYNTAX_ERROR,
"ICE server parsing failed: Transport parameter should "
"always be udp or tcp.");
RTC_LOG(LS_WARNING) << "Transport parameter should always be udp or tcp.";
return RTCErrorType::SYNTAX_ERROR;
}
turn_transport_type = *proto;
}
@ -211,10 +207,8 @@ RTCError ParseIceServerUrl(
auto [service_type, hoststring] =
GetServiceTypeAndHostnameFromUri(uri_without_transport);
if (service_type == ServiceType::INVALID) {
RTC_LOG(LS_ERROR) << "Invalid transport parameter in ICE URI: " << url;
LOG_AND_RETURN_ERROR(
RTCErrorType::SYNTAX_ERROR,
"ICE server parsing failed: Invalid transport parameter in ICE URI");
RTC_LOG(LS_WARNING) << "Invalid transport parameter in ICE URI: " << url;
return RTCErrorType::SYNTAX_ERROR;
}
// GetServiceTypeAndHostnameFromUri should never give an empty hoststring
@ -227,25 +221,22 @@ RTCError ParseIceServerUrl(
}
if (hoststring.find('@') != absl::string_view::npos) {
RTC_LOG(LS_ERROR) << "Invalid url with long deprecated user@host syntax: "
<< uri_without_transport;
LOG_AND_RETURN_ERROR(RTCErrorType::SYNTAX_ERROR,
"ICE server parsing failed: Invalid url with long "
"deprecated user@host syntax");
RTC_LOG(LS_WARNING) << "Invalid url: " << uri_without_transport;
RTC_LOG(LS_WARNING)
<< "Note that user-info@ in turn:-urls is long-deprecated.";
return RTCErrorType::SYNTAX_ERROR;
}
auto [success, address, port] =
ParseHostnameAndPortFromString(hoststring, default_port);
if (!success) {
RTC_LOG(LS_ERROR) << "Invalid hostname format: " << uri_without_transport;
LOG_AND_RETURN_ERROR(RTCErrorType::SYNTAX_ERROR,
"ICE server parsing failed: Invalid hostname format");
RTC_LOG(LS_WARNING) << "Invalid hostname format: " << uri_without_transport;
return RTCErrorType::SYNTAX_ERROR;
}
if (port <= 0 || port > 0xffff) {
RTC_LOG(LS_ERROR) << "Invalid port: " << port;
LOG_AND_RETURN_ERROR(RTCErrorType::SYNTAX_ERROR,
"ICE server parsing failed: Invalid port");
RTC_LOG(LS_WARNING) << "Invalid port: " << port;
return RTCErrorType::SYNTAX_ERROR;
}
switch (service_type) {
@ -258,10 +249,8 @@ RTCError ParseIceServerUrl(
if (server.username.empty() || server.password.empty()) {
// The WebRTC spec requires throwing an InvalidAccessError when username
// or credential are ommitted; this is the native equivalent.
LOG_AND_RETURN_ERROR(
RTCErrorType::INVALID_PARAMETER,
"ICE server parsing failed: TURN server with empty "
"username or password");
RTC_LOG(LS_WARNING) << "TURN server with empty username or password";
return RTCErrorType::INVALID_PARAMETER;
}
// If the hostname field is not empty, then the server address must be
// the resolved IP for that host, the hostname is needed later for TLS
@ -274,11 +263,10 @@ RTCError ParseIceServerUrl(
if (!IPFromString(address, &ip)) {
// When hostname is set, the server address must be a
// resolved ip address.
LOG_AND_RETURN_ERROR(
RTCErrorType::INVALID_PARAMETER,
"ICE server parsing failed: "
"IceServer has hostname field set, but URI does not "
"contain an IP address.");
RTC_LOG(LS_WARNING)
<< "IceServer has hostname field set, but URI does not "
"contain an IP address.";
return RTCErrorType::INVALID_PARAMETER;
}
socket_address.SetResolvedIP(ip);
}
@ -299,16 +287,15 @@ RTCError ParseIceServerUrl(
default:
// We shouldn't get to this point with an invalid service_type, we should
// have returned an error already.
LOG_AND_RETURN_ERROR(
RTCErrorType::INTERNAL_ERROR,
"ICE server parsing failed: Unexpected service type");
RTC_DCHECK_NOTREACHED() << "Unexpected service type";
return RTCErrorType::INTERNAL_ERROR;
}
return RTCError::OK();
return RTCErrorType::NONE;
}
} // namespace
RTCError ParseIceServers(
RTCErrorType ParseIceServers(
const PeerConnectionInterface::IceServers& servers,
cricket::ServerAddresses* stun_servers,
std::vector<cricket::RelayServerConfig>* turn_servers) {
@ -316,26 +303,25 @@ RTCError ParseIceServers(
if (!server.urls.empty()) {
for (const std::string& url : server.urls) {
if (url.empty()) {
LOG_AND_RETURN_ERROR(RTCErrorType::SYNTAX_ERROR,
"ICE server parsing failed: Empty uri.");
RTC_LOG(LS_WARNING) << "Empty uri.";
return RTCErrorType::SYNTAX_ERROR;
}
RTCError err =
RTCErrorType err =
ParseIceServerUrl(server, url, stun_servers, turn_servers);
if (!err.ok()) {
if (err != RTCErrorType::NONE) {
return err;
}
}
} else if (!server.uri.empty()) {
// Fallback to old .uri if new .urls isn't present.
RTCError err =
RTCErrorType err =
ParseIceServerUrl(server, server.uri, stun_servers, turn_servers);
if (!err.ok()) {
if (err != RTCErrorType::NONE) {
return err;
}
} else {
LOG_AND_RETURN_ERROR(RTCErrorType::SYNTAX_ERROR,
"ICE server parsing failed: Empty uri.");
RTC_LOG(LS_WARNING) << "Empty uri.";
return RTCErrorType::SYNTAX_ERROR;
}
}
// Candidates must have unique priorities, so that connectivity checks
@ -345,7 +331,7 @@ RTCError ParseIceServers(
// First in the list gets highest priority.
turn_server.priority = priority--;
}
return RTCError::OK();
return RTCErrorType::NONE;
}
} // namespace webrtc

2
pc/ice_server_parsing.h
View File

@ -27,7 +27,7 @@ namespace webrtc {
//
// Intended to be used to convert/validate the servers passed into a
// PeerConnection through RTCConfiguration.
RTC_EXPORT RTCError
RTC_EXPORT RTCErrorType
ParseIceServers(const PeerConnectionInterface::IceServers& servers,
cricket::ServerAddresses* stun_servers,
std::vector<cricket::RelayServerConfig>* turn_servers);

13
pc/ice_server_parsing_unittest.cc
View File

@ -62,8 +62,8 @@ class IceServerParsingTest : public ::testing::Test {
server.tls_cert_policy = tls_certificate_policy;
server.hostname = hostname;
servers.push_back(server);
return webrtc::ParseIceServers(servers, &stun_servers_, &turn_servers_)
.ok();
return webrtc::ParseIceServers(servers, &stun_servers_, &turn_servers_) ==
webrtc::RTCErrorType::NONE;
}
protected:
@ -229,8 +229,8 @@ TEST_F(IceServerParsingTest, ParseMultipleUrls) {
server.username = "foo";
server.password = "bar";
servers.push_back(server);
EXPECT_TRUE(
webrtc::ParseIceServers(servers, &stun_servers_, &turn_servers_).ok());
EXPECT_EQ(webrtc::RTCErrorType::NONE,
webrtc::ParseIceServers(servers, &stun_servers_, &turn_servers_));
EXPECT_EQ(1U, stun_servers_.size());
EXPECT_EQ(1U, turn_servers_.size());
}
@ -245,9 +245,8 @@ TEST_F(IceServerParsingTest, TurnServerPrioritiesUnique) {
server.username = "foo";
server.password = "bar";
servers.push_back(server);
EXPECT_TRUE(
webrtc::ParseIceServers(servers, &stun_servers_, &turn_servers_).ok());
EXPECT_EQ(webrtc::RTCErrorType::NONE,
webrtc::ParseIceServers(servers, &stun_servers_, &turn_servers_));
EXPECT_EQ(2U, turn_servers_.size());
EXPECT_NE(turn_servers_[0].priority, turn_servers_[1].priority);
}

12
pc/peer_connection.cc
View File

@ -596,10 +596,10 @@ RTCError PeerConnection::Initialize(
cricket::ServerAddresses stun_servers;
std::vector<cricket::RelayServerConfig> turn_servers;
RTCError parse_error =
RTCErrorType parse_error =
ParseIceServers(configuration.servers, &stun_servers, &turn_servers);
if (!parse_error.ok()) {
return parse_error;
if (parse_error != RTCErrorType::NONE) {
return RTCError(parse_error, "ICE server parse failed");
}
// Add the turn logging id to all turn servers
@ -1519,10 +1519,10 @@ RTCError PeerConnection::SetConfiguration(
// Parse ICE servers before hopping to network thread.
cricket::ServerAddresses stun_servers;
std::vector<cricket::RelayServerConfig> turn_servers;
RTCError parse_error =
RTCErrorType parse_error =
ParseIceServers(configuration.servers, &stun_servers, &turn_servers);
if (!parse_error.ok()) {
return parse_error;
if (parse_error != RTCErrorType::NONE) {
return RTCError(parse_error, "ICE server parse failed");
}
// Add the turn logging id to all turn servers
for (cricket::RelayServerConfig& turn_server : turn_servers) {