Attention is currently required from: Adam Rice, Chromium IPC Reviews.
Liza Burakova would like Chromium IPC Reviews and Adam Rice to review this change.
Socket broker classes are now Windows only.
Since socket brokering is only required on windows now, this CL makes
the relevant classes win only instead of platform agnostic.
BrokeredUdpClientSocket still has functionality for connecting
with a network handle, that will be removed in a follow up CL to
simplify connecting code.
Bug: 1485298
Change-Id: I7a05ccaed865e9fa99dcea7399379d86dbdf35e8
---
M content/browser/network/sandboxed_socket_broker_browsertest.cc
M content/test/BUILD.gn
M services/network/BUILD.gn
M services/network/brokered_client_socket_factory.cc
M services/network/brokered_udp_client_socket.cc
M services/network/brokered_udp_client_socket.h
M services/network/network_context.cc
M services/network/public/mojom/network_context.mojom
8 files changed, 18 insertions(+), 31 deletions(-)
To view, visit change 4897359. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Adam Rice, Chromium IPC Reviews.
Attention is currently required from: Adam Rice, Chromium IPC Reviews, Ken Buchanan.
gwsq would like Ken Buchanan to review this change authored by Liza Burakova.
Socket broker classes are now Windows only.
Since socket brokering is only required on windows now, this CL makes
the relevant classes win only instead of platform agnostic.
BrokeredUdpClientSocket still has functionality for connecting
with a network handle, that will be removed in a follow up CL to
simplify connecting code.
Bug: 1485298
Change-Id: I7a05ccaed865e9fa99dcea7399379d86dbdf35e8
---
M content/browser/network/sandboxed_socket_broker_browsertest.cc
M content/test/BUILD.gn
M services/network/BUILD.gn
M services/network/brokered_client_socket_factory.cc
M services/network/brokered_udp_client_socket.cc
M services/network/brokered_udp_client_socket.h
M services/network/network_context.cc
M services/network/public/mojom/network_context.mojom
8 files changed, 18 insertions(+), 31 deletions(-)
To view, visit change 4897359. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Adam Rice, Brendon Tiszka, Chromium IPC Reviews, Ken Buchanan.
gwsq would like Brendon Tiszka to review this change authored by Liza Burakova.
Attention is currently required from: Adam Rice, Brendon Tiszka, Ken Buchanan.
Liza Burakova has uploaded this change for review.
Attention is currently required from: Adam Rice, Brendon Tiszka, Ken Buchanan.
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
Shadow: tis...@chromium.org; IPC: ke...@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/).
Shadow IPC reviewer(s): tis...@chromium.org. Please conduct an IPC review and CR+1 when satisfied. Remember to add the main reviewers to the attention set if needed.
Main IPC reviewer(s): ke...@chromium.org. Please wait for the shadowed IPC reviewer to CR+1 before reviewing.
Shadowed: tis...@chromium.org
Reviewer source(s):
ke...@chromium.org, tis...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Attention is currently required from: Adam Rice, Ken Buchanan, Liza Burakova.
Patch set 2:Code-Review +1
1 comment:
Patchset:
mojom lgtm
To view, visit change 4897359. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Ken Buchanan, Liza Burakova.
Patch set 2:Code-Review +1
2 comments:
Patchset:
Nice cleanup! lgtm!
File services/network/brokered_udp_client_socket.h:
Patch Set #2, Line 50: // Windows only. Not intended to be used by non-brokered
Please re-wrap this comment if you can.
To view, visit change 4897359. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Liza Burakova.
To view, visit change 4897359. To unsubscribe, or for help writing mail filters, visit settings.
Patch Set #2, Line 50: // Windows only. Not intended to be used by non-brokered
Please re-wrap this comment if you can.
Done
To view, visit change 4897359. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Liza Burakova.
Liza Burakova uploaded patch set #3 to this change.
Socket broker classes are now Windows only.
Since socket brokering is only required on windows now, this CL makes
the relevant classes win only instead of platform agnostic.
BrokeredUdpClientSocket still has functionality for connecting
with a network handle, that will be removed in a follow up CL to
simplify connecting code.
Bug: 1485298
Change-Id: I7a05ccaed865e9fa99dcea7399379d86dbdf35e8
---
M content/browser/network/sandboxed_socket_broker_browsertest.cc
M content/test/BUILD.gn
M services/network/BUILD.gn
M services/network/brokered_client_socket_factory.cc
M services/network/brokered_udp_client_socket.cc
M services/network/brokered_udp_client_socket.h
M services/network/network_context.cc
M services/network/public/mojom/network_context.mojom
8 files changed, 22 insertions(+), 36 deletions(-)
To view, visit change 4897359. To unsubscribe, or for help writing mail filters, visit settings.
Patch set 3:Commit-Queue +2
Chromium LUCI CQ submitted this change.
2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: services/network/brokered_udp_client_socket.h
Insertions: 5, Deletions: 6.
@@ -47,12 +47,11 @@
};
// A client socket used exclusively with a socket broker. Currently intended for
-// Windows only. Not intended to be used by non-brokered
-// connections. Generally, all calls pass through to an underlying
-// TCPClientSocket API, but Bind and Connect are the sent to a privileged
-// process using the net:SocketBroker interface. This is because socket creation
-// needs to be brokered, and TCPClientSocket only creates and opens a socket
-// within Bind and Connect.
+// Windows only. Not intended to be used by non-brokered connections. Generally,
+// all calls pass through to an underlying TCPClientSocket API, but Bind and
+// Connect are the sent to a privileged process using the net:SocketBroker
+// interface. This is because socket creation needs to be brokered, and
+// TCPClientSocket only creates and opens a socket within Bind and Connect.
class COMPONENT_EXPORT(NETWORK_SERVICE) BrokeredUdpClientSocket
: public net::DatagramClientSocket {
public:
```
Socket broker classes are now Windows only.
Since socket brokering is only required on windows now, this CL makes
the relevant classes win only instead of platform agnostic.
BrokeredUdpClientSocket still has functionality for connecting
with a network handle, that will be removed in a follow up CL to
simplify connecting code.
Bug: 1485298
Change-Id: I7a05ccaed865e9fa99dcea7399379d86dbdf35e8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4897359
Reviewed-by: Brendon Tiszka <tis...@chromium.org>
Reviewed-by: Ken Buchanan <ke...@chromium.org>
Commit-Queue: Liza Burakova <li...@chromium.org>
Reviewed-by: Adam Rice <ri...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1204744}
---
M content/browser/network/sandboxed_socket_broker_browsertest.cc
M content/test/BUILD.gn
M services/network/BUILD.gn
M services/network/brokered_client_socket_factory.cc
M services/network/brokered_udp_client_socket.cc
M services/network/brokered_udp_client_socket.h
M services/network/network_context.cc
M services/network/public/mojom/network_context.mojom
8 files changed, 22 insertions(+), 36 deletions(-)