Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
76403004:
An implementation of chrome.socket.secure(). (Closed)
|
Created:
7 years, 1 month ago by lally Modified:
6 years, 5 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, jar (doing other things), asvitkine+watch_chromium.org, chromium-apps-reviews_chromium.org, erikwright+watch_chromium.org, Kevin Greer Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAn implementation of chrome.socket.secure().
It adds a new TLSSocket type in the extensions:: namespace, that acts similarly
to TCP/UDPSocket. Failures to enable TLS will auto-close the socket.
API Proposal: https://docs.google.com/a/google.com/document/d/1XSjBtLjyGXQmgB0XE4kxuvM0p2yKI_8T5aOiKDJ9lFg/edit
BUG=132896
TESTED=some, mostly with a custom chrome app, and with the included test.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285804
Patch Set 1 #Patch Set 2 : Pulled out TLSSocket into a new file, cleaned up a bit. #Patch Set 3 : Added TLS Options and unit tests. #Patch Set 4 : Lint fixes, and a gclient-sync to help trybots merge. #Patch Set 5 : Added a virtual tag to a WriteImpl method, to make clang happy. #Patch Set 6 : Fixing another cross-platform build issue. #Patch Set 7 : Fixing another cross-platform build issue (try 2) #
Total comments: 35
Patch Set 8 : Made TLSSocket resumable, responses to sleevi's round-1 comments. #
Total comments: 27
Patch Set 9 : After Renaud's comments. Added a secure() to sockets.tcp. #
Total comments: 20
Patch Set 10 : Another round of responses to Renaud. #
Total comments: 58
Patch Set 11 : Updates for Ryan's comments. #Patch Set 12 : Save and Restores Persistent/Paused state when securing. #
Total comments: 22
Patch Set 13 : An integration test, and some nits fixed. #
Total comments: 34
Patch Set 14 : Separated socket/TLS tests, more smart pointers for stuff.' #Patch Set 15 : One more commenting fix. #Patch Set 16 : Just a git cl format. #Patch Set 17 : Double. Spaces removed. #
Total comments: 16
Patch Set 18 : Updated documentation and clearer identifiers. #Patch Set 19 : Added a check on whether the socket to be TLS'd has a pending read. #
Total comments: 12
Patch Set 20 : An un-DCHECK-ification, a new test, a test clarification, and a lot of nit-double-spaces. #
Total comments: 11
Patch Set 21 : Added a large-write test, some spelling/doc fixes, and a stronger check for canonicalized names. #
Total comments: 2
Patch Set 22 : Addressed, @rsleevi's comments, added a new TLS test, further separated TLS and TCP tests, and reba… #
Total comments: 10
Patch Set 23 : Tiny, tiny, tiny changes to clean up the diffs post-rebase. #
Total comments: 2
Patch Set 24 : Updated test for >16k writes, ignore_result() used, dep on URLRequestContextGetter removed. #Patch Set 25 : Rebase with build fixes only. (2) #Patch Set 26 : Stop using Profile and move completely to URLRequestContext. #
Total comments: 14
Patch Set 27 : Rebase to LKGR for Jul 14. No other changes. #Patch Set 28 : Final or near-final patch for submission. Ryan's comments are addressed, and histograms.xml change… #Patch Set 29 : Updated tls_socket_unittest for a mocked method signature change. (2) #
Total comments: 11
Patch Set 30 : LKGR rebase before reviewer edits. #Patch Set 31 : Updates after mek@ comments. #Patch Set 32 : Morning LKGR Rebase. #Messages
Total messages: 52 (0 generated)
This is great work, but, as mentioned on apps-dev@chromium.org, I think it would be better to add TLS functionality to chrome.sockets.tcp instead of chrome.socket. chrome.socket has major deficiencies (which is why we create the new chrome.sockets.xxx namespaces) and imho we should encourage developers to move to the new API instead of adding more features to the existing API. Note that the work done in CL is probably 80% portable "as-is" to the new API implementation.
Thanks. I'll be working on integrating sockets_tcp in a follow-up CL.
Definitely needs review by the extensions team. I've tried to focus on how the net/ APIs are used here, but I also provided some style and comment nits. I'm not clear all how this is working, given SSLClientSocket's contract regarding Connect(). https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... File chrome/browser/extensions/api/api_resource_manager.h (right): https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/api_resource_manager.h:224: linked_ptr<T> old_resource_ptr = api_resource_map_[api_resource_id]; Is there a reason to take a copy of this linked_ptr? Seems like you could just do T* resource = api_resource_map_[api_resource_id].get(); if (resource && extension_id == resource->owner_extension_id()) { api_resource_map_[api_resource_id].reset(api_resource); return true; } return false; Note that "else after return" is discouraged by the Chromium style guide. - http://www.chromium.org/developers/coding-style#Code_Formatting Finally, is either code correct? It seems it should be if (resource && extension_id != resource->owner_extension_id()) return false; api_resource_map_[api_resource_id].reset(api_resource); return true; This covers the case where an API resource ID is not yet assigned (and assigns it), where the API resource ID is assigned (and owned by the extension), and when the API resource ID is assigned and NOT owned by the extension (and fails appropriately) https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket.h (right): https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket.h:112: std::string original_hostname_; nit: naming wise, original_hostname_ seems a bit confusing, in that it's not clear what a socket's "original" hostname means, and when/how an alternative hostname might be returned. Is there a reason this isn't just called Hostname? And does this need to be on the base Socket interface, or should this be kept for the derived interface? https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket_api.cc (right): https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:954: VLOG(1) << "chrome.socket.secure: Work(): no socket, bolting."; These VLOGs are all included in release/official builds, and thus carry with them non-trivial cost to build and image size. It seems like they should all be DVLOGs or (ideally) removed. https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:981: // P2PSocketHostTcpBase::StartTls() Don't do comments like this. It will be out of date within days. https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:984: // We already know from GetSocketType() above that it's TCP. nit: Please rework the comments here to avoid the "we" eg: // |socket| is guaranteed to be a TCPSocket because of the GetSocketType() check above. https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:991: // APIs that we'd like it to (SetKeepAlive, SetNoDelay). This is a bug. You shouldn't be doing this. Set your KeepAlive / NoDelay settings before beginning negotiation. Note that SSLClientSocket expects to be given an *already connected* socket handle, so you should first do the TCP socket connect, get your TCP callback, set your options, and *then* hand that off to the SSLSocket. https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:1014: uint16 v_min = 0, v_max = 0; As per the Google C++ style guide, eschew abbreviations. "version_min" and "version_max" are much more reasonable. https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:1015: api::socket::TLSVersionConstraints *versions = As per http://www.chromium.org/developers/coding-style#Code_Formatting , the * goes with the type, not the variable name api::socket::TLSVersionConstraints* versions = ... https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:1031: net::ClientSocketFactory::GetDefaultFactory(); The "ideal" form is to pass this in as a variable somehow, rather than to grab it from the factory. Just $.02 https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:1037: socket_handle.Pass(), host_port, ssl_config, context); BUG: This is broken, because you never connected |socket_handle|. CreateSSLClientSocket expects a *connected* socket. https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:1044: TlsConnectDone(status); From an API point, this sort of async-or-recursion tends to cause subtle bugs regarding lifetimes. It's typically much better to re-organize this as all of net/ does, into a state machine-driven loop, rather than to recurse into the function here. https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket_api.h (right): https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.h:71: // Note: Cast needed here, because "T" may be a subclass of "Socket". awong/jyasskin: Am I insane for thinking this is scary in light of vtable adjustments? https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.h:536: // The new SSL stream we're running over the existing TCP socket. "we" / Pronouns in comments considered harmful. https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3... https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.h:538: // The socket we're securing. same nit https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tcp_socket.cc (right): https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/tcp_socket.cc:319: server = accept_socket_.release() != NULL || server; The whole |server| checks are *really* odd to me. Seems like they should be either CHECKs, DCHECKs, or early failures *before* you reset all the object state. if (server_socket_ || accept_socket_) { return false; https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tcp_socket.h (right): https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/tcp_socket.h:58: // attempt to close it. Useful for when someone else takes it over. comment nit re: pronouns https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket.cc (right): https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:76: OnReadComplete(io_buffer, result); This is a dangerous pattern (see comments in the TLS socket establishment). Recursion with async callbacks can lead to dangerous patterns. In particular, I'm concerned about the situation where the callback may want to delete |this| - you now have two potential dangerous situations: 1) if member variables are ever accessed beyond this line, we've got danger 2) |callback| may cause re-entrancy back into the TLSSocket::Read() caller, which can then cause dangerous side-effects This may be an underlying/fundamentally scary part about the extensions code, but it's why the net/ code is extremely careful to avoid this pattern. I recently saw both 1 and 2 in Cast (ChromeCast) code, which was following the pattern established here, so I'm extremely uncomfortable about it. https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket.h (right): https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.h:29: // inner net::StreamSocket. The remaining few do actual TLS work. This definitely violates the SOLID design of is-a inheritance, particularly around the Connect/Bind use case. All of these Forwards / Fails comments really seem to highlight that TLSSocket is-not-a Socket - Forwards shouldn't need to be documented (it's an implementation detail), and Fails seems a violation of the API contract. https://codereview.chromium.org/76403004/diff/270001/chrome/common/extensions... File chrome/common/extensions/api/socket.idl (right): https://codereview.chromium.org/76403004/diff/270001/chrome/common/extensions... chrome/common/extensions/api/socket.idl:112: DOMString? max; Does this IDL support enums instead? Seems like a good way to make it a stronger API guarantee (that is, trying to use a newer socket version on an older version of chrome = throw a WebIDL exception)
Ryan: I did what I could, within what I know how to do with the extensions API. Specifically, avoiding recursion. If it's not up to snuff, I'll take a deeper look. Renaud: I've made the TLS Socket resumable, and factored out the secure() code. Did you want the secure() api to be duplicated under sockets.tcp? Thanks to you both! https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket.h (right): https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket.h:112: std::string original_hostname_; On 2013/11/25 17:30:13, Ryan Sleevi wrote: > nit: naming wise, original_hostname_ seems a bit confusing, in that it's not > clear what a socket's "original" hostname means, and when/how an alternative > hostname might be returned. Fair enough. Changed. > > Is there a reason this isn't just called Hostname? And does this need to be on > the base Socket interface, or should this be kept for the derived interface? The socket's an instance of either TCPSocket or UDPSocket when Connect() is called. So I figured this would let us support DTLS later (if desired). I can move it over to TCPSocket if preferred. https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket_api.cc (right): https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:954: VLOG(1) << "chrome.socket.secure: Work(): no socket, bolting."; On 2013/11/25 17:30:13, Ryan Sleevi wrote: > These VLOGs are all included in release/official builds, and thus carry with > them non-trivial cost to build and image size. > > It seems like they should all be DVLOGs or (ideally) removed. All but 1 removed. The remainder moved to DVLOG (to print the TLS connection's result code) https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:981: // P2PSocketHostTcpBase::StartTls() On 2013/11/25 17:30:13, Ryan Sleevi wrote: > Don't do comments like this. It will be out of date within days. Acknowledged. The file/line# were removed, but the member/method kept. If that's still not acceptable, I'll remove the whole thing. https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:984: // We already know from GetSocketType() above that it's TCP. On 2013/11/25 17:30:13, Ryan Sleevi wrote: > nit: Please rework the comments here to avoid the "we" > > eg: > > // |socket| is guaranteed to be a TCPSocket because of the GetSocketType() check > above. This comment was removed, but instances of "we" were removed. https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:991: // APIs that we'd like it to (SetKeepAlive, SetNoDelay). On 2013/11/25 17:30:13, Ryan Sleevi wrote: > This is a bug. You shouldn't be doing this. Set your KeepAlive / NoDelay > settings before beginning negotiation. > > Note that SSLClientSocket expects to be given an *already connected* socket > handle, so you should first do the TCP socket connect, get your TCP callback, > set your options, and *then* hand that off to the SSLSocket. I've disabled the KeepAlive/NoDelay methods. The socket's already connected when passed to SSLClientSocket. https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:1014: uint16 v_min = 0, v_max = 0; On 2013/11/25 17:30:13, Ryan Sleevi wrote: > As per the Google C++ style guide, eschew abbreviations. "version_min" and > "version_max" are much more reasonable. Done. https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:1015: api::socket::TLSVersionConstraints *versions = On 2013/11/25 17:30:13, Ryan Sleevi wrote: > As per http://www.chromium.org/developers/coding-style#Code_Formatting , the * > goes with the type, not the variable name > > api::socket::TLSVersionConstraints* versions = ... Done. https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:1037: socket_handle.Pass(), host_port, ssl_config, context); On 2013/11/25 17:30:13, Ryan Sleevi wrote: > BUG: This is broken, because you never connected |socket_handle|. > CreateSSLClientSocket expects a *connected* socket. I set it to underlying_socket above. That socket was checked to make sure it was connected (and TCP). I've since cleaned it up a bit. https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:1044: TlsConnectDone(status); On 2013/11/25 17:30:13, Ryan Sleevi wrote: > From an API point, this sort of async-or-recursion tends to cause subtle bugs > regarding lifetimes. > > It's typically much better to re-organize this as all of net/ does, into a state > machine-driven loop, rather than to recurse into the function here. I've added a comment on why this can't recurse into another secure() call on this socket. Is that the concern you had? https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket_api.h (right): https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.h:536: // The new SSL stream we're running over the existing TCP socket. On 2013/11/25 17:30:13, Ryan Sleevi wrote: > "we" / Pronouns in comments considered harmful. > > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3... Done. https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.h:538: // The socket we're securing. On 2013/11/25 17:30:13, Ryan Sleevi wrote: > same nit Done. https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tcp_socket.cc (right): https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/tcp_socket.cc:319: server = accept_socket_.release() != NULL || server; On 2013/11/25 17:30:13, Ryan Sleevi wrote: > The whole |server| checks are *really* odd to me. > > Seems like they should be either CHECKs, DCHECKs, or early failures *before* you > reset all the object state. > > if (server_socket_ || accept_socket_) { > return false; Fair enough. Cleaned up. https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tcp_socket.h (right): https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/tcp_socket.h:58: // attempt to close it. Useful for when someone else takes it over. On 2013/11/25 17:30:13, Ryan Sleevi wrote: > comment nit re: pronouns Done. https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket.cc (right): https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:76: OnReadComplete(io_buffer, result); On 2013/11/25 17:30:13, Ryan Sleevi wrote: > This is a dangerous pattern (see comments in the TLS socket establishment). > Recursion with async callbacks can lead to dangerous patterns. > > In particular, I'm concerned about the situation where the callback may want to > delete |this| - you now have two potential dangerous situations: > > 1) if member variables are ever accessed beyond this line, we've got danger > 2) |callback| may cause re-entrancy back into the TLSSocket::Read() caller, > which can then cause dangerous side-effects > > This may be an underlying/fundamentally scary part about the extensions code, > but it's why the net/ code is extremely careful to avoid this pattern. I > recently saw both 1 and 2 in Cast (ChromeCast) code, which was following the > pattern established here, so I'm extremely uncomfortable about it. Fair enough on both counts. The existing APIs do the same as above. To help mitigate, I don't touch any member variables after invoking the ReadCallback. The callback can delete 'this' without harm, and it can't invoke any other methods on this instance after deleting it (it has to go through the integer-handle API to get here, and that API will bounce just return an error). https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket.h (right): https://codereview.chromium.org/76403004/diff/270001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.h:29: // inner net::StreamSocket. The remaining few do actual TLS work. On 2013/11/25 17:30:13, Ryan Sleevi wrote: > This definitely violates the SOLID design of is-a inheritance, particularly > around the Connect/Bind use case. > > All of these Forwards / Fails comments really seem to highlight that TLSSocket > is-not-a Socket - Forwards shouldn't need to be documented (it's an > implementation detail), and Fails seems a violation of the API contract. No disagreement here, but this is an artifact of secure()'s convert-after-connect and Socket's union-of-interfaces semantics. No implementation of Socket will have both RecvFrom and SetKeepAlive() implemented. The existing extensions::(TCP|UDP)Socket have the same forward/fail semantics for their methods. https://codereview.chromium.org/76403004/diff/270001/chrome/common/extensions... File chrome/common/extensions/api/socket.idl (right): https://codereview.chromium.org/76403004/diff/270001/chrome/common/extensions... chrome/common/extensions/api/socket.idl:112: DOMString? max; On 2013/11/25 17:30:13, Ryan Sleevi wrote: > Does this IDL support enums instead? Seems like a good way to make it a stronger > API guarantee (that is, trying to use a newer socket version on an older version > of chrome = throw a WebIDL exception) It does, but not with the identifiers the API spec gave (e.g., "tls1.1"). Would you prefer to keep the identifiers the same as the flag, or swap the periods with _ and use the IDL?
https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket.h (right): https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket.h:52: } The convention for simple accessors is "set_hostname". https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket.h:56: The convention for simple accessor is "hostname()" (+ "const" in this case) https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket_api.cc (right): https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:917: void SocketSecureFunction::Work() { Using "Work" is slightly different than the convention we use. I would change to 1) not override the "Work" method at all and 2) move the code to the "AsyncWorkStart" method. https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:920: const char * error_str = TLSSocket::SecureTCPSocket( There are issues with the way error handling is done here. I would change the code like this: 1) Move the initial code of TLSSocket::SecureTCPSocket into this method, and call "AsyncWorkCompleted()" whenever you set an error string (see for example SocketsUdpSendFunction::AsyncWorkStart) 2) Change TLSSocket::SecureTCPSocket to return "void" and to have no "result" parameter. 3) Ensure the callback is always called 4) Remove the ""async_completed_needs_invocation_" field 5) Update the end of the "TlsConnectDone" method to do something along the lines of (see SocketsUdpSendFunction::SetSendResult): if (net_result != net::OK) error_ = net::ErrorToString(net_result); results_ = socket::Secure::Results::Create(net_result); AsyncWorkCompleted(); https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:954: if (async_completed_needs_invocation_) { As mentioned above, the code here should be something along the lines of: if (net_result != net::OK) error_ = net::ErrorToString(net_result); results_ = socket::Secure::Results::Create(net_result); AsyncWorkCompleted(); https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket_api.h (right): https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.h:541: bool async_completed_needs_invocation_; See AsyncWorkStart comment: this flag should not be needed. https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket.cc (right): https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:152: namespace impl { Convention: Remove the "impl" namespace and move this function to the anonymous namespace at the top of the file. https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:155: int result ) { No space before ")" https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:190: int result = -1; Use "net::ERROR_FAILED" instead of "-1". https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:211: As mentioned in another comment, the 3 checks above should move to the API function implementation (AsyncWorkStart method). https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket.h (right): https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.h:19: typedef base::Callback<void(Socket*, int)> SecureCallback; Should "Socket*" be "TLSSocket*"? https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket_unittest.cc (right): https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket_unittest.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. nit: no "(c)" https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... File chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.h (right): https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.h:242: /*class SocketsTcpSecureFunction : public TCPSocketAsyncApiFunction { Should be implemented if "sockets_tcp.idl" is updated with a "resume" function. https://codereview.chromium.org/76403004/diff/380001/chrome/common/extensions... File chrome/common/extensions/api/socket.idl (right): https://codereview.chromium.org/76403004/diff/380001/chrome/common/extensions... chrome/common/extensions/api/socket.idl:351: ConnectCallback callback); I would create a "SecureCallback" (even though it has the same signature as ConnectCallback).
Gentlemen, please take another look. Ryan: this patch-set, versus the prior, isn't much different. So, if you've already started on that one, there shouldn't be much of a surprise here. Renaud: I'll ping you tomorrow re: resume(). https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket.h (right): https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket.h:52: } On 2013/12/09 23:02:03, rpaquay wrote: > The convention for simple accessors is "set_hostname". Done. https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket.h:56: On 2013/12/09 23:02:03, rpaquay wrote: > The convention for simple accessor is "hostname()" (+ "const" in this case) Done. https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket_api.cc (right): https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:917: void SocketSecureFunction::Work() { On 2013/12/09 23:02:03, rpaquay wrote: > Using "Work" is slightly different than the convention we use. I would change to > 1) not override the "Work" method at all and 2) move the code to the > "AsyncWorkStart" method. Done. https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:920: const char * error_str = TLSSocket::SecureTCPSocket( On 2013/12/09 23:02:03, rpaquay wrote: > There are issues with the way error handling is done here. I would change the > code like this: > 1) Move the initial code of TLSSocket::SecureTCPSocket into this method, and > call "AsyncWorkCompleted()" whenever you set an error string (see for example > SocketsUdpSendFunction::AsyncWorkStart) > 2) Change TLSSocket::SecureTCPSocket to return "void" and to have no "result" > parameter. > 3) Ensure the callback is always called > 4) Remove the ""async_completed_needs_invocation_" field > 5) Update the end of the "TlsConnectDone" method to do something along the lines > of (see SocketsUdpSendFunction::SetSendResult): > if (net_result != net::OK) > error_ = net::ErrorToString(net_result); > results_ = socket::Secure::Results::Create(net_result); > AsyncWorkCompleted(); Done. https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:954: if (async_completed_needs_invocation_) { On 2013/12/09 23:02:03, rpaquay wrote: > As mentioned above, the code here should be something along the lines of: > if (net_result != net::OK) > error_ = net::ErrorToString(net_result); > results_ = socket::Secure::Results::Create(net_result); > AsyncWorkCompleted(); Done. https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket_api.h (right): https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.h:541: bool async_completed_needs_invocation_; On 2013/12/09 23:02:03, rpaquay wrote: > See AsyncWorkStart comment: this flag should not be needed. Done. https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket.cc (right): https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:152: namespace impl { On 2013/12/09 23:02:03, rpaquay wrote: > Convention: Remove the "impl" namespace and move this function to the anonymous > namespace at the top of the file. Done. https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:155: int result ) { On 2013/12/09 23:02:03, rpaquay wrote: > No space before ")" Done. https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:190: int result = -1; On 2013/12/09 23:02:03, rpaquay wrote: > Use "net::ERROR_FAILED" instead of "-1". Done. https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:211: On 2013/12/09 23:02:03, rpaquay wrote: > As mentioned in another comment, the 3 checks above should move to the API > function implementation (AsyncWorkStart method). I moved them, but kept simpler versions here that just returned instead of allowed bad inputs. https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket.h (right): https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.h:19: typedef base::Callback<void(Socket*, int)> SecureCallback; On 2013/12/09 23:02:03, rpaquay wrote: > Should "Socket*" be "TLSSocket*"? Done. https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket_unittest.cc (right): https://codereview.chromium.org/76403004/diff/380001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket_unittest.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/12/09 23:02:03, rpaquay wrote: > nit: no "(c)" Done. https://codereview.chromium.org/76403004/diff/380001/chrome/common/extensions... File chrome/common/extensions/api/socket.idl (right): https://codereview.chromium.org/76403004/diff/380001/chrome/common/extensions... chrome/common/extensions/api/socket.idl:351: ConnectCallback callback); On 2013/12/09 23:02:03, rpaquay wrote: > I would create a "SecureCallback" (even though it has the same signature as > ConnectCallback). Done.
https://codereview.chromium.org/76403004/diff/400001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket_api.cc (right): https://codereview.chromium.org/76403004/diff/400001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:927: Profile* profile = GetProfile(); nit: move this closer to first usage. https://codereview.chromium.org/76403004/diff/400001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:960: SetResult(new base::FundamentalValue(result)); remove this line (see comment below) https://codereview.chromium.org/76403004/diff/400001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:969: results_ = api::socket::Secure::Results::Create(result); move this out of the "if" block. https://codereview.chromium.org/76403004/diff/400001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket.cc (right): https://codereview.chromium.org/76403004/diff/400001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:67: const char kSocketNotFoundError[] = "Socket not found"; I think this is not used anymore in this file. https://codereview.chromium.org/76403004/diff/400001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket.h (right): https://codereview.chromium.org/76403004/diff/400001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.h:76: // |url_request_getter| is Released() in this call. Note: |callback| may It is not clear to me why "url_request_getter" needs to be released. I am not familiar with this component, but it looks to me the caller doesn't actually "AddRef" it. Furthermore, looking at SecureTCPSocket implementation, "Release" is not called in all code path. https://codereview.chromium.org/76403004/diff/400001/chrome/browser/extension... File chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc (right): https://codereview.chromium.org/76403004/diff/400001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc:23: "Only TCP sockets are supported for TLS."; This message should not be needed (use DCHEK instead), as all sockets used with this API will be of type TCP. https://codereview.chromium.org/76403004/diff/400001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc:476: if (socket->GetSocketType() != Socket::TYPE_TCP The check for TYPE_TCP should be DCHECK, because it should always be the case. I am not 100% sure what "ClientStream()" is supposed to check, but it seems like it should be part of the DCHECK too (unless we are checking for client connection established). https://codereview.chromium.org/76403004/diff/400001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc:499: SetResult(new base::FundamentalValue(result)); This call is not needed if you always use "results_ = " as noted below. https://codereview.chromium.org/76403004/diff/400001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc:508: results_ = api::socket::Secure::Results::Create(result); 1) move this out of the if block 2) RHS should be "api::sockets::tcp::Secure::Results::Create(result)"; https://codereview.chromium.org/76403004/diff/400001/chrome/common/extensions... File chrome/common/extensions/api/sockets_tcp.idl (right): https://codereview.chromium.org/76403004/diff/400001/chrome/common/extensions... chrome/common/extensions/api/sockets_tcp.idl:256: SecureCallback callback); nit: can you move this method between "disconnect" and "send". (trying to keep methods in order they are called from a client app).
PTAL. Thanks. https://codereview.chromium.org/76403004/diff/400001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket_api.cc (right): https://codereview.chromium.org/76403004/diff/400001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:927: Profile* profile = GetProfile(); On 2013/12/16 20:18:46, rpaquay wrote: > nit: move this closer to first usage. Done. https://codereview.chromium.org/76403004/diff/400001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:960: SetResult(new base::FundamentalValue(result)); On 2013/12/16 20:18:46, rpaquay wrote: > remove this line (see comment below) Done. https://codereview.chromium.org/76403004/diff/400001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:969: results_ = api::socket::Secure::Results::Create(result); On 2013/12/16 20:18:46, rpaquay wrote: > move this out of the "if" block. Done. https://codereview.chromium.org/76403004/diff/400001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket.cc (right): https://codereview.chromium.org/76403004/diff/400001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:67: const char kSocketNotFoundError[] = "Socket not found"; On 2013/12/16 20:18:46, rpaquay wrote: > I think this is not used anymore in this file. Good catch, thanks. https://codereview.chromium.org/76403004/diff/400001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket.h (right): https://codereview.chromium.org/76403004/diff/400001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.h:76: // |url_request_getter| is Released() in this call. Note: |callback| may On 2013/12/16 20:18:46, rpaquay wrote: > It is not clear to me why "url_request_getter" needs to be released. I am not > familiar with this component, but it looks to me the caller doesn't actually > "AddRef" it. Furthermore, looking at SecureTCPSocket implementation, "Release" > is not called in all code path. Good point. I've moved the Release() call to the calling classes (SocketSecureFunction and SocketsTcpSecureFunction's TlsConnectDone() methods). https://codereview.chromium.org/76403004/diff/400001/chrome/browser/extension... File chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc (right): https://codereview.chromium.org/76403004/diff/400001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc:23: "Only TCP sockets are supported for TLS."; On 2013/12/16 20:18:46, rpaquay wrote: > This message should not be needed (use DCHEK instead), as all sockets used with > this API will be of type TCP. Ah, fixed. I was actually trying to stop already-TLS sockets from trying to re-secure, but didn't adjust the error messages or symbols. Fixed. https://codereview.chromium.org/76403004/diff/400001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc:476: if (socket->GetSocketType() != Socket::TYPE_TCP On 2013/12/16 20:18:46, rpaquay wrote: > The check for TYPE_TCP should be DCHECK, because it should always be the case. I > am not 100% sure what "ClientStream()" is supposed to check, but it seems like > it should be part of the DCHECK too (unless we are checking for client > connection established). I've added some comments and clarified this part. https://codereview.chromium.org/76403004/diff/400001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc:499: SetResult(new base::FundamentalValue(result)); On 2013/12/16 20:18:46, rpaquay wrote: > This call is not needed if you always use "results_ = " as noted below. Ok, done. Thanks. https://codereview.chromium.org/76403004/diff/400001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc:508: results_ = api::socket::Secure::Results::Create(result); On 2013/12/16 20:18:46, rpaquay wrote: > 1) move this out of the if block > 2) RHS should be "api::sockets::tcp::Secure::Results::Create(result)"; Done. https://codereview.chromium.org/76403004/diff/400001/chrome/common/extensions... File chrome/common/extensions/api/sockets_tcp.idl (right): https://codereview.chromium.org/76403004/diff/400001/chrome/common/extensions... chrome/common/extensions/api/sockets_tcp.idl:256: SecureCallback callback); On 2013/12/16 20:18:46, rpaquay wrote: > nit: can you move this method between "disconnect" and "send". (trying to keep > methods in order they are called from a client app). Done.
Just a ... few... comments. I still have some real issues with the inheritance-that-is-not-inheritance, and I think we need to work those out before I'll feel comfortable approving. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket.h (right): https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket.h:115: std::string hostname_; This is not consistent with the implementation, given that you allow set_hostname to replace the hostname given to Connect(). https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket_api.cc (right): https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:924: int result = net::ERR_INVALID_ARGUMENT; You never assign to result, but I think it makes this code less readable to constantly have to check and ensure this. I'd recommend updating lines 929, 938, and 945 with an explicit net::ERR_INVALID_ARGUMENT parameter. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:925: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO)); style nit: Normally we put pre-condition checks first in the function (eg: consistent with line 914) https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:937: || tcp_socket->ClientStream() == NULL) { style: Wrap after binary operators, not before ( http://www.chromium.org/developers/coding-style#Code_Formatting ) DANGER: You do the cast before the comparison. Seems like this could be rewritten as if (socket->GetSocketType() != Socket::TYPE_TCP || static_cast<TCPSocket*>(socket)->ClientStream() == NULL) { } https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:964: RemoveSocket(params_->socket_id); naming: sock -> socket DESIGN: I find this conditional confusing, with respect to |result|, in that it's not clear if sock != NULL and result != net::OK is a valid result. It seems like sock == NULL if result != net::OK, and sock != NULL if result == net::OK https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket_api.h (right): https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.h:46: virtual void Set(const std::string& extension_id, int api_resource_id, style nit: api_resource_id should be on its own line, as per http://www.chromium.org/developers/coding-style#Code_Formatting style nit: * goes with type, not variable - see http://www.chromium.org/developers/coding-style#Code_Formatting https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.h:530: private: DISALLOW_COPY_AND_ASSIGN ? Although this seems to be an endemic problem within this file (and perhaps, and unfortunately, intentional?) https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tcp_socket.cc (right): https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tcp_socket.cc:312: VLOG(1) << "TCPSocket::Release getting called in server mode. Odd."; nit: Delete the " Odd." https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tcp_socket.cc:321: // scoped_ptr.release(). outdated comment https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket.cc (right): https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:19: namespace { style nit: line break https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:47: if (result == net::OK) { Generally speaking, it's better to detect and handle errors first, and return (if appropriate), and then let the 'success' case dominate this function. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:58: // Failed TLS connection. This'll delete the underlying TCPClientSocket. comment nit: This'll -> This will https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:63: } // namespace style nit: Line break https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:71: const std::string& owner_extension_id) style nit: unnecessary extra space https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:127: read_callback_.Reset(); STYLE: simplify this as base::ResetAndReturn(&read_callback_).Run(result, io_buffer); https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:138: return tls_socket_->Write(io_buffer, io_buffer_size, callback); style: indent https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:189: || tcp_socket->ClientStream() == NULL || !socket->IsConnected()) { style: inproper binary operator wrapping - http://www.chromium.org/developers/coding-style#Code_Formatting https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:195: socket->GetPeerAddress(&dest_host_port_pair); BUG: GetPeerAddress returns a result. Check it. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:213: url_request_getter->Release(); BUG: ?!? Seems incredibly dangerous here. You should be using a scoped_refptr<> and shouldn't be calling Release yourself. It is especially dangerous for lifetimes and such to be releasing it so explicitly. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:244: socket_handle.Pass(), host_port, ssl_config, context).release(); DESIGN: Make your ownership explicit here. For example, just reading this code (242 - 259), it's impossible to reason when |ssl_socket| gets deleted (FWIW, it requires digging into TlsConnectDone to see there is a pseudo-ownership transfer) https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket.h (right): https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.h:26: // behavioral leakage from the underlying implementation. style nit: single space after a period, not double. Applies throughout. style nit: Pronouns in comments considered harmful - https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3... For what it's worth, I'm still confused on that last sentence. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.h:40: const CompletionCallback& callback) OVERRIDE; This still heavily rankles my design sentiments, because this clearly violates the is-a inheritance model being suggested here. I'd like to be able to find a solution that doesn't require this. If we can't, we've got a design issue with the sockets API in general. Same for every other "Fails" below. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.h:83: SecureCallback callback); style nit: four space indent comment nit: Explain the lifetime requirements for |profile| comment/style nit: Is ownership of |socket| transferred? If so, it should be scoped_ptr<Socket> and use our C++11 emulation of scoped_ptr<T>::Pass() style nit: "SecureCallback" -> "const SecureCallback&" naming nit: SecureTCPSocket is arguably ambiguous with respect to "Is it a verb - eg: secure this TCP socket - or a noun - eg: this is a constructor for a secure TCP socket". Perhaps "UpgradeSocketToSecureSocket" or something equally descriptive and unambiguously verb-ified. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.h:87: const net::CompletionCallback& callback) OVERRIDE; style: line breaks - http://www.chromium.org/developers/coding-style#Code_Formatting https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.h:88: void OnReadComplete(scoped_refptr<net::IOBuffer> io_buffer, int result); style nit: "scoped_refptr<net::IOBuffer>" -> "const scoped_refptr<IOBuffer>&" https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... File chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc (right): https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc:462: int result = net::ERR_INVALID_ARGUMENT; style: Previous comments about using |reuslt| here https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc:480: || socket->ClientStream() == NULL) { style: Conditionals on preceding line. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... File chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.h (right): https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.h:247: protected: STYLE: Follow the local style (and dominant Chromium style) by including a line break between public / protected / private See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_... https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.h:253: scoped_ptr<api::socket::Secure::Params> params_; style: Line break
I think I've gotten everything. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket.h (right): https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket.h:115: std::string hostname_; On 2013/12/17 00:37:40, Ryan Sleevi wrote: > This is not consistent with the implementation, given that you allow > set_hostname to replace the hostname given to Connect(). Connect() wants a resolved hostname (the string contains an IP address). I've updated the documentation. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket_api.cc (right): https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:924: int result = net::ERR_INVALID_ARGUMENT; On 2013/12/17 00:37:40, Ryan Sleevi wrote: > You never assign to result, but I think it makes this code less readable to > constantly have to check and ensure this. > > I'd recommend updating lines 929, 938, and 945 with an explicit > net::ERR_INVALID_ARGUMENT parameter. Done. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:925: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO)); On 2013/12/17 00:37:40, Ryan Sleevi wrote: > style nit: Normally we put pre-condition checks first in the function (eg: > consistent with line 914) Done. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:937: || tcp_socket->ClientStream() == NULL) { On 2013/12/17 00:37:40, Ryan Sleevi wrote: > style: Wrap after binary operators, not before ( > http://www.chromium.org/developers/coding-style#Code_Formatting ) > > > DANGER: You do the cast before the comparison. Seems like this could be > rewritten as > > if (socket->GetSocketType() != Socket::TYPE_TCP || > static_cast<TCPSocket*>(socket)->ClientStream() == NULL) { > > } Done. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:964: RemoveSocket(params_->socket_id); On 2013/12/17 00:37:40, Ryan Sleevi wrote: > naming: sock -> socket > > DESIGN: I find this conditional confusing, with respect to |result|, in that > it's not clear if sock != NULL and result != net::OK is a valid result. > > It seems like sock == NULL if result != net::OK, and sock != NULL if result == > net::OK Ah, fair enough. socket can only be non-null if result == net::OK. I've got a DCHECK in now to make sure that's true. The error path now runs if either (socket == NULL || result != net::OK). https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket_api.h (right): https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.h:46: virtual void Set(const std::string& extension_id, int api_resource_id, On 2013/12/17 00:37:40, Ryan Sleevi wrote: > style nit: api_resource_id should be on its own line, as per > http://www.chromium.org/developers/coding-style#Code_Formatting > > style nit: * goes with type, not variable - see > http://www.chromium.org/developers/coding-style#Code_Formatting Done. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.h:530: private: On 2013/12/17 00:37:40, Ryan Sleevi wrote: > DISALLOW_COPY_AND_ASSIGN ? Although this seems to be an endemic problem within > this file (and perhaps, and unfortunately, intentional?) Done. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tcp_socket.cc (right): https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tcp_socket.cc:312: VLOG(1) << "TCPSocket::Release getting called in server mode. Odd."; On 2013/12/17 00:37:40, Ryan Sleevi wrote: > nit: Delete the " Odd." Done. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tcp_socket.cc:321: // scoped_ptr.release(). On 2013/12/17 00:37:40, Ryan Sleevi wrote: > outdated comment Thanks. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket.cc (right): https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:19: namespace { On 2013/12/17 00:37:40, Ryan Sleevi wrote: > style nit: line break Done. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:47: if (result == net::OK) { On 2013/12/17 00:37:40, Ryan Sleevi wrote: > Generally speaking, it's better to detect and handle errors first, and return > (if appropriate), and then let the 'success' case dominate this function. Done. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:58: // Failed TLS connection. This'll delete the underlying TCPClientSocket. On 2013/12/17 00:37:40, Ryan Sleevi wrote: > comment nit: This'll -> This will Done. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:63: } // namespace On 2013/12/17 00:37:40, Ryan Sleevi wrote: > style nit: Line break Done. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:71: const std::string& owner_extension_id) On 2013/12/17 00:37:40, Ryan Sleevi wrote: > style nit: unnecessary extra space Done. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:127: read_callback_.Reset(); On 2013/12/17 00:37:40, Ryan Sleevi wrote: > STYLE: simplify this as > > base::ResetAndReturn(&read_callback_).Run(result, io_buffer); Done. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:138: return tls_socket_->Write(io_buffer, io_buffer_size, callback); On 2013/12/17 00:37:40, Ryan Sleevi wrote: > style: indent Done. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:189: || tcp_socket->ClientStream() == NULL || !socket->IsConnected()) { On 2013/12/17 00:37:40, Ryan Sleevi wrote: > style: inproper binary operator wrapping - > http://www.chromium.org/developers/coding-style#Code_Formatting Done. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:195: socket->GetPeerAddress(&dest_host_port_pair); On 2013/12/17 00:37:40, Ryan Sleevi wrote: > BUG: GetPeerAddress returns a result. Check it. Done. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:213: url_request_getter->Release(); On 2013/12/17 00:37:40, Ryan Sleevi wrote: > BUG: ?!? Seems incredibly dangerous here. You should be using a scoped_refptr<> > and shouldn't be calling Release yourself. It is especially dangerous for > lifetimes and such to be releasing it so explicitly. I moved lifetime responsibility to the caller. Now the callbacks do this. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:244: socket_handle.Pass(), host_port, ssl_config, context).release(); On 2013/12/17 00:37:40, Ryan Sleevi wrote: > DESIGN: Make your ownership explicit here. > > For example, just reading this code (242 - 259), it's impossible to reason when > |ssl_socket| gets deleted (FWIW, it requires digging into TlsConnectDone to see > there is a pseudo-ownership transfer) Done. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket.h (right): https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.h:26: // behavioral leakage from the underlying implementation. On 2013/12/17 00:37:40, Ryan Sleevi wrote: > style nit: single space after a period, not double. Applies throughout. > > style nit: Pronouns in comments considered harmful - > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3... > > For what it's worth, I'm still confused on that last sentence. Fixed, with a re-written last sentence. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.h:40: const CompletionCallback& callback) OVERRIDE; On 2013/12/17 00:37:40, Ryan Sleevi wrote: > This still heavily rankles my design sentiments, because this clearly violates > the is-a inheritance model being suggested here. > > I'd like to be able to find a solution that doesn't require this. If we can't, > we've got a design issue with the sockets API in general. Same for every other > "Fails" below. I don't know how to get around this issue without rewriting the sockets API implementation. The Fails/Forwards comments are part of an attempt to at least document the impact of that design decision as it impacts TLSSocket. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.h:83: SecureCallback callback); On 2013/12/17 00:37:40, Ryan Sleevi wrote: > style nit: four space indent > > comment nit: Explain the lifetime requirements for |profile| > > comment/style nit: Is ownership of |socket| transferred? If so, it should be > scoped_ptr<Socket> and use our C++11 emulation of scoped_ptr<T>::Pass() > > style nit: "SecureCallback" -> "const SecureCallback&" > > naming nit: SecureTCPSocket is arguably ambiguous with respect to "Is it a verb > - eg: secure this TCP socket - or a noun - eg: this is a constructor for a > secure TCP socket". Perhaps "UpgradeSocketToSecureSocket" or something equally > descriptive and unambiguously verb-ified. I chose UpgradeSocketToTLS, to avoid having Socket in there twice. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.h:87: const net::CompletionCallback& callback) OVERRIDE; On 2013/12/17 00:37:40, Ryan Sleevi wrote: > style: line breaks - > http://www.chromium.org/developers/coding-style#Code_Formatting Done. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.h:88: void OnReadComplete(scoped_refptr<net::IOBuffer> io_buffer, int result); On 2013/12/17 00:37:40, Ryan Sleevi wrote: > style nit: "scoped_refptr<net::IOBuffer>" -> "const scoped_refptr<IOBuffer>&" Done. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... File chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc (right): https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc:462: int result = net::ERR_INVALID_ARGUMENT; On 2013/12/17 00:37:40, Ryan Sleevi wrote: > style: Previous comments about using |reuslt| here Done. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc:480: || socket->ClientStream() == NULL) { On 2013/12/17 00:37:40, Ryan Sleevi wrote: > style: Conditionals on preceding line. Done. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... File chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.h (right): https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.h:247: protected: On 2013/12/17 00:37:40, Ryan Sleevi wrote: > STYLE: Follow the local style (and dominant Chromium style) by including a line > break between public / protected / private > > See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_... Done. https://codereview.chromium.org/76403004/diff/420001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.h:253: scoped_ptr<api::socket::Secure::Params> params_; On 2013/12/17 00:37:40, Ryan Sleevi wrote: > style: Line break Done.
Comments just on the diffs from patchset 10. I feel like I should push back on this and go for a bit more "fixing the sockets API to support this", since I'm concerned that if I don't, this code won't really be touched. I realize that's asking a fair bit, but I don't think we should push for this until the underlying implementation can more cleanly (eg: without the inheritance that isn't) support it. I'd like to get rpaquay's take on it. There's also some type oddities that bother me a bit (eg: std::string for IP address, which can lead to API ambiguity, as we've seen). https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket_api.cc (right): https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:927: Socket* socket(GetSocket(params_->socket_id)); Your original form was fine / equivalent. I tend to find this form a bit ambiguous for scalar types / initializers. https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:962: // socket can only be non-null if result == net::OK. nit: socket -> |socket|, result -> |result| https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:966: SetSocket(params_->socket_id, socket); What's the contents of |error_| at this point? Is it guaranteed? Should it be explicitly set to some "no error" state? https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket_api.h (right): https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.h:538: DISALLOW_COPY_AND_ASSIGN(SocketSecureFunction); nit: linebreak before https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket.cc (right): https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:110: else style: delete the else here. ("Don't use else after return") - http://www.chromium.org/developers/coding-style#Code_Formatting https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:164: tcp_socket->ClientStream() == NULL || !tcp_socket->IsConnected()) { style: A bit odd to see !tcp_socket but then "tcp_socket->ClientStream() == NULL" Just rewrite if !tcp_socket->ClientStream() ? https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:188: // only one active here). Then have the old socket release ownership on of nit: extra space here ;) https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:253: void TLSSocket::TlsConnectDone(net::SSLClientSocket* ssl_socket, use scoped_ptr<net::SSLClientSocket> to indicate the ownership is passed here void TLSSocket::TlsConnectDone(scoped_ptr<net::SSLClientSocket> ssl_socket, ...) { if (result != net::OK) { // Is it necessary to explicitly delete ssl_socket here? callback.Run(NULL, result); } scoped_ptr<TLSSocket> wrapper(new TLSSocket(ssl_socket.Pass(), extension_id)); callback.Run(wrapper.Pass(), result); } https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket.h (right): https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.h:20: typedef base::Callback<void(TLSSocket*, int)> SecureCallback; Does this make more sense as "scoped_ptr<TLSSocket>" / "const scoped_refptr<TLSSocket>&" (depending on whether it's refcounted), to indicate it's an ownership transferring/sharing operation? https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.h:78: // is invoked. |callback| is always invoked. Note: |callback| may get nit: unnecessary extra space ;) "Note: |callback| may be invoked synchronously prior to return" ? https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... File chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.h (right): https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.h:259: DISALLOW_COPY_AND_ASSIGN(SocketsTcpSecureFunction); nit: linebreak before
On 2014/01/14 01:32:06, Ryan Sleevi wrote: > Comments just on the diffs from patchset 10. > > I feel like I should push back on this and go for a bit more "fixing the sockets > API to support this", since I'm concerned that if I don't, this code won't > really be touched. > > I realize that's asking a fair bit, but I don't think we should push for this > until the underlying implementation can more cleanly (eg: without the > inheritance that isn't) support it. > > I'd like to get rpaquay's take on it. There's also some type oddities that > bother me a bit (eg: std::string for IP address, which can lead to API > ambiguity, as we've seen). > > https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... > File chrome/browser/extensions/api/socket/socket_api.cc (right): > > https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... > chrome/browser/extensions/api/socket/socket_api.cc:927: Socket* > socket(GetSocket(params_->socket_id)); > Your original form was fine / equivalent. I tend to find this form a bit > ambiguous for scalar types / initializers. > > https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... > chrome/browser/extensions/api/socket/socket_api.cc:962: // socket can only be > non-null if result == net::OK. > nit: socket -> |socket|, result -> |result| > > https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... > chrome/browser/extensions/api/socket/socket_api.cc:966: > SetSocket(params_->socket_id, socket); > What's the contents of |error_| at this point? Is it guaranteed? Should it be > explicitly set to some "no error" state? > > https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... > File chrome/browser/extensions/api/socket/socket_api.h (right): > > https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... > chrome/browser/extensions/api/socket/socket_api.h:538: > DISALLOW_COPY_AND_ASSIGN(SocketSecureFunction); > nit: linebreak before > > https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... > File chrome/browser/extensions/api/socket/tls_socket.cc (right): > > https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... > chrome/browser/extensions/api/socket/tls_socket.cc:110: else > style: delete the else here. > > ("Don't use else after return") - > http://www.chromium.org/developers/coding-style#Code_Formatting > > https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... > chrome/browser/extensions/api/socket/tls_socket.cc:164: > tcp_socket->ClientStream() == NULL || !tcp_socket->IsConnected()) { > style: A bit odd to see !tcp_socket but then "tcp_socket->ClientStream() == > NULL" > > Just rewrite if !tcp_socket->ClientStream() ? > > https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... > chrome/browser/extensions/api/socket/tls_socket.cc:188: // only one active > here). Then have the old socket release ownership on of > nit: extra space here ;) > > https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... > chrome/browser/extensions/api/socket/tls_socket.cc:253: void > TLSSocket::TlsConnectDone(net::SSLClientSocket* ssl_socket, > use scoped_ptr<net::SSLClientSocket> to indicate the ownership is passed here > > void TLSSocket::TlsConnectDone(scoped_ptr<net::SSLClientSocket> ssl_socket, > ...) { > if (result != net::OK) { > // Is it necessary to explicitly delete ssl_socket here? > callback.Run(NULL, result); > } > > scoped_ptr<TLSSocket> wrapper(new TLSSocket(ssl_socket.Pass(), extension_id)); > callback.Run(wrapper.Pass(), result); > } > > https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... > File chrome/browser/extensions/api/socket/tls_socket.h (right): > > https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... > chrome/browser/extensions/api/socket/tls_socket.h:20: typedef > base::Callback<void(TLSSocket*, int)> SecureCallback; > Does this make more sense as "scoped_ptr<TLSSocket>" / "const > scoped_refptr<TLSSocket>&" (depending on whether it's refcounted), to indicate > it's an ownership transferring/sharing operation? > > https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... > chrome/browser/extensions/api/socket/tls_socket.h:78: // is invoked. |callback| > is always invoked. Note: |callback| may get > nit: unnecessary extra space ;) > > "Note: |callback| may be invoked synchronously prior to return" ? > > https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... > File chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.h (right): > > https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... > chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.h:259: > DISALLOW_COPY_AND_ASSIGN(SocketsTcpSecureFunction); > nit: linebreak before Also, can you update the description with the bug # tracking this?
I've added an integration test (piggybacking on an existing test that lived in a test chrome extension), and addressed most of Ryan's comments. PTAL, thanks. https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket_api.cc (right): https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:927: Socket* socket(GetSocket(params_->socket_id)); On 2014/01/14 01:32:06, Ryan Sleevi wrote: > Your original form was fine / equivalent. I tend to find this form a bit > ambiguous for scalar types / initializers. Done. https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:962: // socket can only be non-null if result == net::OK. On 2014/01/14 01:32:06, Ryan Sleevi wrote: > nit: socket -> |socket|, result -> |result| Done. https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:966: SetSocket(params_->socket_id, socket); On 2014/01/14 01:32:06, Ryan Sleevi wrote: > What's the contents of |error_| at this point? Is it guaranteed? Should it be > explicitly set to some "no error" state? Default constructed to the empty string. I can add a superfluous clear() if that helps. https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket_api.h (right): https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.h:538: DISALLOW_COPY_AND_ASSIGN(SocketSecureFunction); On 2014/01/14 01:32:06, Ryan Sleevi wrote: > nit: linebreak before Done. https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket.cc (right): https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:110: else On 2014/01/14 01:32:06, Ryan Sleevi wrote: > style: delete the else here. > > ("Don't use else after return") - > http://www.chromium.org/developers/coding-style#Code_Formatting Done. https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:164: tcp_socket->ClientStream() == NULL || !tcp_socket->IsConnected()) { On 2014/01/14 01:32:06, Ryan Sleevi wrote: > style: A bit odd to see !tcp_socket but then "tcp_socket->ClientStream() == > NULL" > > Just rewrite if !tcp_socket->ClientStream() ? Done. https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:188: // only one active here). Then have the old socket release ownership on of On 2014/01/14 01:32:06, Ryan Sleevi wrote: > nit: extra space here ;) Done. https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:253: void TLSSocket::TlsConnectDone(net::SSLClientSocket* ssl_socket, I tried that, and ended up punting. The issue is in the base::Bind() object above. Connect() may not invoke the callback it's given, in the ERR_IO_PENDING case. That would delete the socket before the branch below it could call TlsConnectDone. If you've got an idea to get around this, I'm all ears. https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket.h (right): https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.h:20: typedef base::Callback<void(TLSSocket*, int)> SecureCallback; On 2014/01/14 01:32:06, Ryan Sleevi wrote: > Does this make more sense as "scoped_ptr<TLSSocket>" / "const > scoped_refptr<TLSSocket>&" (depending on whether it's refcounted), to indicate > it's an ownership transferring/sharing operation? Yes, yes it does. Done. https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.h:78: // is invoked. |callback| is always invoked. Note: |callback| may get On 2014/01/14 01:32:06, Ryan Sleevi wrote: > nit: unnecessary extra space ;) > > "Note: |callback| may be invoked synchronously prior to return" ? Note: |callback| may be invoked, synchronously, before UpgradeSocketToTLS returns. https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... File chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.h (right): https://codereview.chromium.org/76403004/diff/460001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.h:259: DISALLOW_COPY_AND_ASSIGN(SocketsTcpSecureFunction); On 2014/01/14 01:32:06, Ryan Sleevi wrote: > nit: linebreak before Done.
lgtm assuming Ryan has no further comments.
Still some pretty serious threading issues, AFAICT, and a few SECURITY-related questions duplicated between both. Also, I'd like to see the TCP and SSL tests split, to test minimal functionality. rpaquay will likely need to re-review https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... File chrome/browser/extensions/api/api_resource_manager.h (right): https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/api_resource_manager.h:230: } Don't use else after return http://www.chromium.org/developers/coding-style#Code_Formatting from "} else { return false; }" to "} return false;" https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket_api.cc (right): https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:952: Profile* profile = GetProfile(); I'm quite surprised this is safe. GetProfile() was only safe on the UI thread, I thought? At the time this task is run, isn't it possible for the profile to be torn down? That's why we have the URLRequestContextGetter interface https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:974: url_request_getter_->Release(); Why are you manually releasing here? Won't this cause a double-free on the dtor of SocketSecureFunction, which will ~scoped_refptr<> and Release the ref that line 918 added? Don't you mean url_request_getter_.reset(); or assigning it = NULL? https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket_api.h (right): https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.h:537: net::URLRequestContextGetter* url_request_getter_; use scoped_refptr<> when passing between threads. Otherwise, this is unsafe, because you're not holding "ownership" on the returned object. https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket.cc (right): https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:24: // chrome/browser/net/ssl_config_service_manager_pref.cc. Delete this last line. The fact that it overlaps with prefs is irrelevant - this is the API contract you're making, this is your code now, and it shouldn't be shared (because it's part of a public API contract) https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:89: scoped_refptr<net::IOBuffer> io_buffer = new net::IOBuffer(count); nit: io_buffer(new net::IOBuffer(count)); https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:182: // Modeled after P2PSocketHostTcpBase::StartTls() Delete comment. https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:204: profile->GetSSLConfigService()->GetSSLConfig(&ssl_config); SECURITY: I don't think it's safe to access |profile| here (on the IO thread - see line 157/158) https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:238: if (status != net::ERR_IO_PENDING) { what about status == OK? https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:248: } You can still use the scoped_ptr<> to accomplish what you want base::Callback<void(int)> cb(base::Bind(&TLSSocket::TlsConnectDone, base::Passed(ssl_socket), extension_id, callback))); int status = ssl_socket->Connect(cb); if (status != net::ERR_IO_PENDING) cb.Run(status); } The above *should* work just fine, because base::Callback uses scoped_refptr<> under the hood. https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:260: // no longer bound to the original TCPSocket. It belongs to ssl_socket_, |ssl_socket| ? https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... File chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc (right): https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc:494: Profile* profile = GetProfile(); SECURITY: I don't think it's safe to do this from the IO thread https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc:498: socket, profile, url_request_getter_, extension_id(), SECURITY: url_request_getter_ may be invalid by the time this runs on the IO thread, because you didn't take ownership. https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc:505: scoped_ptr<TLSSocket> socket, int result) { STYLE: one parameter per line see http://www.chromium.org/developers/coding-style#Code_Formatting Note: You can run "git cl format" to clean this up, although I recommend doing that in a *separate* patchset - that is, after addressing all other substantive issues - so that the diffs are not too unwieldy https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc:519: url_request_getter_->Release(); SECURITY: manual memory management? https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... File chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.h (right): https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.h:259: net::URLRequestContextGetter* url_request_getter_; scoped_refptr<> https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... File chrome/browser/extensions/api/sockets_tcp/sockets_tcp_apitest.cc (right): https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_apitest.cc:85: IN_PROC_BROWSER_TEST_F(SocketsTcpApiTest, SocketTcpExtension) { Rather than modifying the existing test, you should be adding an additional test modelled after it. That is, test the Socket TCP extension, then test the Socket TCP + SSL extension.
Thanks, I've just uploaded changes. The test-case code on the javascript was already split up (dispatched from a single driver), so I just added the original TCP-only test back in, atop the new TCP+TLS test. Other than that, I think I've got the ownership now all automatic and well-specified via shared_(ref)ptr<>s. Please let me know if there's anything else. https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... File chrome/browser/extensions/api/api_resource_manager.h (right): https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/api_resource_manager.h:230: } On 2014/02/04 22:28:14, Ryan Sleevi wrote: > Don't use else after return > > http://www.chromium.org/developers/coding-style#Code_Formatting > > from "} else { > return false; > }" > to > "} > return false;" Done. https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket_api.cc (right): https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:952: Profile* profile = GetProfile(); On 2014/02/04 22:28:14, Ryan Sleevi wrote: > I'm quite surprised this is safe. GetProfile() was only safe on the UI thread, I > thought? > > At the time this task is run, isn't it possible for the profile to be torn down? > That's why we have the URLRequestContextGetter interface This isn't the normal GetProfile(). It's implemented in https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... -> https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro... as a simple static_cast<>. The extensions API mechanism already depends on GetProfile()'s return value living as long as this API call -- the mechanism that holds the socket IDs, in manager_, is a BrowserContextKeyedService. It's held by manager_, which holds that service in manager_->manager_. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... -> https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... -> https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... -> https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... -> https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.cc:974: url_request_getter_->Release(); On 2014/02/04 22:28:14, Ryan Sleevi wrote: > Why are you manually releasing here? Won't this cause a double-free on the dtor > of SocketSecureFunction, which will ~scoped_refptr<> and Release the ref that > line 918 added? > > Don't you mean url_request_getter_.reset(); or assigning it = NULL? Changed. Now passing ownership to UpgradeSocketToTLS(). https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket_api.h (right): https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket_api.h:537: net::URLRequestContextGetter* url_request_getter_; On 2014/02/04 22:28:14, Ryan Sleevi wrote: > use scoped_refptr<> when passing between threads. Otherwise, this is unsafe, > because you're not holding "ownership" on the returned object. Done. https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket.cc (right): https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:24: // chrome/browser/net/ssl_config_service_manager_pref.cc. On 2014/02/04 22:28:14, Ryan Sleevi wrote: > Delete this last line. The fact that it overlaps with prefs is irrelevant - this > is the API contract you're making, this is your code now, and it shouldn't be > shared (because it's part of a public API contract) Done. https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:89: scoped_refptr<net::IOBuffer> io_buffer = new net::IOBuffer(count); On 2014/02/04 22:28:14, Ryan Sleevi wrote: > nit: io_buffer(new net::IOBuffer(count)); Done. https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:182: // Modeled after P2PSocketHostTcpBase::StartTls() On 2014/02/04 22:28:14, Ryan Sleevi wrote: > Delete comment. Done. https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:204: profile->GetSSLConfigService()->GetSSLConfig(&ssl_config); On 2014/02/04 22:28:14, Ryan Sleevi wrote: > SECURITY: I don't think it's safe to access |profile| here (on the IO thread - > see line 157/158) I've removed the access to that, and instead made a call to SSLConfigService::GetSSLConfig(), which should only be called on IO (https://code.google.com/p/chromium/codesearch#chromium/src/net/ssl/ssl_config... ). https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:238: if (status != net::ERR_IO_PENDING) { On 2014/02/04 22:28:14, Ryan Sleevi wrote: > what about status == OK? Sure, a new implementation of that interface could just do it all and return OK. Added in. https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:248: } On 2014/02/04 22:28:14, Ryan Sleevi wrote: > You can still use the scoped_ptr<> to accomplish what you want > > base::Callback<void(int)> cb(base::Bind(&TLSSocket::TlsConnectDone, > base::Passed(ssl_socket), extension_id, callback))); > int status = ssl_socket->Connect(cb); > if (status != net::ERR_IO_PENDING) > cb.Run(status); > } > > The above *should* work just fine, because base::Callback uses scoped_refptr<> > under the hood. Great!, thanks. https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:260: // no longer bound to the original TCPSocket. It belongs to ssl_socket_, On 2014/02/04 22:28:14, Ryan Sleevi wrote: > |ssl_socket| ? Yikes, missed this one on the last cl upload. Changed locally, will upload. https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... File chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc (right): https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc:494: Profile* profile = GetProfile(); On 2014/02/04 22:28:14, Ryan Sleevi wrote: > SECURITY: I don't think it's safe to do this from the IO thread This isn't the normal GetProfile() api. It actually resolves to a static_cast<> (!). https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc:498: socket, profile, url_request_getter_, extension_id(), On 2014/02/04 22:28:14, Ryan Sleevi wrote: > SECURITY: url_request_getter_ may be invalid by the time this runs on the IO > thread, because you didn't take ownership. now in a scoped_refptr<>. https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc:505: scoped_ptr<TLSSocket> socket, int result) { On 2014/02/04 22:28:14, Ryan Sleevi wrote: > STYLE: one parameter per line > > see http://www.chromium.org/developers/coding-style#Code_Formatting > > Note: You can run "git cl format" to clean this up, although I recommend doing > that in a *separate* patchset - that is, after addressing all other substantive > issues - so that the diffs are not too unwieldy > Thanks for the pointer. I fixed this one. A general formatting cleanup patchset sounds pretty good. https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc:519: url_request_getter_->Release(); On 2014/02/04 22:28:14, Ryan Sleevi wrote: > SECURITY: manual memory management? Fixed. https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... File chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.h (right): https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.h:259: net::URLRequestContextGetter* url_request_getter_; On 2014/02/04 22:28:14, Ryan Sleevi wrote: > scoped_refptr<> Done. https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... File chrome/browser/extensions/api/sockets_tcp/sockets_tcp_apitest.cc (right): https://codereview.chromium.org/76403004/diff/680001/chrome/browser/extension... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_apitest.cc:85: IN_PROC_BROWSER_TEST_F(SocketsTcpApiTest, SocketTcpExtension) { On 2014/02/04 22:28:14, Ryan Sleevi wrote: > Rather than modifying the existing test, you should be adding an additional test > modelled after it. > > That is, test the Socket TCP extension, then test the Socket TCP + SSL > extension. Done.
Thanks for your continued patience on this lally. Another round of feedback after approaching this with fresh eyes and paging in all this information. We're close, very close - I think we're largely in the cleanup phase. https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... File chrome/browser/extensions/api/api_resource_manager.h (right): https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... chrome/browser/extensions/api/api_resource_manager.h:221: bool Set(const std::string& extension_id, I know this is not your code, and so the lack of documentation here is not a bug you introduced. However, I have a bit of trouble reasoning why Set() will only let you set an api_resource_id iff there exists a previous resource with that same api_resource_id. If this is meant to be a compliment to ApiResourceData::Add(), then I believe you should document both this method and on 136, to explain the correct usage. Namely, that it exists to _replace_ an existing resource. I'm going to have to defer to the extension OWNERs on the overall pattern, but it does have the back hairs on my neck tingling that there may be an opportunity for "confused deputy" attacks - in which a caller still has a pointer to an older resource ID, and it wants to perform some action, and some security check is made against the new object (eg: Returned by Get()). This is primarily because of the linked_ptr<> pattern. https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket.h (right): https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket.h:52: // TLS (see TLSSocket::UpgradeSocketToTLS()). Comment wise, this wording seems a bit on the wishy-washy side, in that it seems more of a suggestion than clear guidance. Let's try this: // Note: |address| contains the resolved IP address, not the hostname // of the remote endpoint. In order to upgrade this socket to TLS, // callers must also supply the hostname of the endpoint via // |set_hostname()|. https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket.h:111: // verifying a TLS certificate. // The hostname of the remote host that this socket is connected to. // Note: This may contain an IP literal. In the case of IDNs, this // will contain a series of A-LABELs, not U-LABELs. Am I correct with that above comment? How do you handle IDN names? If you pass them through, should this be a UTF-8 string? Or will you apply IDNA to canonicalize from the JS? And is it valid for callers to supply IP addresses, for IP address certs? If so, what's the form of IPv6 - bracketed or unbracketed? (Presumably, the latter). https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tcp_socket.cc (right): https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... chrome/browser/extensions/api/socket/tcp_socket.cc:311: VLOG(1) << "TCPSocket::Release getting called in server mode."; Question: Is this a (C++/Chromium) developer error if this happens, or is this code-path reachable/inducible by the JS path? I feel like we should be D/CHECKing if the only possible path if through Chromium developer, but if it's from JS, then I agree this is the right error. Same comment applies to line 324. https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket_unittest.cc (right): https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket_unittest.cc:108: TEST(SocketTest, TestTLSSocketRead) { Can you add comments to each of these (eg: on line 107 / 125) that explains what you're testing? This is particularly important for the BlockedWrite/BlockedWriteReentry, as the test is not entirely clear. https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket_unittest.cc:155: new TLSSocket(ssl_socket, tcp_client_socket, FAKE_ID)); Given the redundancy of this setup logic, does it make more sense to use a TEST_F fixture instead? https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket_unittest.cc:177: CompleteHandler handlers[5]; magic numbers: What exists to guarantee that this line, line 185, line 187, and line 199 are kept in sync? Seems like you can use a static const int kSomethingOrOther = 5; here within the function. And/or update lines 187/200 to use ARRAY_SIZE or the like. And it's not clear why line 183 is .Times(5), when the other tests are .Times(2) for a single test (eg: 158/152, 136/130)
Thank you for your review. It's important to get this right, so it's worth the time and energy. Thanks for your patience with me :-) If there's anything I can do, please don't hesitate to say so. Thanks! - Lally https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... File chrome/browser/extensions/api/api_resource_manager.h (right): https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... chrome/browser/extensions/api/api_resource_manager.h:221: bool Set(const std::string& extension_id, On 2014/02/24 20:06:49, Ryan Sleevi wrote: > I know this is not your code, and so the lack of documentation here is not a bug > you introduced. > > However, I have a bit of trouble reasoning why Set() will only let you set an > api_resource_id iff there exists a previous resource with that same > api_resource_id. > > If this is meant to be a compliment to ApiResourceData::Add(), then I believe > you should document both this method and on 136, to explain the correct usage. > > Namely, that it exists to _replace_ an existing resource. > > I'm going to have to defer to the extension OWNERs on the overall pattern, but > it does have the back hairs on my neck tingling that there may be an opportunity > for "confused deputy" attacks - in which a caller still has a pointer to an > older resource ID, and it wants to perform some action, and some security check > is made against the new object (eg: Returned by Get()). This is primarily > because of the linked_ptr<> pattern. I've renamed it Replace(), and documented that it requires that the resource ID has to already exist and be owned by extension_id. I *think* that different extensions are firewalled by the different extension_ids, which are outside the control of the relevant extensions -- does that help your concern about confused deputies? https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket.h (right): https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket.h:52: // TLS (see TLSSocket::UpgradeSocketToTLS()). On 2014/02/24 20:06:49, Ryan Sleevi wrote: > Comment wise, this wording seems a bit on the wishy-washy side, in that it seems > more of a suggestion than clear guidance. Let's try this: > > // Note: |address| contains the resolved IP address, not the hostname > // of the remote endpoint. In order to upgrade this socket to TLS, > // callers must also supply the hostname of the endpoint via > // |set_hostname()|. Done. https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket.h:111: // verifying a TLS certificate. On 2014/02/24 20:06:49, Ryan Sleevi wrote: > // The hostname of the remote host that this socket is connected to. > // Note: This may contain an IP literal. In the case of IDNs, this > // will contain a series of A-LABELs, not U-LABELs. > > Am I correct with that above comment? How do you handle IDN names? If you pass > them through, should this be a UTF-8 string? Or will you apply IDNA to > canonicalize from the JS? > I had to investigate to find out what the upper-layer behavior is. This passes through the UTF-8 string passed in from javascript. It's passed directly from the javascript API as just a hostname, so there won't be any parsing, and there shouldn't be any brackets for IPv6 addresses. Should I canonicalize? > And is it valid for callers to supply IP addresses, for IP address certs? If so, > what's the form of IPv6 - bracketed or unbracketed? (Presumably, the latter). https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tcp_socket.cc (right): https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... chrome/browser/extensions/api/socket/tcp_socket.cc:311: VLOG(1) << "TCPSocket::Release getting called in server mode."; On 2014/02/24 20:06:49, Ryan Sleevi wrote: > Question: Is this a (C++/Chromium) developer error if this happens, or is this > code-path reachable/inducible by the JS path? > > I feel like we should be D/CHECKing if the only possible path if through > Chromium developer, but if it's from JS, then I agree this is the right error. > > Same comment applies to line 324. Good question. The only call site is within TLSSocket::UpgradeSocketToTLS, and we only currently support client-mode sockets for upgrading. A socket that isn't client mode will invoke an early return before the call to Release(), so there isn't a way to get here from JS. I'm happy to put in a DCHECK. https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket_unittest.cc (right): https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket_unittest.cc:108: TEST(SocketTest, TestTLSSocketRead) { On 2014/02/24 20:06:49, Ryan Sleevi wrote: > Can you add comments to each of these (eg: on line 107 / 125) that explains what > you're testing? > > This is particularly important for the BlockedWrite/BlockedWriteReentry, as the > test is not entirely clear. Done. https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket_unittest.cc:155: new TLSSocket(ssl_socket, tcp_client_socket, FAKE_ID)); On 2014/02/24 20:06:49, Ryan Sleevi wrote: > Given the redundancy of this setup logic, does it make more sense to use a > TEST_F fixture instead? Good point. Done. https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket_unittest.cc:177: CompleteHandler handlers[5]; On 2014/02/24 20:06:49, Ryan Sleevi wrote: > magic numbers: What exists to guarantee that this line, line 185, line 187, and > line 199 are kept in sync? > > Seems like you can use a static const int kSomethingOrOther = 5; here within the > function. > > And/or update lines 187/200 to use ARRAY_SIZE or the like. > > And it's not clear why line 183 is .Times(5), when the other tests are .Times(2) > for a single test (eg: 158/152, 136/130) Documentation added, and the 5 has been replaced with a constant.
Quick update after talking to Renaud. https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... File chrome/browser/extensions/api/api_resource_manager.h (right): https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... chrome/browser/extensions/api/api_resource_manager.h:221: bool Set(const std::string& extension_id, On 2014/02/27 17:05:59, lally wrote: > On 2014/02/24 20:06:49, Ryan Sleevi wrote: > > I know this is not your code, and so the lack of documentation here is not a > bug > > you introduced. > > > > However, I have a bit of trouble reasoning why Set() will only let you set an > > api_resource_id iff there exists a previous resource with that same > > api_resource_id. > > > > If this is meant to be a compliment to ApiResourceData::Add(), then I believe > > you should document both this method and on 136, to explain the correct usage. > > > > Namely, that it exists to _replace_ an existing resource. > > > > I'm going to have to defer to the extension OWNERs on the overall pattern, but > > it does have the back hairs on my neck tingling that there may be an > opportunity > > for "confused deputy" attacks - in which a caller still has a pointer to an > > older resource ID, and it wants to perform some action, and some security > check > > is made against the new object (eg: Returned by Get()). This is primarily > > because of the linked_ptr<> pattern. > > I've renamed it Replace(), and documented that it requires that the resource ID > has to already exist and be owned by extension_id. I *think* that different > extensions are firewalled by the different extension_ids, which are outside the > control of the relevant extensions -- does that help your concern about confused > deputies? I just checked with Renaud, yup, the extension_ids keep the extensions' state separate from one another.
On 2014/02/27 21:43:18, lally wrote: > Quick update after talking to Renaud. > > https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... > File chrome/browser/extensions/api/api_resource_manager.h (right): > > https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... > chrome/browser/extensions/api/api_resource_manager.h:221: bool Set(const > std::string& extension_id, > On 2014/02/27 17:05:59, lally wrote: > > On 2014/02/24 20:06:49, Ryan Sleevi wrote: > > > I know this is not your code, and so the lack of documentation here is not a > > bug > > > you introduced. > > > > > > However, I have a bit of trouble reasoning why Set() will only let you set > an > > > api_resource_id iff there exists a previous resource with that same > > > api_resource_id. > > > > > > If this is meant to be a compliment to ApiResourceData::Add(), then I > believe > > > you should document both this method and on 136, to explain the correct > usage. > > > > > > Namely, that it exists to _replace_ an existing resource. > > > > > > I'm going to have to defer to the extension OWNERs on the overall pattern, > but > > > it does have the back hairs on my neck tingling that there may be an > > opportunity > > > for "confused deputy" attacks - in which a caller still has a pointer to an > > > older resource ID, and it wants to perform some action, and some security > > check > > > is made against the new object (eg: Returned by Get()). This is primarily > > > because of the linked_ptr<> pattern. > > > > I've renamed it Replace(), and documented that it requires that the resource > ID > > has to already exist and be owned by extension_id. I *think* that different > > extensions are firewalled by the different extension_ids, which are outside > the > > control of the relevant extensions -- does that help your concern about > confused > > deputies? > > I just checked with Renaud, yup, the extension_ids keep the extensions' state > separate from one another. So, that wasn't exactly what I was concerned about. Assume a mapping of Extension ID A and resource_id 1, which maps to Foo* foo. Your new Set/Replace method introduces a means to associate a new object - Bar* bar (where Bar is-a Foo) with A-1 The confused deputy issue I'm talking about is for the following call sequence: Foo* local_foo = Get(A,1); Replace(A, 1, bar); local_foo->DoSomething(); // Use-after-free! You can also apply it to the confused deputy problem of if (Get(A,1)->IsAllowedToDoStep1()) { // Do step 1 PostTask(base::Bind(DoStep2, A, 1)); } void DoStep2(extension, resource) { if (Get(extension, resource)->IsAllowedToDoStep2()) { // Do step 2 } } Now, imagine that before DoStep2 is posted as a task, some other task is posted first. That task does something like Replace(A, 1, bar); You now have a confusion where "IsAllowedToDoStep1" was checked on Foo, but IsAllowedToDoStep2() is checked on Bar. You can logically extend this to any shared state - not just permission grants - but it hopefully demonstrates where the concern is coming from. I'm not concerned about extensions A and B "sharing" the same resource with resource_id of 1. I AM worried about extension A getting confused as to the underlying object it's talking to when it asks for "Resource 1". That sort of reasoning is much harder to do in this code review, and requires a holistic understanding of all the points of entry into the system. Which, you know, scares me a little. That's why I asked Renaud to take a look at this overall pattern and re-review it to make sure that the entry points are robust against this type of confusion or deferral.
That's a nice attack! I don't believe we're susceptible to it.
Renaud, am I correct in my analysis of the code?
You can only replace a socket ID with a secure()'d version of that socket.
The only callers to ReplaceSocket() are the implementations of
secure()[1]. In both cases (the old socket API and the new sockets_tcp
api), they replace the socked ID mapping to a TLSSocket, which wraps the
original socket. The underlying socket must have already passed the access
checks for the extension. If it hadn't, that socket would fail when trying
to connect, and secure() would error out without touching any socket state.
A socket can only be secure()'d once. When the secure process begins, the
socket's resources are taken into custody of the API object
(SocketSecureFunction), and the TCPSocket object mapped to the ID is in a
state that looks disconnected to the outside world -- all its internal
net::*Socket members are empty. Further calls to secure() will immediately
fail, treating the socket as disconnected.
Additionally, the socked IDs aren't reused. There isn't any attempt to
keep the IDs compact: the generator for new IDs is simply an increment [2].
There's no equivalent of dup2(2) in the sockets API.
[1] In light of this attack, a warning should be put around the declaration
of ReplaceSocket() to keep later APIs from accidentally using it in such a
way that this attack is possible.
[2] Creating a new socket in
SocketCreateFunction::Work()<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/extensions/api/socket/socket_api.cc&rcl=1393803615&l=157>
and
SocketsTcpCreateFunction::Work()<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/extensions/api/sockets_tcp/sockets_tcp_api.cc&l=118>
both
use
SocketAsyncApiFunction::AddSocket()<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/extensions/api/socket/socket_api.cc&rcl=1393803615&l=71>
to
add the socket. It calls
SocketResourceManager<T>::Add()<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/extensions/api/socket/socket_api.h&rcl=1393803615&l=70>,
which then just calls
ApiResourceManager::Add()<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/extensions/api/api_resource_manager.h&rcl=1393803615&l=119>,
which calls
ApiResourceData::Add()<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/extensions/api/api_resource_manager.h&rcl=1393803615&l=182>,
which allocates a new identifier by calling
ApiResourceData::GenerateId()<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/extensions/api/api_resource_manager.h&rcl=1393803615&l=316>,
whose body is:
int GenerateId
<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
{ return next_id_
<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...;
}
next_id_'s declaration is: int next_id_
<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...>;
Cheers,
$ ls
Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering |
lally@google.com
On Thu, Feb 27, 2014 at 7:09 PM, <rsleevi@chromium.org> wrote:
> On 2014/02/27 21:43:18, lally wrote:
>
>> Quick update after talking to Renaud.
>>
>
>
> https://codereview.chromium.org/76403004/diff/850001/
> chrome/browser/extensions/api/api_resource_manager.h
>
>> File chrome/browser/extensions/api/api_resource_manager.h (right):
>>
>
>
> https://codereview.chromium.org/76403004/diff/850001/
> chrome/browser/extensions/api/api_resource_manager.h#newcode221
>
>> chrome/browser/extensions/api/api_resource_manager.h:221: bool Set(const
>> std::string& extension_id,
>> On 2014/02/27 17:05:59, lally wrote:
>> > On 2014/02/24 20:06:49, Ryan Sleevi wrote:
>> > > I know this is not your code, and so the lack of documentation here
>> is not
>>
> a
>
>> > bug
>> > > you introduced.
>> > >
>> > > However, I have a bit of trouble reasoning why Set() will only let
>> you set
>> an
>> > > api_resource_id iff there exists a previous resource with that same
>> > > api_resource_id.
>> > >
>> > > If this is meant to be a compliment to ApiResourceData::Add(), then I
>> believe
>> > > you should document both this method and on 136, to explain the
>> correct
>> usage.
>> > >
>> > > Namely, that it exists to _replace_ an existing resource.
>> > >
>> > > I'm going to have to defer to the extension OWNERs on the overall
>> pattern,
>> but
>> > > it does have the back hairs on my neck tingling that there may be an
>> > opportunity
>> > > for "confused deputy" attacks - in which a caller still has a pointer
>> to
>>
> an
>
>> > > older resource ID, and it wants to perform some action, and some
>> security
>> > check
>> > > is made against the new object (eg: Returned by Get()). This is
>> primarily
>> > > because of the linked_ptr<> pattern.
>> >
>> > I've renamed it Replace(), and documented that it requires that the
>> resource
>> ID
>> > has to already exist and be owned by extension_id. I *think* that
>> different
>> > extensions are firewalled by the different extension_ids, which are
>> outside
>> the
>> > control of the relevant extensions -- does that help your concern about
>> confused
>> > deputies?
>>
>
> I just checked with Renaud, yup, the extension_ids keep the extensions'
>> state
>> separate from one another.
>>
>
> So, that wasn't exactly what I was concerned about.
>
> Assume a mapping of Extension ID A and resource_id 1, which maps to Foo*
> foo.
>
> Your new Set/Replace method introduces a means to associate a new object -
> Bar*
> bar (where Bar is-a Foo) with A-1
>
> The confused deputy issue I'm talking about is for the following call
> sequence:
>
> Foo* local_foo = Get(A,1);
> Replace(A, 1, bar);
> local_foo->DoSomething(); // Use-after-free!
>
> You can also apply it to the confused deputy problem of
>
> if (Get(A,1)->IsAllowedToDoStep1()) {
> // Do step 1
> PostTask(base::Bind(DoStep2, A, 1));
> }
>
> void DoStep2(extension, resource) {
> if (Get(extension, resource)->IsAllowedToDoStep2()) {
> // Do step 2
> }
> }
>
> Now, imagine that before DoStep2 is posted as a task, some other task is
> posted
> first. That task does something like
>
> Replace(A, 1, bar);
>
> You now have a confusion where "IsAllowedToDoStep1" was checked on Foo, but
> IsAllowedToDoStep2() is checked on Bar.
>
> You can logically extend this to any shared state - not just permission
> grants -
> but it hopefully demonstrates where the concern is coming from.
>
> I'm not concerned about extensions A and B "sharing" the same resource with
> resource_id of 1. I AM worried about extension A getting confused as to the
> underlying object it's talking to when it asks for "Resource 1".
>
> That sort of reasoning is much harder to do in this code review, and
> requires a
> holistic understanding of all the points of entry into the system. Which,
> you
> know, scares me a little. That's why I asked Renaud to take a look at this
> overall pattern and re-review it to make sure that the entry points are
> robust
> against this type of confusion or deferral.
>
> https://codereview.chromium.org/76403004/
>
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
> Renaud, am I correct in my analysis of the code? I think so. I think the fact that * we are essentially single-threaded (everything happens on the IO thread), * socket instances are re-acquired from resource ids from the resource manager before every operation * making a socket secure is a non-reversible operation (we only move "forward" from non-secure to secure) makes it (imho) very unlikely we expose "use after free" or "confused deputy" issues. That said, given what Ryan pointed out, I am slightly worried about "interleaved" calls from the app while the socket is switching from "non secure" to "secure" state. For example, what happens if an app calls "secure()", then "sent" right away without waiting for the "secure" callback to be invoked? It will likely be that the app will send clear data without realizing it, as the "secure" call can take some time to establish the secure channel. I don't have enough expertise with TLS to know if this is something we should worry about. In case we need to worry about it, we could add a "securing_" boolean to "Socket" and reject most operations while in that state. Another check we should probably make is that there is no pending "read" callback on the socket when securing it. I don't recall seeing code to that purpose. (I might be wrong). Lally, what do you think?
Yeah, if you call secure, you shouldn't allow any operations until secure finishes. Simply deferring then until secure is done should be sufficient. If you have pending operations outstanding, they need to complete before you begin securing. On Mar 3, 2014 4:25 PM, <rpaquay@chromium.org> wrote: > Renaud, am I correct in my analysis of the code? >> > > I think so. I think the fact that > > * we are essentially single-threaded (everything happens on the IO thread), > * socket instances are re-acquired from resource ids from the resource > manager > before every operation > * making a socket secure is a non-reversible operation (we only move > "forward" > from non-secure to secure) > > makes it (imho) very unlikely we expose "use after free" or "confused > deputy" > issues. > > That said, given what Ryan pointed out, I am slightly worried about > "interleaved" calls from the app while the socket is switching from "non > secure" > to "secure" state. For example, what happens if an app calls "secure()", > then > "sent" right away without waiting for the "secure" callback to be invoked? > It > will likely be that the app will send clear data without realizing it, as > the > "secure" call can take some time to establish the secure channel. I don't > have > enough expertise with TLS to know if this is something we should worry > about. In > case we need to worry about it, we could add a "securing_" boolean to > "Socket" > and reject most operations while in that state. > > Another check we should probably make is that there is no pending "read" > callback on the socket when securing it. I don't recall seeing code to that > purpose. (I might be wrong). > > Lally, what do you think? > > > https://codereview.chromium.org/76403004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Mon, Mar 3, 2014 at 6:25 PM, <rpaquay@chromium.org> wrote: > Renaud, am I correct in my analysis of the code? >> > > I think so. I think the fact that > > * we are essentially single-threaded (everything happens on the IO thread), > * socket instances are re-acquired from resource ids from the resource > manager > before every operation > * making a socket secure is a non-reversible operation (we only move > "forward" > from non-secure to secure) > > makes it (imho) very unlikely we expose "use after free" or "confused > deputy" > issues. > > That said, given what Ryan pointed out, I am slightly worried about > "interleaved" calls from the app while the socket is switching from "non > secure" > to "secure" state. For example, what happens if an app calls "secure()", > then > "sent" right away without waiting for the "secure" callback to be invoked? > It > will likely be that the app will send clear data without realizing it, as > the > "secure" call can take some time to establish the secure channel. I don't > have > enough expertise with TLS to know if this is something we should worry > about. In > case we need to worry about it, we could add a "securing_" boolean to > "Socket" > and reject most operations while in that state. > Before secure() returns, (SocketSecureFunction::Work calls UpgradeSocketToTLS), it calls TCPSocket::Release(), which releases all its net::*Socket members (UpgradeSocketToTLS takes a new reference to the client socket before calling Release), and resets its callbacks. It does everything TCPSocket::Disconnect does, except call Disconnect() on the underlying socket. This should have any other API call on that socket see the socket as closed. Any attempts to Destroy() the socket then just destroy the TCPSocket object, which secure()'s already emptied out at this point. When the TLS connection comes in, the socket ID gets re-populated with the TLSSocket. If the TLS connection fails, the SocketSecureFunction callback's redundant attempt to remove the mapping to the socket ID is harmless (via a GetOwnedResource() check in ApiResourceData::Remove()). > Another check we should probably make is that there is no pending "read" > callback on the socket when securing it. I don't recall seeing code to that > purpose. (I might be wrong). > Good point! I should add an accessor to TCPSocket that checks if read_callback_ is NULL, and make sure that read_callback_ is NULL when attempting to secure() (and aborting the attempt if there is a pending read). Currently, I was just wiping the read callback in TCPSocket::Release(), which is harmful. > Lally, what do you think? > Good catch on the pending read! To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Small addendum: at least with NSS, we can't hit this problem (as I've tested this situation exactly). UpgradeSocketToTLS will immediately fail when it can't issue a read() (as one is already outstanding), and the socket will get closed before the old callback can get invoked. That doesn't change the need for the read_callback_ check I mentioned in the prior email, I should add that in. Renaud, Ryan, thoughts on secure() and confused deputy now? Cheers, $ ls Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | lally@google.com On Mon, Mar 3, 2014 at 6:49 PM, Lally Singh <lally@google.com> wrote: > > On Mon, Mar 3, 2014 at 6:25 PM, <rpaquay@chromium.org> wrote: > >> Renaud, am I correct in my analysis of the code? >>> >> >> I think so. I think the fact that >> >> * we are essentially single-threaded (everything happens on the IO >> thread), >> * socket instances are re-acquired from resource ids from the resource >> manager >> before every operation >> * making a socket secure is a non-reversible operation (we only move >> "forward" >> from non-secure to secure) >> >> makes it (imho) very unlikely we expose "use after free" or "confused >> deputy" >> issues. >> >> That said, given what Ryan pointed out, I am slightly worried about >> "interleaved" calls from the app while the socket is switching from "non >> secure" >> to "secure" state. For example, what happens if an app calls "secure()", >> then >> "sent" right away without waiting for the "secure" callback to be >> invoked? It >> will likely be that the app will send clear data without realizing it, as >> the >> "secure" call can take some time to establish the secure channel. I don't >> have >> enough expertise with TLS to know if this is something we should worry >> about. In >> case we need to worry about it, we could add a "securing_" boolean to >> "Socket" >> and reject most operations while in that state. >> > > Before secure() returns, (SocketSecureFunction::Work calls > UpgradeSocketToTLS), it calls TCPSocket::Release(), which releases all its > net::*Socket members (UpgradeSocketToTLS takes a new reference to the > client socket before calling Release), and resets its callbacks. It does > everything TCPSocket::Disconnect does, except call Disconnect() on the > underlying socket. > > This should have any other API call on that socket see the socket as > closed. Any attempts to Destroy() the socket then just destroy the > TCPSocket object, which secure()'s already emptied out at this point. When > the TLS connection comes in, the socket ID gets re-populated with the > TLSSocket. If the TLS connection fails, the SocketSecureFunction > callback's redundant attempt to remove the mapping to the socket ID is > harmless (via a GetOwnedResource() check in ApiResourceData::Remove()). > > > >> Another check we should probably make is that there is no pending "read" >> callback on the socket when securing it. I don't recall seeing code to >> that >> purpose. (I might be wrong). >> > > Good point! I should add an accessor to TCPSocket that checks if > read_callback_ is NULL, and make sure that read_callback_ is NULL when > attempting to secure() (and aborting the attempt if there is a pending > read). Currently, I was just wiping the read callback in > TCPSocket::Release(), which is harmful. > > > >> Lally, what do you think? >> > Good catch on the pending read! > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The new patch set is up. PTAL, thanks. On Mar 4, 2014 12:01 PM, "Lally Singh" <lally@google.com> wrote: > Small addendum: at least with NSS, we can't hit this problem (as I've > tested this situation exactly). UpgradeSocketToTLS will immediately fail > when it can't issue a read() (as one is already outstanding), and the > socket will get closed before the old callback can get invoked. That > doesn't change the need for the read_callback_ check I mentioned in the > prior email, I should add that in. > > Renaud, Ryan, thoughts on secure() and confused deputy now? > > > Cheers, > $ ls > > Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | > lally@google.com > > > > On Mon, Mar 3, 2014 at 6:49 PM, Lally Singh <lally@google.com> wrote: > >> >> On Mon, Mar 3, 2014 at 6:25 PM, <rpaquay@chromium.org> wrote: >> >>> Renaud, am I correct in my analysis of the code? >>>> >>> >>> I think so. I think the fact that >>> >>> * we are essentially single-threaded (everything happens on the IO >>> thread), >>> * socket instances are re-acquired from resource ids from the resource >>> manager >>> before every operation >>> * making a socket secure is a non-reversible operation (we only move >>> "forward" >>> from non-secure to secure) >>> >>> makes it (imho) very unlikely we expose "use after free" or "confused >>> deputy" >>> issues. >>> >>> That said, given what Ryan pointed out, I am slightly worried about >>> "interleaved" calls from the app while the socket is switching from "non >>> secure" >>> to "secure" state. For example, what happens if an app calls "secure()", >>> then >>> "sent" right away without waiting for the "secure" callback to be >>> invoked? It >>> will likely be that the app will send clear data without realizing it, >>> as the >>> "secure" call can take some time to establish the secure channel. I >>> don't have >>> enough expertise with TLS to know if this is something we should worry >>> about. In >>> case we need to worry about it, we could add a "securing_" boolean to >>> "Socket" >>> and reject most operations while in that state. >>> >> >> Before secure() returns, (SocketSecureFunction::Work calls >> UpgradeSocketToTLS), it calls TCPSocket::Release(), which releases all its >> net::*Socket members (UpgradeSocketToTLS takes a new reference to the >> client socket before calling Release), and resets its callbacks. It does >> everything TCPSocket::Disconnect does, except call Disconnect() on the >> underlying socket. >> >> This should have any other API call on that socket see the socket as >> closed. Any attempts to Destroy() the socket then just destroy the >> TCPSocket object, which secure()'s already emptied out at this point. When >> the TLS connection comes in, the socket ID gets re-populated with the >> TLSSocket. If the TLS connection fails, the SocketSecureFunction >> callback's redundant attempt to remove the mapping to the socket ID is >> harmless (via a GetOwnedResource() check in ApiResourceData::Remove()). >> >> >> >>> Another check we should probably make is that there is no pending "read" >>> callback on the socket when securing it. I don't recall seeing code to >>> that >>> purpose. (I might be wrong). >>> >> >> Good point! I should add an accessor to TCPSocket that checks if >> read_callback_ is NULL, and make sure that read_callback_ is NULL when >> attempting to secure() (and aborting the attempt if there is a pending >> read). Currently, I was just wiping the read callback in >> TCPSocket::Release(), which is harmful. >> >> >> >>> Lally, what do you think? >>> >> Good catch on the pending read! >> >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/03/03 23:50:14, Lally Singh wrote: > On Mon, Mar 3, 2014 at 6:25 PM, <mailto:rpaquay@chromium.org> wrote: > > > Renaud, am I correct in my analysis of the code? > >> > > > > I think so. I think the fact that > > > > * we are essentially single-threaded (everything happens on the IO thread), Note that this isn't entirely correct (at least from a SWE perspective), because we're using asynchronous events, it's not really fair to consider it single-threaded, especially in the context of multiple (distinct) API calls. It's correct that we won't have data races, but we absolutely can have (acquire), (mutate), (check) as three distinct events on the single thread, where 'mutate' is the event that mutates internal state. > > * socket instances are re-acquired from resource ids from the resource > > manager > > before every operation > > * making a socket secure is a non-reversible operation (we only move > > "forward" > > from non-secure to secure) > > > > makes it (imho) very unlikely we expose "use after free" or "confused > > deputy" > > issues. > > > > That said, given what Ryan pointed out, I am slightly worried about > > "interleaved" calls from the app while the socket is switching from "non > > secure" > > to "secure" state. For example, what happens if an app calls "secure()", > > then > > "sent" right away without waiting for the "secure" callback to be invoked? > > It > > will likely be that the app will send clear data without realizing it, as > > the > > "secure" call can take some time to establish the secure channel. I don't > > have > > enough expertise with TLS to know if this is something we should worry > > about. In > > case we need to worry about it, we could add a "securing_" boolean to > > "Socket" > > and reject most operations while in that state. > > > > Before secure() returns, (SocketSecureFunction::Work calls > UpgradeSocketToTLS), it calls TCPSocket::Release(), which releases all its > net::*Socket members (UpgradeSocketToTLS takes a new reference to the > client socket before calling Release), and resets its callbacks. It does > everything TCPSocket::Disconnect does, except call Disconnect() on the > underlying socket. This makes me a little nervous, in that are you sure no pending callbacks exist? If they do, you run the risk of hanging things. > > This should have any other API call on that socket see the socket as > closed. Any attempts to Destroy() the socket then just destroy the > TCPSocket object, which secure()'s already emptied out at this point. When > the TLS connection comes in, the socket ID gets re-populated with the > TLSSocket. If the TLS connection fails, the SocketSecureFunction > callback's redundant attempt to remove the mapping to the socket ID is > harmless (via a GetOwnedResource() check in ApiResourceData::Remove()). Good. > > > > > Another check we should probably make is that there is no pending "read" > > callback on the socket when securing it. I don't recall seeing code to that > > purpose. (I might be wrong). > > > > Good point! I should add an accessor to TCPSocket that checks if > read_callback_ is NULL, and make sure that read_callback_ is NULL when > attempting to secure() (and aborting the attempt if there is a pending > read). Currently, I was just wiping the read callback in > TCPSocket::Release(), which is harmful. Right. > > > > > Lally, what do you think? > > > Good catch on the pending read! > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On 2014/03/04 17:02:15, Lally Singh wrote: > Small addendum: at least with NSS, we can't hit this problem (as I've > tested this situation exactly). UpgradeSocketToTLS will immediately fail > when it can't issue a read() (as one is already outstanding), and the > socket will get closed before the old callback can get invoked. That > doesn't change the need for the read_callback_ check I mentioned in the > prior email, I should add that in. > > Renaud, Ryan, thoughts on secure() and confused deputy now? Definitely don't rely on the behaviour of the library - especially as we're planning on switching to OpenSSL :) > > > Cheers, > $ ls > > Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | > mailto:lally@google.com > > > > On Mon, Mar 3, 2014 at 6:49 PM, Lally Singh <mailto:lally@google.com> wrote: > > > > > On Mon, Mar 3, 2014 at 6:25 PM, <mailto:rpaquay@chromium.org> wrote: > > > >> Renaud, am I correct in my analysis of the code? > >>> > >> > >> I think so. I think the fact that > >> > >> * we are essentially single-threaded (everything happens on the IO > >> thread), > >> * socket instances are re-acquired from resource ids from the resource > >> manager > >> before every operation > >> * making a socket secure is a non-reversible operation (we only move > >> "forward" > >> from non-secure to secure) > >> > >> makes it (imho) very unlikely we expose "use after free" or "confused > >> deputy" > >> issues. > >> > >> That said, given what Ryan pointed out, I am slightly worried about > >> "interleaved" calls from the app while the socket is switching from "non > >> secure" > >> to "secure" state. For example, what happens if an app calls "secure()", > >> then > >> "sent" right away without waiting for the "secure" callback to be > >> invoked? It > >> will likely be that the app will send clear data without realizing it, as > >> the > >> "secure" call can take some time to establish the secure channel. I don't > >> have > >> enough expertise with TLS to know if this is something we should worry > >> about. In > >> case we need to worry about it, we could add a "securing_" boolean to > >> "Socket" > >> and reject most operations while in that state. > >> > > > > Before secure() returns, (SocketSecureFunction::Work calls > > UpgradeSocketToTLS), it calls TCPSocket::Release(), which releases all its > > net::*Socket members (UpgradeSocketToTLS takes a new reference to the > > client socket before calling Release), and resets its callbacks. It does > > everything TCPSocket::Disconnect does, except call Disconnect() on the > > underlying socket. > > > > This should have any other API call on that socket see the socket as > > closed. Any attempts to Destroy() the socket then just destroy the > > TCPSocket object, which secure()'s already emptied out at this point. When > > the TLS connection comes in, the socket ID gets re-populated with the > > TLSSocket. If the TLS connection fails, the SocketSecureFunction > > callback's redundant attempt to remove the mapping to the socket ID is > > harmless (via a GetOwnedResource() check in ApiResourceData::Remove()). > > > > > > > >> Another check we should probably make is that there is no pending "read" > >> callback on the socket when securing it. I don't recall seeing code to > >> that > >> purpose. (I might be wrong). > >> > > > > Good point! I should add an accessor to TCPSocket that checks if > > read_callback_ is NULL, and make sure that read_callback_ is NULL when > > attempting to secure() (and aborting the attempt if there is a pending > > read). Currently, I was just wiping the read callback in > > TCPSocket::Release(), which is harmful. > > > > > > > >> Lally, what do you think? > >> > > Good catch on the pending read! > > > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Can you add a test for attempting to upgrade while a read is pending? https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket.h (right): https://codereview.chromium.org/76403004/diff/850001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket.h:111: // verifying a TLS certificate. On 2014/02/27 17:05:59, lally wrote: > On 2014/02/24 20:06:49, Ryan Sleevi wrote: > > // The hostname of the remote host that this socket is connected to. > > // Note: This may contain an IP literal. In the case of IDNs, this > > // will contain a series of A-LABELs, not U-LABELs. > > > > Am I correct with that above comment? How do you handle IDN names? If you pass > > them through, should this be a UTF-8 string? Or will you apply IDNA to > > canonicalize from the JS? At least the SSLSocket layer expects the canonicalized, valid form. That is, all hostnames as A-Labels, IPv6 as bracketed. I'm not sure where you want to do that, but that's at least needed. > > > > I had to investigate to find out what the upper-layer behavior is. This passes > through the UTF-8 string passed in from javascript. It's passed directly from > the javascript API as just a hostname, so there won't be any parsing, and there > shouldn't be any brackets for IPv6 addresses. > > Should I canonicalize? > > And is it valid for callers to supply IP addresses, for IP address certs? If > so, > > what's the form of IPv6 - bracketed or unbracketed? (Presumably, the latter). > https://codereview.chromium.org/76403004/diff/940001/chrome/browser/extension... File chrome/browser/extensions/api/api_resource_manager.h (right): https://codereview.chromium.org/76403004/diff/940001/chrome/browser/extension... chrome/browser/extensions/api/api_resource_manager.h:227: // |api_resource_id| to |resource|. |api_resource_id| must already nit: double spaces were re-introduced (and on line 137, and in the other files). I'll simply 'nit' them https://codereview.chromium.org/76403004/diff/940001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket.h (right): https://codereview.chromium.org/76403004/diff/940001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket.h:111: // This may contain an IP literal. In the case of IDNs, this will contain nit https://codereview.chromium.org/76403004/diff/940001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket.h:112: // a series of U-LABELs (UTF-8), not A-LABELs. IP literals for IPv6 will nit https://codereview.chromium.org/76403004/diff/940001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tcp_socket.cc (right): https://codereview.chromium.org/76403004/diff/940001/chrome/browser/extension... chrome/browser/extensions/api/socket/tcp_socket.cc:327: DCHECK(socket_.release() != NULL) SECURITY BUG: This will not actually release socket_, because DCHECKs may be compiled out. As such, it'll cause a double-free or a use-after-free. Your original code was correct. https://codereview.chromium.org/76403004/diff/940001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket_unittest.cc (right): https://codereview.chromium.org/76403004/diff/940001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket_unittest.cc:115: // ssl_socket_ is owned by socket_. SocketTest keeps a pointer to it to nit https://codereview.chromium.org/76403004/diff/940001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket_unittest.cc:196: // Each call from TLSSocket::Write() will invoke ssl_socket_'s Write(). ssl_socket_ (or at least, SSLClientSocket) only allows 1 write_ to be pending. This comment doesn't seem correct?
Hi. I've added a test that has a pending read() when calling secure(). Additionally, some global cleanup on documentation, spacing, and formatting. Please take another look. Thanks, - Lally https://codereview.chromium.org/76403004/diff/940001/chrome/browser/extension... File chrome/browser/extensions/api/api_resource_manager.h (right): https://codereview.chromium.org/76403004/diff/940001/chrome/browser/extension... chrome/browser/extensions/api/api_resource_manager.h:227: // |api_resource_id| to |resource|. |api_resource_id| must already On 2014/03/12 23:35:27, Ryan Sleevi wrote: > nit: double spaces were re-introduced (and on line 137, and in the other files). > I'll simply 'nit' them Ugh, old habits die hard. https://codereview.chromium.org/76403004/diff/940001/chrome/browser/extension... File chrome/browser/extensions/api/socket/socket.h (right): https://codereview.chromium.org/76403004/diff/940001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket.h:111: // This may contain an IP literal. In the case of IDNs, this will contain On 2014/03/12 23:35:27, Ryan Sleevi wrote: > nit Done. https://codereview.chromium.org/76403004/diff/940001/chrome/browser/extension... chrome/browser/extensions/api/socket/socket.h:112: // a series of U-LABELs (UTF-8), not A-LABELs. IP literals for IPv6 will On 2014/03/12 23:35:27, Ryan Sleevi wrote: > nit Done. https://codereview.chromium.org/76403004/diff/940001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tcp_socket.cc (right): https://codereview.chromium.org/76403004/diff/940001/chrome/browser/extension... chrome/browser/extensions/api/socket/tcp_socket.cc:327: DCHECK(socket_.release() != NULL) On 2014/03/12 23:35:27, Ryan Sleevi wrote: > SECURITY BUG: This will not actually release socket_, because DCHECKs may be > compiled out. As such, it'll cause a double-free or a use-after-free. > > Your original code was correct. Yikes. Fixed. https://codereview.chromium.org/76403004/diff/940001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket_unittest.cc (right): https://codereview.chromium.org/76403004/diff/940001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket_unittest.cc:115: // ssl_socket_ is owned by socket_. SocketTest keeps a pointer to it to On 2014/03/12 23:35:27, Ryan Sleevi wrote: > nit Done. https://codereview.chromium.org/76403004/diff/940001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket_unittest.cc:196: // Each call from TLSSocket::Write() will invoke ssl_socket_'s Write(). On 2014/03/12 23:35:27, Ryan Sleevi wrote: > ssl_socket_ (or at least, SSLClientSocket) only allows 1 write_ to be pending. > > This comment doesn't seem correct? Clarified here and in the documentation of TLSSocket. TLSSocket implements WriteImpl(), and TLSSocket::Write() is inherited from Socket::Write(), which implements a queue over calls to WriteImpl(). So, there's only ever 0 or 1 pending call to TLSSocket::WriteImpl(), allowing only 0 or 1 pending call to ssl_socket_->Write().
Also, I forgot to mention: I added IDN canonicalization (via net::CanonicalizeHost) to TLSSocket::UpgradeSocketToTLS. The NSS TLS stack already did this, but we shouldn't depend that other TLS implementations will as well. Cheers, $ ls Lally Singh | Google Ideas <http://www.google.com/ideas/> | Engineering | lally@google.com On Sun, Mar 16, 2014 at 9:58 PM, <lally@chromium.org> wrote: > Hi. > > I've added a test that has a pending read() when calling secure(). > Additionally, some global cleanup on documentation, spacing, and > formatting. > Please take another look. > > Thanks, > - Lally > > > > https://codereview.chromium.org/76403004/diff/940001/ > chrome/browser/extensions/api/api_resource_manager.h > File chrome/browser/extensions/api/api_resource_manager.h (right): > > https://codereview.chromium.org/76403004/diff/940001/ > chrome/browser/extensions/api/api_resource_manager.h#newcode227 > chrome/browser/extensions/api/api_resource_manager.h:227: // > |api_resource_id| to |resource|. |api_resource_id| must already > On 2014/03/12 23:35:27, Ryan Sleevi wrote: > >> nit: double spaces were re-introduced (and on line 137, and in the >> > other files). > >> I'll simply 'nit' them >> > > Ugh, old habits die hard. > > > https://codereview.chromium.org/76403004/diff/940001/ > chrome/browser/extensions/api/socket/socket.h > File chrome/browser/extensions/api/socket/socket.h (right): > > https://codereview.chromium.org/76403004/diff/940001/ > chrome/browser/extensions/api/socket/socket.h#newcode111 > chrome/browser/extensions/api/socket/socket.h:111: // This may contain > an IP literal. In the case of IDNs, this will contain > On 2014/03/12 23:35:27, Ryan Sleevi wrote: > >> nit >> > > Done. > > > https://codereview.chromium.org/76403004/diff/940001/ > chrome/browser/extensions/api/socket/socket.h#newcode112 > chrome/browser/extensions/api/socket/socket.h:112: // a series of > U-LABELs (UTF-8), not A-LABELs. IP literals for IPv6 will > On 2014/03/12 23:35:27, Ryan Sleevi wrote: > >> nit >> > > Done. > > > https://codereview.chromium.org/76403004/diff/940001/ > chrome/browser/extensions/api/socket/tcp_socket.cc > File chrome/browser/extensions/api/socket/tcp_socket.cc (right): > > https://codereview.chromium.org/76403004/diff/940001/ > chrome/browser/extensions/api/socket/tcp_socket.cc#newcode327 > chrome/browser/extensions/api/socket/tcp_socket.cc:327: > DCHECK(socket_.release() != NULL) > On 2014/03/12 23:35:27, Ryan Sleevi wrote: > >> SECURITY BUG: This will not actually release socket_, because DCHECKs >> > may be > >> compiled out. As such, it'll cause a double-free or a use-after-free. >> > > Your original code was correct. >> > Yikes. Fixed. > > > https://codereview.chromium.org/76403004/diff/940001/ > chrome/browser/extensions/api/socket/tls_socket_unittest.cc > File chrome/browser/extensions/api/socket/tls_socket_unittest.cc > (right): > > https://codereview.chromium.org/76403004/diff/940001/ > chrome/browser/extensions/api/socket/tls_socket_unittest.cc#newcode115 > chrome/browser/extensions/api/socket/tls_socket_unittest.cc:115: // > ssl_socket_ is owned by socket_. SocketTest keeps a pointer to it to > On 2014/03/12 23:35:27, Ryan Sleevi wrote: > >> nit >> > > Done. > > > https://codereview.chromium.org/76403004/diff/940001/ > chrome/browser/extensions/api/socket/tls_socket_unittest.cc#newcode196 > chrome/browser/extensions/api/socket/tls_socket_unittest.cc:196: // Each > call from TLSSocket::Write() will invoke ssl_socket_'s Write(). > On 2014/03/12 23:35:27, Ryan Sleevi wrote: > >> ssl_socket_ (or at least, SSLClientSocket) only allows 1 write_ to be >> > pending. > > This comment doesn't seem correct? >> > Clarified here and in the documentation of TLSSocket. TLSSocket > implements WriteImpl(), and TLSSocket::Write() is inherited from > Socket::Write(), which implements a queue over calls to WriteImpl(). > So, there's only ever 0 or 1 pending call to TLSSocket::WriteImpl(), > allowing only 0 or 1 pending call to ssl_socket_->Write(). > > https://codereview.chromium.org/76403004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/76403004/diff/960001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tcp_socket.cc (right): https://codereview.chromium.org/76403004/diff/960001/chrome/browser/extension... chrome/browser/extensions/api/socket/tcp_socket.cc:326: client_socket = socket_.release(); may get a clang warning here (because client_socket is assigned but not used) You could rewrite this DCHECK(socket_.get()) << "TCPSocket::Release on a null client socket."; socket_.release(); https://codereview.chromium.org/76403004/diff/960001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket.cc (right): https://codereview.chromium.org/76403004/diff/960001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:185: << "UpgradeSocketToTLS(): could not cononicalize hostnaame"; Seems to me like you shouldn't be DCHECKing here - this is caller-supplied (read: 'untrusted') input. if (host_info.family == url_canon::CanonHostInfo::BROKEN) { DVLOG(1) << "Could not canonicalize hostname"; TlsConnectDone(null_sock.Pass(), extension_id, callback, net::ERR_INVALID_ARGUMENT); } [Note: No need to include "UpgradeSocketToTLS()" in the log strings - dvlog includes file/line] https://codereview.chromium.org/76403004/diff/960001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket_unittest.cc (right): https://codereview.chromium.org/76403004/diff/960001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket_unittest.cc:109: pubic: typo: public != pubic https://codereview.chromium.org/76403004/diff/960001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket_unittest.cc:196: // The implemntation of TLSSocket::Write() is inherited from typo: implementation https://codereview.chromium.org/76403004/diff/960001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket_unittest.cc:201: // blocked request. The mocked Write() will get one request per Write() the mocked SSLClientSocket::Write() will get one request per TLSSocket::Write() request invoked on |socket_| ^ is that correct? Also a unit test: Large write (eg: > 16K). SSLClientSocketNSS won't accept more than 16K (one SSL record size). For safety, bump it up to 16K * 2 + 4K fudge (since implementations may allow one 'inflight' packet and one 'internal' packet)
Hi Ryan and Renaud, I've added the test Ryan asked for, and cleaned up any possible nits I could find along the way. Most importantly, this has been rebase'd to LKGR for 3/24. Please take another look and tell me what you think. Thank you for your patience and diligence. - Lally https://codereview.chromium.org/76403004/diff/960001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tcp_socket.cc (right): https://codereview.chromium.org/76403004/diff/960001/chrome/browser/extension... chrome/browser/extensions/api/socket/tcp_socket.cc:326: client_socket = socket_.release(); On 2014/03/18 01:05:13, Ryan Sleevi wrote: > may get a clang warning here (because client_socket is assigned but not used) > > You could rewrite this > > DCHECK(socket_.get()) << "TCPSocket::Release on a null client socket."; > socket_.release(); release() has WARN_UNUSED_RESULT on it, so the bare release() would get a warning instead. It builds in the current way for both debug and release modes without compiler complaint. https://codereview.chromium.org/76403004/diff/960001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket.cc (right): https://codereview.chromium.org/76403004/diff/960001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket.cc:185: << "UpgradeSocketToTLS(): could not cononicalize hostnaame"; On 2014/03/18 01:05:13, Ryan Sleevi wrote: > Seems to me like you shouldn't be DCHECKing here - this is caller-supplied > (read: 'untrusted') input. > > if (host_info.family == url_canon::CanonHostInfo::BROKEN) { > DVLOG(1) << "Could not canonicalize hostname"; > TlsConnectDone(null_sock.Pass(), extension_id, callback, > net::ERR_INVALID_ARGUMENT); > } > > [Note: No need to include "UpgradeSocketToTLS()" in the log strings - dvlog > includes file/line] Ah. I thought that since it successfully connected using that name, it should be fine. But yeah, there are some assumptions in there that I shouldn't be making. Fixed. https://codereview.chromium.org/76403004/diff/960001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket_unittest.cc (right): https://codereview.chromium.org/76403004/diff/960001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket_unittest.cc:109: pubic: On 2014/03/18 01:05:13, Ryan Sleevi wrote: > typo: public != pubic That's just embarrassing. https://codereview.chromium.org/76403004/diff/960001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket_unittest.cc:196: // The implemntation of TLSSocket::Write() is inherited from On 2014/03/18 01:05:13, Ryan Sleevi wrote: > typo: implementation Done.
Hi Lally, I've attempted to review your changes from patchset 19 to present. Rebasing and making changes in the same CL made this a bit harder. Usually a good strategy is to make all of your changes, then rebase. That way it's easier to see the diffs between things on a per-patchset level (eg: if you did fixes in 20/21, then rebased in 22, I could review just the diffs from 19->21) I apologize that I've missed a few things. Every time I come back, there's a huge amount of code to review, and I notice different things. I apologize that I haven't noticed everything at once, but hopefully this helps. https://codereview.chromium.org/76403004/diff/960001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tcp_socket.cc (right): https://codereview.chromium.org/76403004/diff/960001/chrome/browser/extension... chrome/browser/extensions/api/socket/tcp_socket.cc:326: client_socket = socket_.release(); On 2014/03/26 03:17:51, lally wrote: > On 2014/03/18 01:05:13, Ryan Sleevi wrote: > > may get a clang warning here (because client_socket is assigned but not used) > > > > You could rewrite this > > > > DCHECK(socket_.get()) << "TCPSocket::Release on a null client socket."; > > socket_.release(); > > release() has WARN_UNUSED_RESULT on it, so the bare release() would get a > warning instead. It builds in the current way for both debug and release modes > without compiler complaint. https://code.google.com/p/chromium/codesearch#chromium/src/base/macros.h&l=277 Is specifically designed for this use case. Sorry I wasn't clearer. https://codereview.chromium.org/76403004/diff/980001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket_unittest.cc (right): https://codereview.chromium.org/76403004/diff/980001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket_unittest.cc:255: .WillRepeatedly(Return(net::ERR_INVALID_ARGUMENT)); I don't entirely understand this. If the caller attempts to Write 32K, we should return success (that we wrote 16K). You seem to be suggesting the caller should be instead dropping things to 15K? https://codereview.chromium.org/76403004/diff/1040001/chrome/browser/extensio... File chrome/browser/extensions/api/sockets_tcp/sockets_tcp_apitest.cc (right): https://codereview.chromium.org/76403004/diff/1040001/chrome/browser/extensio... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_apitest.cc:123: // Start the servers in parallel. You say "servers", except you're only starting a single server here. What gives? https://codereview.chromium.org/76403004/diff/1040001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/sockets_tcp/api/background.js (right): https://codereview.chromium.org/76403004/diff/1040001/chrome/test/data/extens... chrome/test/data/extensions/api_test/sockets_tcp/api/background.js:355: // net::ERR_INVALID_ARGUMENT is -4. Wait, are we surfing the net:: errors as part of this API? I missed that, but that's not really desirable. I'm all for exposing errors as informative, but we "reserve" the right to change net errors (and what net errors are generated) at any time, and it seems dangerous to allow callers to bake things in. This may be an already existing issue with the sockets API, in which case, I just say "grump grump" and move on, but a red flag none the less. https://codereview.chromium.org/76403004/diff/1040001/extensions/browser/api/... File extensions/browser/api/socket/socket.h (right): https://codereview.chromium.org/76403004/diff/1040001/extensions/browser/api/... extensions/browser/api/socket/socket.h:48: void set_hostname(const std::string& hostname) { hostname_ = hostname; } Seems like the comment from 111-114 should be moved up here, documenting what the expected/acceptable form for |hostname| is when callers call set_hostname. https://codereview.chromium.org/76403004/diff/1040001/extensions/browser/api/... File extensions/browser/api/socket/tls_socket.h (right): https://codereview.chromium.org/76403004/diff/1040001/extensions/browser/api/... extensions/browser/api/socket/tls_socket.h:20: class TLSSocket; linebreak between 19/20 https://codereview.chromium.org/76403004/diff/1040001/extensions/browser/api/... extensions/browser/api/socket/tls_socket.h:21: typedef base::Callback<void(scoped_ptr<TLSSocket>, int)> SecureCallback; Should this be a TLSSocket typedef, rather than an extensions:: typedef? Otherwise, rename to TLSSocketCallback ? https://codereview.chromium.org/76403004/diff/1180001/extensions/browser/api/... File extensions/browser/api/socket/tls_socket.h (right): https://codereview.chromium.org/76403004/diff/1180001/extensions/browser/api/... extensions/browser/api/socket/tls_socket.h:92: scoped_refptr<net::URLRequestContextGetter> url_request_getter, DESIGN: Apologies that I just now noticed this (this is what happens with long/big reviews :/), but shouldn't we be instead passing CertVerifier and TransportSecurityState directly (eg: not forcing a dependency on URLRequestContextGetter here) We're already passing the SSLConfigService, and passing the CV/TSS (which may come from the URC, but that's an "implementation detail") seems to be a looser coupling to the necessary set of dependencies.
Hello! Updates on the latest patch-set: - use ignore_result() around socket_.release() (which is WARN_UNUSED_RESULT). - Changed the large-SSL write test's mocks to match TLS-record fragmentation behavior. - Stopped testing for specific error codes in the sockets_tcp test extension. - Moved a comment in socket/socket.h from the member decl (hostname_) to the setter for that member (set_hostname()). - renamed SecureCallback to TLSSocket::SecureCallback. - Removed dependencies on URLRequestContextGetter in TLSSocket. Callers now interrogate it and give only CertVerifier and TransportSecurityState to TLSSocket::UpgradeSocketToTLS(). There's an open question to be resolved over email (Renaud, Ryan, a Lucas are recipients) over the extension functions' use of profile.h and io_thread.h (as we need to get the SSLConfigServce from somewhere, and AFAICT, the profile and IO thread are the only sources). This was OK before, but has since become a trigger for a presubmit-check failure. I'm uploading this patch set (#24) in parallel to that discussion. Please let me know what you think! Thank you! https://codereview.chromium.org/76403004/diff/960001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tcp_socket.cc (right): https://codereview.chromium.org/76403004/diff/960001/chrome/browser/extension... chrome/browser/extensions/api/socket/tcp_socket.cc:326: client_socket = socket_.release(); On 2014/03/26 19:57:40, Ryan Sleevi wrote: > On 2014/03/26 03:17:51, lally wrote: > > On 2014/03/18 01:05:13, Ryan Sleevi wrote: > > > may get a clang warning here (because client_socket is assigned but not > used) > > > > > > You could rewrite this > > > > > > DCHECK(socket_.get()) << "TCPSocket::Release on a null client socket."; > > > socket_.release(); > > > > release() has WARN_UNUSED_RESULT on it, so the bare release() would get a > > warning instead. It builds in the current way for both debug and release > modes > > without compiler complaint. > > https://code.google.com/p/chromium/codesearch#chromium/src/base/macros.h&l=277 > > Is specifically designed for this use case. Sorry I wasn't clearer. Done. https://codereview.chromium.org/76403004/diff/980001/chrome/browser/extension... File chrome/browser/extensions/api/socket/tls_socket_unittest.cc (right): https://codereview.chromium.org/76403004/diff/980001/chrome/browser/extension... chrome/browser/extensions/api/socket/tls_socket_unittest.cc:255: .WillRepeatedly(Return(net::ERR_INVALID_ARGUMENT)); On 2014/03/26 19:57:40, Ryan Sleevi wrote: > I don't entirely understand this. > > If the caller attempts to Write 32K, we should return success (that we wrote > 16K). You seem to be suggesting the caller should be instead dropping things to > 15K? I misunderstood you when you said "SSLClientSocketNSS won't accept more than 16K (one SSL record size)" to mean it'll return an error (as above). I've changed the behavior of the test to do as you say above. https://codereview.chromium.org/76403004/diff/1040001/chrome/browser/extensio... File chrome/browser/extensions/api/sockets_tcp/sockets_tcp_apitest.cc (right): https://codereview.chromium.org/76403004/diff/1040001/chrome/browser/extensio... chrome/browser/extensions/api/sockets_tcp/sockets_tcp_apitest.cc:123: // Start the servers in parallel. On 2014/03/26 19:57:40, Ryan Sleevi wrote: > You say "servers", except you're only starting a single server here. What gives? The prior version of this test loaded up both stock-TCP and HTTPS servers, which I think is unnecessary bundling. I fixed the comment, thanks. https://codereview.chromium.org/76403004/diff/1040001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/sockets_tcp/api/background.js (right): https://codereview.chromium.org/76403004/diff/1040001/chrome/test/data/extens... chrome/test/data/extensions/api_test/sockets_tcp/api/background.js:355: // net::ERR_INVALID_ARGUMENT is -4. On 2014/03/26 19:57:40, Ryan Sleevi wrote: > Wait, are we surfing the net:: errors as part of this API? I missed that, but > that's not really desirable. > > I'm all for exposing errors as informative, but we "reserve" the right to change > net errors (and what net errors are generated) at any time, and it seems > dangerous to allow callers to bake things in. > > This may be an already existing issue with the sockets API, in which case, I > just say "grump grump" and move on, but a red flag none the less. Ah, I didn't think that this was a declaration of public API, as it's test-data for a unit test. I can change it to just a test for <0 https://codereview.chromium.org/76403004/diff/1040001/extensions/browser/api/... File extensions/browser/api/socket/socket.h (right): https://codereview.chromium.org/76403004/diff/1040001/extensions/browser/api/... extensions/browser/api/socket/socket.h:48: void set_hostname(const std::string& hostname) { hostname_ = hostname; } On 2014/03/26 19:57:40, Ryan Sleevi wrote: > Seems like the comment from 111-114 should be moved up here, documenting what > the expected/acceptable form for |hostname| is when callers call set_hostname. Done. https://codereview.chromium.org/76403004/diff/1040001/extensions/browser/api/... File extensions/browser/api/socket/tls_socket.h (right): https://codereview.chromium.org/76403004/diff/1040001/extensions/browser/api/... extensions/browser/api/socket/tls_socket.h:20: class TLSSocket; On 2014/03/26 19:57:40, Ryan Sleevi wrote: > linebreak between 19/20 Done. https://codereview.chromium.org/76403004/diff/1040001/extensions/browser/api/... extensions/browser/api/socket/tls_socket.h:21: typedef base::Callback<void(scoped_ptr<TLSSocket>, int)> SecureCallback; On 2014/03/26 19:57:40, Ryan Sleevi wrote: > Should this be a TLSSocket typedef, rather than an extensions:: typedef? > > Otherwise, rename to TLSSocketCallback ? Done. https://codereview.chromium.org/76403004/diff/1180001/extensions/browser/api/... File extensions/browser/api/socket/tls_socket.h (right): https://codereview.chromium.org/76403004/diff/1180001/extensions/browser/api/... extensions/browser/api/socket/tls_socket.h:92: scoped_refptr<net::URLRequestContextGetter> url_request_getter, On 2014/03/26 19:57:40, Ryan Sleevi wrote: > DESIGN: Apologies that I just now noticed this (this is what happens with > long/big reviews :/), but shouldn't we be instead passing CertVerifier and > TransportSecurityState directly (eg: not forcing a dependency on > URLRequestContextGetter here) > > We're already passing the SSLConfigService, and passing the CV/TSS (which may > come from the URC, but that's an "implementation detail") seems to be a looser > coupling to the necessary set of dependencies. No worries; Done. In the future I'll have to start thinking more strategically about CL sizes to make this easier for reviewers.
lally, How is it going? I can't wait to see this feature in chrome apps.
Hi Martin. I'm injured and in the hospital, but will get back to work on this as soon as I can. Cheers, $ ls On May 8, 2014 8:47 AM, <martincharles07@gmail.com> wrote: > lally, How is it going? I can't wait to see this feature in chrome apps. > > https://codereview.chromium.org/76403004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Oh, Sorry. Get well soon. Here have a hug. *hugs*
Hello! It's been a while! There are two new patch sets uploaded, 25 and 26. Patch set 25 is just a rebase to LKGR (Jul 3). The only manual changes here were for the rename from url_canon::CanonHostInfo to url::CanonHostInfo. 'git cl format' made some 1-line functions 3 lines long. Patch set 26 makes the same change to two files: - extensions/browser/api/socket/socket_api.cc - extensions/browser/api/sockets_tcp/sockets_tcp_api.cc The parameters to TLSSocket::UpgradeSocketToTLS() are now pulled only from the URLRequestContext (or the arguments to secure()). Previously, the SSLConfigService was pulled from the Profile (which is now no longer referenced or used). Please let me know what you think.
Oof. You're a saint for continuing to push on this CL. I'd be far more exhausted as author than as reviewer. I've focused only on the implementation; the tests I previously reviewed in detail, and in general, am going to defer to the extension/ owners for idiomatic extension-ness. There's some weirdness with the socket/ API that in an ideal world, one in which I don't live in, we could clean up, which would make the "synchronously or asynchronously invoke the callback" stuff clearer. The answer is "State machines, state machines everywhere", but I think that's a broader concern with the sockets/ API than your code, nor do I think it would do good for your code to buck the dominant trend here of this code. So, with all of those caveats, LGTM, with the following nits regarding comments/documentation. https://codereview.chromium.org/76403004/diff/1260001/extensions/browser/api/... File extensions/browser/api/socket/socket.h (right): https://codereview.chromium.org/76403004/diff/1260001/extensions/browser/api/... extensions/browser/api/socket/socket.h:51: // be unbracketed. This doc-comment documents what hostname_ stores, but it seems like you're instead documenting what the caller should supply (i.e. what their requirements are) Another distinction worth noting is whether or not hostname() is permitted to be empty / may ever be empty. https://codereview.chromium.org/76403004/diff/1260001/extensions/browser/api/... File extensions/browser/api/socket/socket_api.cc (right): https://codereview.chromium.org/76403004/diff/1260001/extensions/browser/api/... extensions/browser/api/socket/socket_api.cc:980: DCHECK(result == net::OK || socket == NULL); This comment documents the inversion of the conditional. From the comment, I would expect to read DCHECK(socket != NULL || result != net::OK) (which I believe it did, in the past) // If an error occurred, socket MUST be NULL alternatively DCHECK((socket && result == net::OK) || (!socket && result != net::OK)); or DCHECK((!!socket) == (result == net::OK)); although that's arguably less readable. https://codereview.chromium.org/76403004/diff/1260001/extensions/browser/api/... File extensions/browser/api/socket/tls_socket.cc (right): https://codereview.chromium.org/76403004/diff/1260001/extensions/browser/api/... extensions/browser/api/socket/tls_socket.cc:265: if (status != net::ERR_IO_PENDING && status != net::OK) { If Connect() synchronously returns net::OK, it's not going to invoke your connect_cb. While unlikely to happen, it's worth noting here. You should run connect_cb for all != ERR_IO_PENDING, but perhaps only log for != net::OK ? https://codereview.chromium.org/76403004/diff/1260001/extensions/browser/api/... extensions/browser/api/socket/tls_socket.cc:278: void TLSSocket::TlsConnectDone(scoped_ptr<net::SSLClientSocket> ssl_socket, This doesn't need to be a static class member, does it? That is, it seems to access all public data, so it should be fine to move to the unnamed namespace at the top. https://codereview.chromium.org/76403004/diff/1260001/extensions/browser/api/... File extensions/browser/api/socket/tls_socket.h (right): https://codereview.chromium.org/76403004/diff/1260001/extensions/browser/api/... extensions/browser/api/socket/tls_socket.h:55: // buffer, or a negative number if an error occurred. Does TLS work. "Does TLS work" should be expanded. At first, I thought this was a question that had the wrong punctuation. Perhaps: // Attempts to read |count| bytes of decrypted data from the TLS socket, // invoking |callback| with the actual number of bytes read, or a // network error code if an error occurred. https://codereview.chromium.org/76403004/diff/1260001/extensions/browser/api/... extensions/browser/api/socket/tls_socket.h:89: // may be NULL. Note: |callback| may be synchronously invoked before nit: line break before the Note: https://codereview.chromium.org/76403004/diff/1260001/extensions/browser/api/... File extensions/browser/api/sockets_tcp/sockets_tcp_api.cc (right): https://codereview.chromium.org/76403004/diff/1260001/extensions/browser/api/... extensions/browser/api/sockets_tcp/sockets_tcp_api.cc:532: DCHECK(result == net::OK || socket == NULL); same comments as before.
Mark: Hi! You were suggested from git-cl owners for:
extensions/browser/extension_function_histogram_value.h
tools/metrics/histograms/histograms.xml
Please let me know if you aren't the right person for this, and I'll fiddle
with git-cl owners to have it shake out someone else for this part of the
review. The remainder of the CL's already been reviewed, so please don't feel
obligated to worry about it!
Ryan: I tried to address your comments. Please let me know if there's anything
I didn't do the way you liked. Thanks again, it's been a long ride!!
https://codereview.chromium.org/76403004/diff/1260001/extensions/browser/api/...
File extensions/browser/api/socket/socket.h (right):
https://codereview.chromium.org/76403004/diff/1260001/extensions/browser/api/...
extensions/browser/api/socket/socket.h:51: // be unbracketed.
On 2014/07/15 00:18:45, Ryan Sleevi wrote:
> This doc-comment documents what hostname_ stores, but it seems like you're
> instead documenting what the caller should supply (i.e. what their
requirements
> are)
>
> Another distinction worth noting is whether or not hostname() is permitted to
be
> empty / may ever be empty.
Done.
https://codereview.chromium.org/76403004/diff/1260001/extensions/browser/api/...
File extensions/browser/api/socket/socket_api.cc (right):
https://codereview.chromium.org/76403004/diff/1260001/extensions/browser/api/...
extensions/browser/api/socket/socket_api.cc:980: DCHECK(result == net::OK ||
socket == NULL);
I changed the comment as suggested. Thanks.
On 2014/07/15 00:18:45, Ryan Sleevi wrote:
> This comment documents the inversion of the conditional.
>
> From the comment, I would expect to read
> DCHECK(socket != NULL || result != net::OK)
>
> (which I believe it did, in the past)
>
> // If an error occurred, socket MUST be NULL
>
> alternatively
>
> DCHECK((socket && result == net::OK) || (!socket && result != net::OK));
>
> or
> DCHECK((!!socket) == (result == net::OK)); although that's arguably less
> readable.
https://codereview.chromium.org/76403004/diff/1260001/extensions/browser/api/...
File extensions/browser/api/socket/tls_socket.cc (right):
https://codereview.chromium.org/76403004/diff/1260001/extensions/browser/api/...
extensions/browser/api/socket/tls_socket.cc:265: if (status !=
net::ERR_IO_PENDING && status != net::OK) {
Unexpected behavior for Connect(). I've fixed as you've suggested. Thanks.
On 2014/07/15 00:18:45, Ryan Sleevi wrote:
> If Connect() synchronously returns net::OK, it's not going to invoke your
> connect_cb. While unlikely to happen, it's worth noting here.
>
> You should run connect_cb for all != ERR_IO_PENDING, but perhaps only log for
!=
> net::OK ?
https://codereview.chromium.org/76403004/diff/1260001/extensions/browser/api/...
extensions/browser/api/socket/tls_socket.cc:278: void
TLSSocket::TlsConnectDone(scoped_ptr<net::SSLClientSocket> ssl_socket,
Good point, moved up and declaration removed from header.
On 2014/07/15 00:18:45, Ryan Sleevi wrote:
> This doesn't need to be a static class member, does it?
>
> That is, it seems to access all public data, so it should be fine to move to
the
> unnamed namespace at the top.
https://codereview.chromium.org/76403004/diff/1260001/extensions/browser/api/...
File extensions/browser/api/socket/tls_socket.h (right):
https://codereview.chromium.org/76403004/diff/1260001/extensions/browser/api/...
extensions/browser/api/socket/tls_socket.h:55: // buffer, or a negative number
if an error occurred. Does TLS work.
On 2014/07/15 00:18:45, Ryan Sleevi wrote:
> "Does TLS work" should be expanded. At first, I thought this was a question
that
> had the wrong punctuation.
>
> Perhaps:
> // Attempts to read |count| bytes of decrypted data from the TLS socket,
> // invoking |callback| with the actual number of bytes read, or a
> // network error code if an error occurred.
Done.
https://codereview.chromium.org/76403004/diff/1260001/extensions/browser/api/...
extensions/browser/api/socket/tls_socket.h:89: // may be NULL. Note: |callback|
may be synchronously invoked before
On 2014/07/15 00:18:45, Ryan Sleevi wrote:
> nit: line break before the Note:
Done.
https://codereview.chromium.org/76403004/diff/1260001/extensions/browser/api/...
File extensions/browser/api/sockets_tcp/sockets_tcp_api.cc (right):
https://codereview.chromium.org/76403004/diff/1260001/extensions/browser/api/...
extensions/browser/api/sockets_tcp/sockets_tcp_api.cc:532: DCHECK(result ==
net::OK || socket == NULL);
On 2014/07/15 00:18:45, Ryan Sleevi wrote:
> same comments as before.
Done.
histograms.xml and extension_function_histogram_value.h lgtm
Sorry for not having looked at this earlier. Assuming the base::Unretained in calling OnReadComplete is correct, lgtm https://codereview.chromium.org/76403004/diff/1340001/extensions/browser/api/... File extensions/browser/api/socket/tls_socket.cc (right): https://codereview.chromium.org/76403004/diff/1340001/extensions/browser/api/... extensions/browser/api/socket/tls_socket.cc:117: &TLSSocket::OnReadComplete, base::Unretained(this), io_buffer)); I'm probably missing something, since the normal TCPSocket code is doing the same things here, but what is making sure that this base::Unretained is correct here? If I understand how this works, |this| is owned by the APIResourceManager, and things like closing a socket Remove the socket from there and cause it to be deleted. I don't see anywhere in that code where it makes sure no reads are in progress. But maybe I'm missing some ownership link somewhere that makes this all work correctly. https://codereview.chromium.org/76403004/diff/1340001/extensions/browser/api/... File extensions/browser/api/socket/tls_socket.h (right): https://codereview.chromium.org/76403004/diff/1340001/extensions/browser/api/... extensions/browser/api/socket/tls_socket.h:25: // TLS Sockets from the chrome.socket and chrome.socket.tcp APIs. A regular nit: chrome.sockets.tcp https://codereview.chromium.org/76403004/diff/1340001/extensions/browser/api/... File extensions/browser/api/sockets_tcp/sockets_tcp_api.cc (right): https://codereview.chromium.org/76403004/diff/1340001/extensions/browser/api/... extensions/browser/api/sockets_tcp/sockets_tcp_api.cc:505: core_api::socket::SecureOptions legacy_params; Should this maybe be the other way around? Instead of using the deprecated API's SecureOptions in UpgradeSocketToTLS use the new one? I'm not entirely sure if it even makes sense to add new features to an API we've deprecated 5 or 6 releases before this will be available, although it probably doesn't hurt. https://codereview.chromium.org/76403004/diff/1340001/extensions/common/api/s... File extensions/common/api/sockets_tcp.idl (right): https://codereview.chromium.org/76403004/diff/1340001/extensions/common/api/s... extensions/common/api/sockets_tcp.idl:87: DOMString? min; I'd say that this should be an enum, but apparently our IDL files don't follow the web idl spec, so those values can't be enum members...
https://codereview.chromium.org/76403004/diff/1340001/extensions/browser/api/... File extensions/browser/api/socket/tls_socket.cc (right): https://codereview.chromium.org/76403004/diff/1340001/extensions/browser/api/... extensions/browser/api/socket/tls_socket.cc:117: &TLSSocket::OnReadComplete, base::Unretained(this), io_buffer)); On 2014/07/24 22:44:08, Marijn Kruisselbrink wrote: > I'm probably missing something, since the normal TCPSocket code is doing the > same things here, but what is making sure that this base::Unretained is correct > here? > > If I understand how this works, |this| is owned by the APIResourceManager, and > things like closing a socket Remove the socket from there and cause it to be > deleted. I don't see anywhere in that code where it makes sure no reads are in > progress. But maybe I'm missing some ownership link somewhere that makes this > all work correctly. My understanding is 1) At lower level, the actual read writes data to |io_buffer| which is refcounted, so we are never in danger of writing to free'ed memory. 2) The read callback is destroyed (and not called) when the socket is closed and/or deleted. 3) All of this should always happen on the same thread. If 1) 2) and 3) are verified, this should be safe.
https://codereview.chromium.org/76403004/diff/1340001/extensions/browser/api/... File extensions/browser/api/socket/tls_socket.cc (right): https://codereview.chromium.org/76403004/diff/1340001/extensions/browser/api/... extensions/browser/api/socket/tls_socket.cc:117: &TLSSocket::OnReadComplete, base::Unretained(this), io_buffer)); On 2014/07/24 23:06:13, rpaquay wrote: > On 2014/07/24 22:44:08, Marijn Kruisselbrink wrote: > > I'm probably missing something, since the normal TCPSocket code is doing the > > same things here, but what is making sure that this base::Unretained is > correct > > here? > > > > If I understand how this works, |this| is owned by the APIResourceManager, and > > things like closing a socket Remove the socket from there and cause it to be > > deleted. I don't see anywhere in that code where it makes sure no reads are in > > progress. But maybe I'm missing some ownership link somewhere that makes this > > all work correctly. > > My understanding is > > 1) At lower level, the actual read writes data to |io_buffer| which is > refcounted, so we are never in danger of writing to free'ed memory. > 2) The read callback is destroyed (and not called) when the socket is closed > and/or deleted. > 3) All of this should always happen on the same thread. > > If 1) 2) and 3) are verified, this should be safe. Ah yes, digging deeper through several more layers of base::Unretained passed socket_ objects, it indeed seems to be safe because in the end they all own eachother and get destroyed together, so when |this| in this spot is destroyed it ensures that the read callback won't be called. Sorry for the noise. Although maybe a comment like in TCPClientSocket::Read would help to make it clear why it is safe to use base::Unretained
On 2014/07/24 23:17:43, Marijn Kruisselbrink wrote: > https://codereview.chromium.org/76403004/diff/1340001/extensions/browser/api/... > File extensions/browser/api/socket/tls_socket.cc (right): > > https://codereview.chromium.org/76403004/diff/1340001/extensions/browser/api/... > extensions/browser/api/socket/tls_socket.cc:117: &TLSSocket::OnReadComplete, > base::Unretained(this), io_buffer)); > On 2014/07/24 23:06:13, rpaquay wrote: > > On 2014/07/24 22:44:08, Marijn Kruisselbrink wrote: > > > I'm probably missing something, since the normal TCPSocket code is doing the > > > same things here, but what is making sure that this base::Unretained is > > correct > > > here? > > > > > > If I understand how this works, |this| is owned by the APIResourceManager, > and > > > things like closing a socket Remove the socket from there and cause it to be > > > deleted. I don't see anywhere in that code where it makes sure no reads are > in > > > progress. But maybe I'm missing some ownership link somewhere that makes > this > > > all work correctly. > > > > My understanding is > > > > 1) At lower level, the actual read writes data to |io_buffer| which is > > refcounted, so we are never in danger of writing to free'ed memory. > > 2) The read callback is destroyed (and not called) when the socket is closed > > and/or deleted. > > 3) All of this should always happen on the same thread. > > > > If 1) 2) and 3) are verified, this should be safe. > Ah yes, digging deeper through several more layers of base::Unretained passed > socket_ objects, it indeed seems to be safe because in the end they all own > eachother and get destroyed together, so when |this| in this spot is destroyed > it ensures that the read callback won't be called. Sorry for the noise. Although > maybe a comment like in TCPClientSocket::Read would help to make it clear why it > is safe to use base::Unretained +1 for a comment to explain this. Wasn't obvious to me either :)
https://codereview.chromium.org/76403004/diff/1340001/extensions/browser/api/... File extensions/browser/api/socket/tls_socket.cc (right): https://codereview.chromium.org/76403004/diff/1340001/extensions/browser/api/... extensions/browser/api/socket/tls_socket.cc:117: &TLSSocket::OnReadComplete, base::Unretained(this), io_buffer)); On 2014/07/24 23:17:42, Marijn Kruisselbrink wrote: > Ah yes, digging deeper through several more layers of base::Unretained passed > socket_ objects, it indeed seems to be safe because in the end they all own > eachother and get destroyed together, so when |this| in this spot is destroyed > it ensures that the read callback won't be called. Sorry for the noise. Although > maybe a comment like in TCPClientSocket::Read would help to make it clear why it > is safe to use base::Unretained > It is documented - https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/stream_... Namely, if you call Disconnect() on a socket, you're guaranteed callbacks will not be called. It's safe iff you delete tls_socket_ when this object goes away. Which it does. Ergo safe :) This is actually the pattern I've recommended for extensions (such as, well, Extensions, this implementation, ChromeCast, Pepper), because this is actually a stronger guarantee about correctness than these APIs provide today, and would have avoided a variety of security bugs related to UAFs for the ad-hoc patterns employed. But yes, safe.
Also documented here: https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/socket....
The CQ bit was checked by lally@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lally@chromium.org/76403004/1400001
Thanks everyone. I've put in a comment modeled on TCPClientSocket::Read to explain the use of Unretained(). Committing now. https://codereview.chromium.org/76403004/diff/1340001/extensions/browser/api/... File extensions/browser/api/socket/tls_socket.cc (right): https://codereview.chromium.org/76403004/diff/1340001/extensions/browser/api/... extensions/browser/api/socket/tls_socket.cc:117: &TLSSocket::OnReadComplete, base::Unretained(this), io_buffer)); On 2014/07/24 23:17:42, Marijn Kruisselbrink wrote: > On 2014/07/24 23:06:13, rpaquay wrote: > > On 2014/07/24 22:44:08, Marijn Kruisselbrink wrote: > > > I'm probably missing something, since the normal TCPSocket code is doing the > > > same things here, but what is making sure that this base::Unretained is > > correct > > > here? > > > > > > If I understand how this works, |this| is owned by the APIResourceManager, > and > > > things like closing a socket Remove the socket from there and cause it to be > > > deleted. I don't see anywhere in that code where it makes sure no reads are > in > > > progress. But maybe I'm missing some ownership link somewhere that makes > this > > > all work correctly. > > > > My understanding is > > > > 1) At lower level, the actual read writes data to |io_buffer| which is > > refcounted, so we are never in danger of writing to free'ed memory. > > 2) The read callback is destroyed (and not called) when the socket is closed > > and/or deleted. > > 3) All of this should always happen on the same thread. > > > > If 1) 2) and 3) are verified, this should be safe. > Ah yes, digging deeper through several more layers of base::Unretained passed > socket_ objects, it indeed seems to be safe because in the end they all own > eachother and get destroyed together, so when |this| in this spot is destroyed > it ensures that the read callback won't be called. Sorry for the noise. Although > maybe a comment like in TCPClientSocket::Read would help to make it clear why it > is safe to use base::Unretained > Done. https://codereview.chromium.org/76403004/diff/1340001/extensions/browser/api/... File extensions/browser/api/socket/tls_socket.h (right): https://codereview.chromium.org/76403004/diff/1340001/extensions/browser/api/... extensions/browser/api/socket/tls_socket.h:25: // TLS Sockets from the chrome.socket and chrome.socket.tcp APIs. A regular On 2014/07/24 22:44:08, Marijn Kruisselbrink wrote: > nit: chrome.sockets.tcp Done. https://codereview.chromium.org/76403004/diff/1340001/extensions/browser/api/... File extensions/browser/api/sockets_tcp/sockets_tcp_api.cc (right): https://codereview.chromium.org/76403004/diff/1340001/extensions/browser/api/... extensions/browser/api/sockets_tcp/sockets_tcp_api.cc:505: core_api::socket::SecureOptions legacy_params; On 2014/07/24 22:44:08, Marijn Kruisselbrink wrote: > Should this maybe be the other way around? Instead of using the deprecated API's > SecureOptions in UpgradeSocketToTLS use the new one? > I'm not entirely sure if it even makes sense to add new features to an API we've > deprecated 5 or 6 releases before this will be available, although it probably > doesn't hurt. My thinking was to avoid having the old API (via its implementation of secure()) depend on any parts of the new API. The reverse dependency already exists generally. https://codereview.chromium.org/76403004/diff/1340001/extensions/common/api/s... File extensions/common/api/sockets_tcp.idl (right): https://codereview.chromium.org/76403004/diff/1340001/extensions/common/api/s... extensions/common/api/sockets_tcp.idl:87: DOMString? min; On 2014/07/24 22:44:08, Marijn Kruisselbrink wrote: > I'd say that this should be an enum, but apparently our IDL files don't follow > the web idl spec, so those values can't be enum members... Acknowledged.
Message was sent while issue was closed.
Change committed as 285804 |
