Add SSM support to Direct Sockets API (services/network and Blink layers) [chromium/src : main]
Omar Ramadan (Gerrit)
Omar Ramadan abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot added 2 comments![]( "Open in Gerrit")
Vector<String> joined_groups_;Why not hold MembershipKey in this vector? Then string operations to join strings + "@" won't be necessary.
Related details
- Andrew Rayskiy
- Omar Ramadan
- Reilly Grant
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot added 1 comment![]( "Open in Gerrit")
std::optional<net::IPAddress> MulticastController::ParseAndValidateIPAddress(IsValidMulticastAddress can be included in this function. It is fine if old joinGroup/leaveGroup will also be checked for multicast address.
Omar Ramadan (Gerrit)
Omar Ramadan added 3 comments![]( "Open in Gerrit")
Why not hold MembershipKey in this vector? Then string operations to join strings + "@" won't be necessary.
Done. Changed joined_groups_ from Vector<String> to Vector<MembershipKey> to eliminate string concatenation operations.
std::optional<net::IPAddress> MulticastController::ParseAndValidateIPAddress(IsValidMulticastAddress can be included in this function. It is fine if old joinGroup/leaveGroup will also be checked for multicast address.
Done. Created ParseAndValidateMulticastAddress() and ParseAndValidateUnicastAddress() that include the multicast check. Both joinGroup and leaveGroup now validate addresses appropriately.
'net::IPAddress',Omar RamadanWhy this is needed?
MulticastController uses it in the public API for ParseAndValidateIPAddress and IsValidMulticastAddress. The audit tool requires explicit approval for non-Blink types used in Blink code.
Related details
- Andrew Rayskiy
- Reilly Grant
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot added 2 comments![]( "Open in Gerrit")
HeapHashMap<String, Member<ScriptPromiseResolver<IDLUndefined>>>Why not use MembershipKey here as well as a key. Then membership_key_str will not be necessary. The hashing function will be required.
const String& ip_string,Since these functions are not used anywhere outside of this file, I recommend to move them to .cc and to anynimous namespace. This will also eliminate the need for touching third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py
Related details
- Andrew Rayskiy
- Omar Ramadan
- Reilly Grant
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omar Ramadan (Gerrit)
Omar Ramadan added 2 comments![]( "Open in Gerrit")
HeapHashMap<String, Member<ScriptPromiseResolver<IDLUndefined>>>Why not use MembershipKey here as well as a key. Then membership_key_str will not be necessary. The hashing function will be required.
Changed HeapHashMap keys from String to MembershipKey with custom hash function, eliminating membership_key_str conversions.
const String& ip_string,Since these functions are not used anywhere outside of this file, I recommend to move them to .cc and to anynimous namespace. This will also eliminate the need for touching third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py
Moved to anonymous namespace in .cc but audit still complained about net::IPAddress although was already used in multicast_controller.cc. Is it safe to ignore?
Related details
- Andrew Rayskiy
- Reilly Grant
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reilly Grant (Gerrit)
Reilly Grant added 2 comments![]( "Open in Gerrit")
bool IsValidMulticastAddress(const net::IPAddress& address) {Consider adding this to `IPAddress` like the existing `IsPubliclyRoutable` method.
return {};We now need to catch the case where an empty string was provided for this non-optional parameter.
Related details
- Andrew Rayskiy
- Omar Ramadan
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reilly Grant (Gerrit)
Reilly Grant added 1 comment![]( "Open in Gerrit")
Promise<undefined> joinGroup(DOMString ipAddress, optional DOMString? sourceAddress);Create a new feature in `runtime_enabled_features.json5` for this and add the [RuntimeEnabled] attribute to `sourceAddress` so that it's only accepted if the feature is enabled.
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 8 comments![]( "Open in Gerrit")
return StringHash::GetHash(key.group) ^Tbh I'd rather do a `StringHash::GetHash(key.ToString())` here -- it's unique enough
bool operator==(const MembershipKey& other) const {just default the operator?
String source; // Empty for ASM`optional<>`?
String group;I'd just keep `IP`, `IP` here instead of strings
ScriptPromise<IDLUndefined> joinGroup(ScriptState*,Wouldn't it be simpler to merge these functions? With `(ScriptState*, const String&, MulticastGroupOptions*, ExceptionState&`)?
const String& ip_string,Omar RamadanSince these functions are not used anywhere outside of this file, I recommend to move them to .cc and to anynimous namespace. This will also eliminate the need for touching third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py
Moved to anonymous namespace in .cc but audit still complained about net::IPAddress although was already used in multicast_controller.cc. Is it safe to ignore?
Yes, it's fine.
MembershipKey key;I'd create a proper constructor for this which takes `(const IP&, const optional<IP>&)`
Promise<undefined> joinGroup(DOMString ipAddress, optional DOMString? sourceAddress);I believe you wanted to go with MulticastGroupOptions as per the spec PR?
Related details
- Omar Ramadan
- Vlad Krot
Omar Ramadan (Gerrit)
Omar Ramadan added 11 comments![]( "Open in Gerrit")
thank you so much for the quick review all, and guiding me as a first time contributor. sorry for tagging folks unnecessarily
Tbh I'd rather do a `StringHash::GetHash(key.ToString())` here -- it's unique enough
Done
bool operator==(const MembershipKey& other) const {Omar Ramadanjust default the operator?
Done
String source; // Empty for ASMOmar Ramadan`optional<>`?
Done
I'd just keep `IP`, `IP` here instead of strings
Done
Wouldn't it be simpler to merge these functions? With `(ScriptState*, const String&, MulticastGroupOptions*, ExceptionState&`)?
Done
bool IsValidMulticastAddress(const net::IPAddress& address) {Consider adding this to `IPAddress` like the existing `IsPubliclyRoutable` method.
Done
We now need to catch the case where an empty string was provided for this non-optional parameter.
Done
I'd create a proper constructor for this which takes `(const IP&, const optional<IP>&)`
Done
Promise<undefined> joinGroup(DOMString ipAddress, optional DOMString? sourceAddress);I believe you wanted to go with MulticastGroupOptions as per the spec PR?
Done
Promise<undefined> joinGroup(DOMString ipAddress, optional DOMString? sourceAddress);Create a new feature in `runtime_enabled_features.json5` for this and add the [RuntimeEnabled] attribute to `sourceAddress` so that it's only accepted if the feature is enabled.
- Andrew Rayskiy
- Reilly Grant
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reilly Grant (Gerrit)
Reilly Grant added 5 comments![]( "Open in Gerrit")
bool IPAddress::IsMulticast() const {Use `IPAddressPrefixCheck` in this method.
if (!group_ip.has_value()) {
return {};
}I would move the check above into here to make sure we always catch cases where `group_ip` is nullopt without an exception having been thrown.
```suggestion
if (!group_ip.has_value()) {
if (!exception_state.HadException()) {
exception_state.ThrowTypeError("ipAddress must not be empty");
}
return {};
}
```
required DOMString groupAddress;```suggestion
DOMString groupAddress;
```
MulticastGroupOptions MakeSourceOptions(const char* source_address) {```suggestion
MulticastGroupOptions MakeSourceOptions(std::string_view source_address) {
```
status: "stable",This lets us land this change before the Intent to Ship is approved.
```suggestion
status: "experimental",
```
Related details
- Andrew Rayskiy
- Omar Ramadan
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omar Ramadan (Gerrit)
Omar Ramadan added 6 comments![]( "Open in Gerrit")
Use `IPAddressPrefixCheck` in this method.
Done
I would move the check above into here to make sure we always catch cases where `group_ip` is nullopt without an exception having been thrown.
```suggestion
if (!group_ip.has_value()) {
if (!exception_state.HadException()) {
exception_state.ThrowTypeError("ipAddress must not be empty");
}
return {};
}
```
Done
required DOMString groupAddress;Omar Ramadan```suggestion
DOMString groupAddress;
```
Done
MulticastGroupOptions MakeSourceOptions(const char* source_address) {```suggestion
MulticastGroupOptions MakeSourceOptions(std::string_view source_address) {
```
Done
This lets us land this change before the Intent to Ship is approved.
```suggestion
status: "experimental",
```
- Andrew Rayskiy
- Reilly Grant
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot added 1 comment![]( "Open in Gerrit")
status: "stable",Omar RamadanThis lets us land this change before the Intent to Ship is approved.
```suggestion
status: "experimental",
```
Done
We need a separate flag. This change will disable Multicast altogether which is already used by developers, and they expect this functionality to not disappear.
Also, I would suggest adding a flag in about://flags to simplify testing for those who are interested.
And do this all in a separate CL.
Reference - https://chromium-review.googlesource.com/c/chromium/src/+/6890773
Related details
- Andrew Rayskiy
- Omar Ramadan
- Reilly Grant
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omar Ramadan (Gerrit)
Omar Ramadan added 1 comment![]( "Open in Gerrit")
status: "stable",Omar RamadanThis lets us land this change before the Intent to Ship is approved.
```suggestion
status: "experimental",
```
Vlad KrotDone
We need a separate flag. This change will disable Multicast altogether which is already used by developers, and they expect this functionality to not disappear.
Also, I would suggest adding a flag in about://flags to simplify testing for those who are interested.
And do this all in a separate CL.
Reference - https://chromium-review.googlesource.com/c/chromium/src/+/6890773
- Reilly Grant
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot added 2 comments![]( "Open in Gerrit")
udp_socket_->JoinSourceGroup(group_address, *source_address,Source-Specific Multicast (SSM) feature flag must be checked here in case renderer was compromised. If the flag is disabled, but the function was called with source_address, then do mojo::ReportBadMessage("no permission to use source-specific multicast");
ScriptPromise<IDLUndefined> MulticastController::joinGroup(There should be a check for new feature flag here. Now the addition to multicast API is possible to use with the flag disabled.
Related details
- Omar Ramadan
- Reilly Grant
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot added 1 comment![]( "Open in Gerrit")
name: "SourceSpecificMulticastInDirectSockets",I don't think that a new permission policy is necessary. This is usually added to guard some dangerous functionality, I don't see how source specific multicast is more dangerous then the usual one.
Related details
- Omar Ramadan
- Reilly Grant
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot added 1 comment![]( "Open in Gerrit")
https://chromestatus.com/feature/6208452397498368Create intent to prototype.
Omar Ramadan (Gerrit)
Omar Ramadan added 4 comments![]( "Open in Gerrit")
https://chromestatus.com/feature/6208452397498368Omar RamadanCreate intent to prototype.
Done
I don't think that a new permission policy is necessary. This is usually added to guard some dangerous functionality, I don't see how source specific multicast is more dangerous then the usual one.
Done
udp_socket_->JoinSourceGroup(group_address, *source_address,Source-Specific Multicast (SSM) feature flag must be checked here in case renderer was compromised. If the flag is disabled, but the function was called with source_address, then do mojo::ReportBadMessage("no permission to use source-specific multicast");
Done
ScriptPromise<IDLUndefined> MulticastController::joinGroup(There should be a check for new feature flag here. Now the addition to multicast API is possible to use with the flag disabled.
- Reilly Grant
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot added 2 comments![]( "Open in Gerrit")
if (!base::FeatureList::IsEnabled(Nit: I think it was easier just check this flag in restricted_udp_socket, the reason multicast was passed as a variable, because it had check for permission policy.
// We don't require a separate permissions policy for SSM (see crbug.com/1520671).Related details
- Omar Ramadan
- Reilly Grant
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omar Ramadan (Gerrit)
Omar Ramadan added 2 comments![]( "Open in Gerrit")
Nit: I think it was easier just check this flag in restricted_udp_socket, the reason multicast was passed as a variable, because it had check for permission policy.
Done
// We don't require a separate permissions policy for SSM (see crbug.com/1520671).(see crbug.com/1520671).
How is this bug relevant here?
- Reilly Grant
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot voted Code-Review+1![]( "Open in Gerrit")
- Omar Ramadan
- Reilly Grant
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 1 comment![]( "Open in Gerrit")
IsMulticastAllowed(context_) &&I don't think it's necessary to propagate this all the way down the stack; since all processes generally have the same launch flags, you can simply do
```
if (source_address.has_value()) {
// Source-Specific Multicast (SSM)
if (!base::FeatureList::IsEnabled(blink::features::kSourceSpecificMulticastInDirectSockets)) {
// Calling this method with source_address without the SSM permission
// means that the renderer is sending faulty IPCs.
mojo::ReportBadMessage("no permission to use source-specific multicast");
return;
}
udp_socket_->JoinSourceGroup(group_address, *source_address,
std::move(callback));
} else {
// Any-Source Multicast (ASM)
udp_socket_->JoinGroup(group_address, std::move(callback));
}
```
In your `restricted_udp_socket.cc`
Related details
- Omar Ramadan
- Reilly Grant
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omar Ramadan (Gerrit)
Omar Ramadan added 1 comment![]( "Open in Gerrit")
IsMulticastAllowed(context_) &&I don't think it's necessary to propagate this all the way down the stack; since all processes generally have the same launch flags, you can simply do
```
if (source_address.has_value()) {
// Source-Specific Multicast (SSM)
if (!base::FeatureList::IsEnabled(blink::features::kSourceSpecificMulticastInDirectSockets)) {
// Calling this method with source_address without the SSM permission
// means that the renderer is sending faulty IPCs.
mojo::ReportBadMessage("no permission to use source-specific multicast");
return;
}
udp_socket_->JoinSourceGroup(group_address, *source_address,
std::move(callback));
} else {
// Any-Source Multicast (ASM)
udp_socket_->JoinGroup(group_address, std::move(callback));
}
```In your `restricted_udp_socket.cc`
@green...@google.com if we can use `FeatureList::IsEnabled` directly in `restricted_udp_socket.cc` to check both multicast and source specific multicast, should I remove `allow_multicast` all together from `content/browser`? cc @vk...@google.com
Related details
- Andrew Rayskiy
- Reilly Grant
- Vlad Krot
Vlad Krot (Gerrit)
Vlad Krot added 1 comment![]( "Open in Gerrit")
IsMulticastAllowed(context_) &&Omar RamadanI don't think it's necessary to propagate this all the way down the stack; since all processes generally have the same launch flags, you can simply do
```
if (source_address.has_value()) {
// Source-Specific Multicast (SSM)
if (!base::FeatureList::IsEnabled(blink::features::kSourceSpecificMulticastInDirectSockets)) {
// Calling this method with source_address without the SSM permission
// means that the renderer is sending faulty IPCs.
mojo::ReportBadMessage("no permission to use source-specific multicast");
return;
}
udp_socket_->JoinSourceGroup(group_address, *source_address,
std::move(callback));
} else {
// Any-Source Multicast (ASM)
udp_socket_->JoinGroup(group_address, std::move(callback));
}
```In your `restricted_udp_socket.cc`
@green...@google.com if we can use `FeatureList::IsEnabled` directly in `restricted_udp_socket.cc` to check both multicast and source specific multicast, should I remove `allow_multicast` all together from `content/browser`? cc @vk...@google.com
"should I remove allow_multicast all together from" - no, because AllowMulticast also contains result of permissions policy check for which you need ExecutionContext.
Related details
- Andrew Rayskiy
- Omar Ramadan
- Reilly Grant
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 1 comment![]( "Open in Gerrit")
IsMulticastAllowed(context_) &&Omar RamadanI don't think it's necessary to propagate this all the way down the stack; since all processes generally have the same launch flags, you can simply do
```
if (source_address.has_value()) {
// Source-Specific Multicast (SSM)
if (!base::FeatureList::IsEnabled(blink::features::kSourceSpecificMulticastInDirectSockets)) {
// Calling this method with source_address without the SSM permission
// means that the renderer is sending faulty IPCs.
mojo::ReportBadMessage("no permission to use source-specific multicast");
return;
}
udp_socket_->JoinSourceGroup(group_address, *source_address,
std::move(callback));
} else {
// Any-Source Multicast (ASM)
udp_socket_->JoinGroup(group_address, std::move(callback));
}
```In your `restricted_udp_socket.cc`
Vlad Krot@green...@google.com if we can use `FeatureList::IsEnabled` directly in `restricted_udp_socket.cc` to check both multicast and source specific multicast, should I remove `allow_multicast` all together from `content/browser`? cc @vk...@google.com
"should I remove allow_multicast all together from" - no, because AllowMulticast also contains result of permissions policy check for which you need ExecutionContext.
+1! allow_multicast is per-frame, FeatureList::IsEnabled is global
Related details
- Omar Ramadan
- Reilly Grant
Omar Ramadan (Gerrit)
Omar Ramadan added 1 comment![]( "Open in Gerrit")
Omar RamadanI don't think it's necessary to propagate this all the way down the stack; since all processes generally have the same launch flags, you can simply do
```
if (source_address.has_value()) {
// Source-Specific Multicast (SSM)
if (!base::FeatureList::IsEnabled(blink::features::kSourceSpecificMulticastInDirectSockets)) {
// Calling this method with source_address without the SSM permission
// means that the renderer is sending faulty IPCs.
mojo::ReportBadMessage("no permission to use source-specific multicast");
return;
}
udp_socket_->JoinSourceGroup(group_address, *source_address,
std::move(callback));
} else {
// Any-Source Multicast (ASM)
udp_socket_->JoinGroup(group_address, std::move(callback));
}
```In your `restricted_udp_socket.cc`
Vlad Krot@green...@google.com if we can use `FeatureList::IsEnabled` directly in `restricted_udp_socket.cc` to check both multicast and source specific multicast, should I remove `allow_multicast` all together from `content/browser`? cc @vk...@google.com
Andrew Rayskiy"should I remove allow_multicast all together from" - no, because AllowMulticast also contains result of permissions policy check for which you need ExecutionContext.
+1! allow_multicast is per-frame, FeatureList::IsEnabled is global
- Andrew Rayskiy
- Reilly Grant
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy voted Code-Review+1![]( "Open in Gerrit")
- Omar Ramadan
- Reilly Grant
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Andrew Rayskiy
- Omar Ramadan
- Reilly Grant
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot added 1 comment![]( "Open in Gerrit")
bool is_deleted = false;What is the reason for adding this?
Related details
- Andrew Rayskiy
- Omar Ramadan
- Reilly Grant
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omar Ramadan (Gerrit)
Omar Ramadan added 1 comment![]( "Open in Gerrit")
bool is_deleted = false;What is the reason for adding this?
Blink's hash tables use open addressing so when a collision occurs, the table probes for the next available slot. To work correctly, the table needs to distinguish between three states for each slot (empty/deleted/valid). If you simply marked deleted slots as "empty," you'd break the probing chain. The `is_deleted` acts as a sentinel to differentiate deleted entries as `true` while an invalid/empty IP address `false`
Related details
- Andrew Rayskiy
- Reilly Grant
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: tse...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): tse...@chromium.org
Reviewer source(s):
tse...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Andrew Rayskiy
- David Schinazi
- Reilly Grant
- Tom Sepez
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reilly Grant (Gerrit)
Reilly Grant added 2 comments![]( "Open in Gerrit")
// Calling this method with source_address without the SSM permission```suggestion
// Calling this method with source_address without the SSM feature enabled
```
mojo::ReportBadMessage("no permission to use source-specific multicast");This isn't a permissions thing.
```suggestion
mojo::ReportBadMessage("Source-specific multicast disabled");
```
Related details
- Andrew Rayskiy
- David Schinazi
- Omar Ramadan
- Tom Sepez
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omar Ramadan (Gerrit)
Omar Ramadan added 3 comments![]( "Open in Gerrit")
thank you
// Calling this method with source_address without the SSM permission```suggestion
// Calling this method with source_address without the SSM feature enabled
```
Fix applied.
mojo::ReportBadMessage("no permission to use source-specific multicast");This isn't a permissions thing.
```suggestion
mojo::ReportBadMessage("Source-specific multicast disabled");
```
- Andrew Rayskiy
- David Schinazi
- Reilly Grant
- Tom Sepez
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reilly Grant (Gerrit)
Reilly Grant added 1 comment![]( "Open in Gerrit")
thank you
It looks like you haven't uploaded a new version of this CL.
Related details
- Andrew Rayskiy
- David Schinazi
- Omar Ramadan
- Tom Sepez
- Vlad Krot
David Schinazi (Gerrit)
David Schinazi added 1 comment![]( "Open in Gerrit")
Switching to one of the primary reviewers from //services/network/OWNERS
Related details
- Adam Rice
- Andrew Rayskiy
- Omar Ramadan
- Tom Sepez
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Tom Sepez added 3 comments![]( "Open in Gerrit")
LG otherwise.
assertEq(multicastController.joinedGroups[0].groupAddress, ssmMulticastGroup);nit: wrap at 80?
JoinGroup(IPAddress group_address, IPAddress? source_address)So are multiple calls with different source_address filters allowed? Does such a call replace a prior one? Or does it form a set of allowed addresses?
Related details
- Adam Rice
- Andrew Rayskiy
- Omar Ramadan
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 11 comments![]( "Open in Gerrit")
Generally looks good, just a few things to change.
// Calling this method with source_address without the SSM permissionOmar Ramadan```suggestion
// Calling this method with source_address without the SSM feature enabled
```
Fix applied.
The comment doesn't look updated to me.
mojo::ReportBadMessage("no permission to use source-specific multicast");Omar RamadanThis isn't a permissions thing.
```suggestion
mojo::ReportBadMessage("Source-specific multicast disabled");
```
Fix applied.
It still looks like the message is `"no permission to use source-specific multicast"` to me.
return key.ToString().Impl()->GetHash();This is a very inefficient hash function, as it has to do memory allocation to create the String. I suggest something like
```
static unsigned GetHash(const MembershipKey& key) {
size_t hash = base::FastHash(key.group.bytes());
if (key.source.has_value()) {
size_t source_hash = base::FastHash(key.source->bytes());
if constexpr (sizeof(size_t) == sizeof(uint32_t)) {
hash = base::HashInts32(hash, source_hash);
} else {
hash = base::HashInts64(hash, source_hash);
}
}
return static_cast<unsigned>(hash);
}
```
if (ip_address.empty()) {The next 30 lines or so are duplicated in `leaveGroup()`. I suggest factoring them out into a common function so you can do something like
```
const auto [group_ip, source_ip] =
ParseAndCheckGroupIPs(ip_address, options, exception_state);
```
return {};It would be good to add
```
CHECK(exception_state.HadException());
```
just to make it clear that an exception is always thrown in this case.
SourceSpecificMulticastInDirectSocketsEnabled()) {The body of this if statement should be unreachable because the feature is already conditional in the IDL file. If you want to be extra sure, you can do
```
CHECK(!source_ip.has_value() ||
RuntimeEnabledFeatures::SourceSpecificMulticastInDirectSocketsEnabled());
```
for (const MembershipKey& existing_membership : joined_groups_) {I don't know if joining a lot of groups is a realistic thing to do, but if it is, this loop would make it an O(N^2) operation. On way to make this change more efficient would be to have
```
HashSet<net::IPAddress> asm_groups_;
HashSet<net::IPAddress> ssm_groups_;
```
defined, then you only need to check that `group_id` is not already present in the wrong set.
if (source_ip.has_value() &&The body of this if statement should be unreachable because the feature is already conditional in the IDL file. If you want to be extra sure, you can do
```
CHECK(!source_ip.has_value() ||
RuntimeEnabledFeatures::SourceSpecificMulticastInDirectSocketsEnabled());
```
TEST(MulticastControllerTest, SSM_CannotJoinASMAndSSMForSameGroup) {We should also have a test doing SSM first and then ASM, since there are different code paths for each case.
TEST(MulticastControllerTest, SSM_IPv6WithSource) {We should also have tests that an IPv4 multicast address with an IPv6 source address fails, and vice-versa.
Related details
Adam Rice (Gerrit)
Adam Rice added 1 comment![]( "Open in Gerrit")
Sorry for the slow response! I didn't realise I'd had this for almost a month.
Omar Ramadan (Gerrit)
Omar Ramadan added 13 comments![]( "Open in Gerrit")
- Hash function: Replaced String allocation with base::FastHash() on raw bytes + base::HashInts32/64 for combining
- Code deduplication: Factored out ParseAndValidateGroupIPs() helper for join/leave
- Validation: Added CHECK(exception_state.HadException()) after parse failures
- Feature check: Replaced runtime if (!...Enabled()) with CHECK() since IDL gates the option
- ASM/SSM mixing: Added asm_groups_/ssm_groups_ HashSets for O(1) lookup instead of iterating joined_groups_
- restricted_udp_socket.cc: Added comment explaining faulty IPC check for SSM feature
- New tests: SSM_CannotJoinSSMThenASMForSameGroup, SSM_IPv4MulticastWithIPv6SourceFails, SSM_IPv6MulticastWithIPv4SourceFails
- udp.js: Wrapped lines at 80 chars
- Mojom comments: Clarified that multiple JoinGroup calls with different sources form a set of allowed sources
assertEq(multicastController.joinedGroups[0].groupAddress, ssmMulticastGroup);Omar Ramadannit: wrap at 80?
Done
JoinGroup(IPAddress group_address, IPAddress? source_address)So are multiple calls with different source_address filters allowed? Does such a call replace a prior one? Or does it form a set of allowed addresses?
Yes updating comment for clarity
// Calling this method with source_address without the SSM permissionOmar Ramadan```suggestion
// Calling this method with source_address without the SSM feature enabled
```
Adam RiceFix applied.
The comment doesn't look updated to me.
Done
mojo::ReportBadMessage("no permission to use source-specific multicast");Omar RamadanThis isn't a permissions thing.
```suggestion
mojo::ReportBadMessage("Source-specific multicast disabled");
```
Adam RiceFix applied.
It still looks like the message is `"no permission to use source-specific multicast"` to me.
Done
This is a very inefficient hash function, as it has to do memory allocation to create the String. I suggest something like
```
static unsigned GetHash(const MembershipKey& key) {
size_t hash = base::FastHash(key.group.bytes());
if (key.source.has_value()) {
size_t source_hash = base::FastHash(key.source->bytes());
if constexpr (sizeof(size_t) == sizeof(uint32_t)) {
hash = base::HashInts32(hash, source_hash);
} else {
hash = base::HashInts64(hash, source_hash);
}
}
return static_cast<unsigned>(hash);
}
```
Acknowledged
The next 30 lines or so are duplicated in `leaveGroup()`. I suggest factoring them out into a common function so you can do something like
```
const auto [group_ip, source_ip] =
ParseAndCheckGroupIPs(ip_address, options, exception_state);
```
Done
It would be good to add
```
CHECK(exception_state.HadException());
```
just to make it clear that an exception is always thrown in this case.
Done
The body of this if statement should be unreachable because the feature is already conditional in the IDL file. If you want to be extra sure, you can do
```
CHECK(!source_ip.has_value() ||
RuntimeEnabledFeatures::SourceSpecificMulticastInDirectSocketsEnabled());
```
Acknowledged
for (const MembershipKey& existing_membership : joined_groups_) {I don't know if joining a lot of groups is a realistic thing to do, but if it is, this loop would make it an O(N^2) operation. On way to make this change more efficient would be to have
```
HashSet<net::IPAddress> asm_groups_;
HashSet<net::IPAddress> ssm_groups_;
```
defined, then you only need to check that `group_id` is not already present in the wrong set.
Done
The body of this if statement should be unreachable because the feature is already conditional in the IDL file. If you want to be extra sure, you can do
```
CHECK(!source_ip.has_value() ||
RuntimeEnabledFeatures::SourceSpecificMulticastInDirectSocketsEnabled());
```
Done
TEST(MulticastControllerTest, SSM_CannotJoinASMAndSSMForSameGroup) {We should also have a test doing SSM first and then ASM, since there are different code paths for each case.
Done
We should also have tests that an IPv4 multicast address with an IPv6 source address fails, and vice-versa.
- Adam Rice
- Andrew Rayskiy
- Reilly Grant
- Tom Sepez
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice voted and added 4 comments![]( "Open in Gerrit")
Votes added by Adam Rice
| Commit-Queue | +1 |
4 comments
ASSERT_FALSE(addresses.empty()) << "No IPv4 interface found";Unfortunately I don't think we can guarantee that every Chromium developer will have an IPv4 interface active. I think the best we can do is
```
if (addresses.empty()) {
GTEST_SKIP() << "No IPv4 interface found";
}
```
This risks us losing test coverage without noticing, but I don't have a better idea.
I'm also worried these tests might fail if someone has multicast disabled, but the existing tests have the same issue, so let's ignore that problem for now.
EvalJsResult::IsError());Because we cannot distinguish the type of error in C++, it's better to design the JavaScript to return a string indicating success when the expected exception is thrown.
HashSet<net::IPAddress> ssm_groups_;Maybe use `HashSet<std::pair<net::IPAddress, net::IPAddress>>` so that you can include both the group and source in the set and so avoid an O(N) lookup when leaving the group?
If that doesn't work, you could do something like `HashMap<net::IPAddress, HashSet<net::IPAddress>>` where the key to the map is the source address and the key to the set is the group.
ssm_groups_.insert(membership_key.group);What happens if someone calls `joinGroup()` a second time with the same parameters before `OnJoinedGroup()` is called?
Related details
- Adam Rice
- Andrew Rayskiy
- Omar Ramadan
- Reilly Grant
- Tom Sepez
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omar Ramadan (Gerrit)
Omar Ramadan added 4 comments![]( "Open in Gerrit")
ASSERT_FALSE(addresses.empty()) << "No IPv4 interface found";Unfortunately I don't think we can guarantee that every Chromium developer will have an IPv4 interface active. I think the best we can do is
```
if (addresses.empty()) {
GTEST_SKIP() << "No IPv4 interface found";
}
```
This risks us losing test coverage without noticing, but I don't have a better idea.I'm also worried these tests might fail if someone has multicast disabled, but the existing tests have the same issue, so let's ignore that problem for now.
Fair point, but if you are running without IPv4 I probably can't get your reports of less coverage.
Because we cannot distinguish the type of error in C++, it's better to design the JavaScript to return a string indicating success when the expected exception is thrown.
Done
Maybe use `HashSet<std::pair<net::IPAddress, net::IPAddress>>` so that you can include both the group and source in the set and so avoid an O(N) lookup when leaving the group?
If that doesn't work, you could do something like `HashMap<net::IPAddress, HashSet<net::IPAddress>>` where the key to the map is the source address and the key to the set is the group.
Done
What happens if someone calls `joinGroup()` a second time with the same parameters before `OnJoinedGroup()` is called?
If `joinGroup()` is called a second time with the same parameters before `OnJoinedGroup()` is called, it throws an `InvalidStateError` with the message "Already joining this group/source combination". This is handled in multicast_controller.cc:197-202 the pending join is tracked in `join_group_promises_` immediately when `joinGroup()` is called, so subsequent calls with the same group/source are rejected until `OnJoinedGroup()` completes and removes the entry.
Related details
- Adam Rice
- Andrew Rayskiy
- Reilly Grant
- Tom Sepez
- Vlad Krot
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |