Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
113811:
Adding socks4 support for chromium. tested for windows and linux.... (Closed)
|
Created:
11 years, 7 months ago by Arindam Modified:
7 years, 6 months ago CC:
chromium-reviews_googlegroups.com, wtc, darin (slow to review) Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionAdding socks4 support for chromium. tested for windows and linux.
includes socks4, socks4a
TEST=change proxy settings to use proxy and set only the socks proxy fields (others must remain blank). Use CCProxy for windows or 'ssh -D' for linux / cygwin if trying it via localhost.
Browser should successfully open both http and https URLs.
tests are also at HttpNetworkTransactionTest.SOCKS*
BUG=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=19005
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 54
Patch Set 5 : '' #
Total comments: 24
Patch Set 6 : '' #
Total comments: 5
Patch Set 7 : '' #Patch Set 8 : '' #Patch Set 9 : '' #
Total comments: 87
Patch Set 10 : '' #Patch Set 11 : '' #
Total comments: 57
Patch Set 12 : replying to willchan's comments, added IPv6 failovers #
Total comments: 4
Patch Set 13 : fixing htons issue #Patch Set 14 : final patch #Patch Set 15 : final patch #
Messages
Total messages: 17 (0 generated)
Great work! I did a first pass of the change, focusing on the code style. I am sending that out now, but will follow-up soon with comments on the implementation details. The main thing that will need to be addressed is unit-tests. We can talk about how to best split that work up, but the main thing is going to be to include at least some basic ones in this CL. Check out http_network_transaction_unittest.cc to see how this can work. Those tests use mock socket dependencies. So the test hardcodes the read/write results of a socket, and then runs the transaction to see that it works/fails as expected. The same technique will work well for testing SOCKS. http://codereview.chromium.org/113811/diff/45/1031 File net/base/client_socket_factory.cc (right): http://codereview.chromium.org/113811/diff/45/1031#newcode47 Line 47: const ::GURL& referrer) { Please remove the "::". (Even better, remove the referrer parameter altogether). http://codereview.chromium.org/113811/diff/45/1030 File net/base/client_socket_factory.h (right): http://codereview.chromium.org/113811/diff/45/1030#newcode40 Line 40: const ::GURL& referrer) = 0; Our convention is not to include "::" for this case. Also, I don't believe this parameter is necessary. (From the looks of it, you only use "referrer" in a call to "DidFinishDnsResolutionWithStatus", which I have commented on in the respective file.) http://codereview.chromium.org/113811/diff/45/1033 File net/base/socks_client_socket.h (right): http://codereview.chromium.org/113811/diff/45/1033#newcode1 Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. You can set the date to simply 2009 here. http://codereview.chromium.org/113811/diff/45/1033#newcode17 Line 17: class SocksClientSocket : public ClientSocket { It doesn't seem like there is the need for this class. It seems like what is now SocksClientSocketImpl could be instead named SocksClientSocket. Or were you planning on using this as a branching point for SOCKS 4 vs SOCKS 5 implementations? http://codereview.chromium.org/113811/diff/45/1032 File net/base/socks_client_socket_impl.cc (right): http://codereview.chromium.org/113811/diff/45/1032#newcode1 Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. 2009 http://codereview.chromium.org/113811/diff/45/1032#newcode14 Line 14: #include "base/compiler_specific.h" Please order the headers alphabetically within each group. http://codereview.chromium.org/113811/diff/45/1032#newcode19 Line 19: static const std::string DEFAULT_USER_ID = ""; Our style guide disallows static initializers: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_... So instead you should use a const char* here. Secondarily, we name constants as "kDefaultUserId". http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constant_Names http://codereview.chromium.org/113811/diff/45/1032#newcode20 Line 20: static const char INVALID_IP[] = { '\0', '\0', '\0', '\127' }; Please name constants as kInvalidIp. Also a bit simpler to write this as: static const uint8[] = {0, 0, 0, 127}; Lastly, include a comment explaining what this constant means. http://codereview.chromium.org/113811/diff/45/1032#newcode25 Line 25: const ::GURL& referrer) remove :: http://codereview.chromium.org/113811/diff/45/1032#newcode171 Line 171: DidFinishDnsResolutionWithStatus(ok, referrer_, this); This doesn't seem quite right, can you explain what the intent is? http://codereview.chromium.org/113811/diff/45/1032#newcode196 Line 196: int record_size = 8 + userId.length() + 1; Can you extract "8" to a constant? http://codereview.chromium.org/113811/diff/45/1032#newcode205 Line 205: user_buf_ = scoped_refptr<IOBuffer>(new IOBuffer(user_buf_len_)); You can write this as: user_buf_ = new IOBuffer(user_buf_len_); (it has overloaded operator=). http://codereview.chromium.org/113811/diff/45/1032#newcode207 Line 207: char *buffer = user_buf_->data(); Please write this as "char* buffer" (asterisk beside the type). http://codereview.chromium.org/113811/diff/45/1032#newcode248 Line 248: int SocksClientSocketImpl::DoHandshakeRead() { small nit: add another newline separating from previous line. http://codereview.chromium.org/113811/diff/45/1032#newcode254 Line 254: user_buf_ = scoped_refptr<IOBuffer>(new IOBuffer(user_buf_len_)); Same comment as earlier, you can rely on operator=() overload: user_buf_ = new IOBuffer(user_buf_len_); http://codereview.chromium.org/113811/diff/45/1032#newcode262 Line 262: if (result < 8) In the case where transport socket gave an error, |result| will be negative. In this case should not return ERR_IO_PENDING (since there is nothing to wait for). http://codereview.chromium.org/113811/diff/45/1032#newcode268 Line 268: DCHECK(buffer_[0] == 0x00); Note that DCHECK() causes the application to crash, and it is only for asserting program correctness. In this case, buffer_[0] is a value controlled by the proxy server, so we should expect that it could be wrong. It think instead the handshake should be failed here. http://codereview.chromium.org/113811/diff/45/1032#newcode276 Line 276: return ERR_FAILED; We should try to pick a more specific error code here (potentially adding new ones to net_error_list.h if necessary). We try to make error codes as specific as possible, since it makes investigating user bug reports a LOT simpler :) (and for many network errors, the UI will show the network error code). http://codereview.chromium.org/113811/diff/45/1034 File net/base/socks_client_socket_impl.h (right): http://codereview.chromium.org/113811/diff/45/1034#newcode1 Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. 2009. http://codereview.chromium.org/113811/diff/45/1034#newcode13 Line 13: #include "net/base/completion_callback.h" Style nit: try to keep the headers list sorted, so in this case "completion_callback.h" should be listed before "socks_client_socket.h". The same applies for the rest of the headers. See also: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... http://codereview.chromium.org/113811/diff/45/1034#newcode21 Line 21: // The default socks client socket implementation I would lose the wording "default". Do you expect there will be alternate implementations of this? Would also be good to mention that this is implementing version 4(a) of the protocol. http://codereview.chromium.org/113811/diff/45/1034#newcode23 Line 23: public: The way we indent classes is a little tricky (one space for "public"). http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_... http://codereview.chromium.org/113811/diff/45/1034#newcode27 Line 27: explicit SocksClientSocketImpl(ClientSocket* transport_socket, Please remove the "explicit" keyword -- we only use that for constructors which have a single argument. See: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Explicit_Const... http://codereview.chromium.org/113811/diff/45/1034#newcode30 Line 30: const ::GURL& referrer); Please remove the leading "::" on GURL. http://codereview.chromium.org/113811/diff/45/1034#newcode31 Line 31: ~SocksClientSocketImpl(); Aesthetically, I suggest adding the keyword "virtual" to the destructor. (This isn't strictly necessary since we inherit the virtualness from Socket, but good to be explicit). http://codereview.chromium.org/113811/diff/45/1034#newcode43 Line 43: private: indent by one space. http://codereview.chromium.org/113811/diff/45/1034#newcode72 Line 72: enum ProxyType { How about the name "SocksVersion" instead? http://codereview.chromium.org/113811/diff/45/1034#newcode98 Line 98: ::GURL referrer_; Please remove the leading :: http://codereview.chromium.org/113811/diff/45/1024 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/113811/diff/45/1024#newcode540 Line 540: // any other type of proxy from the list (i.e. SOCKS). Can you update the comment to reflect this as well? http://codereview.chromium.org/113811/diff/45/1024#newcode642 Line 642: // If we are using a Socks connection, we need to setup handshake now. I suggest writing SOCKS in all instances. This is consistent with how we reference other protocols (HTTP, FTP, SSL), and is also correct since SOCKS is an acronym. http://codereview.chromium.org/113811/diff/45/1024#newcode649 Line 649: next_state_ = STATE_SOCKS_CONNECT; remove 2 spaces of indentation. http://codereview.chromium.org/113811/diff/45/1024#newcode670 Line 670: s, request_->url.HostNoBrackets(), request_->url.EffectiveIntPort(), To simplify supporting IPv6 down the road, I suggest passing a GURL instead of (host, port). i.e., pass along request_->url.GetOrigin() so you can pick out the bits later. http://codereview.chromium.org/113811/diff/45/1024#newcode678 Line 678: if (using_ssl_ && !using_tunnel_) { This duplicates some logic with DoTCPConnectComplete(), perhaps the commonality can be extracted to a method. http://codereview.chromium.org/113811/diff/45/1025 File net/http/http_network_transaction.h (right): http://codereview.chromium.org/113811/diff/45/1025#newcode286 Line 286: bool using_socks_proxy_; // True if using a socks proxy Instead of adding another boolean here (i find these boolean state already confusing to get right), perhaps this could be expressed as: enum ProxyType { PROXY_TYPE_NONE, PROXY_TYPE_HTTP, PROXY_TYPE_SOCKS, }; ProxyType proxy_type_; All the current instance of |using_proxy_| would become "proxy_type_ == PROXY_TYPE_HTTP", and the new stuff for transparent proxy (SOCKS) would become "proxy_type == PROXY_TYPE_SOCKS". What do you think? http://codereview.chromium.org/113811/diff/45/1026 File net/http/http_network_transaction_unittest.cc (right): http://codereview.chromium.org/113811/diff/45/1026#newcode342 Line 342: const std::string& hostname, Instead of hostname and port, I would suggest having a single: const GURL& origin; (This will make it easier to tell if the host is ipv6 or not, and defer the stripping of brackets till later). http://codereview.chromium.org/113811/diff/45/1026#newcode343 Line 343: int port, const ::GURL& referrer) { Can we remove the referrer param? http://codereview.chromium.org/113811/diff/45/1027 File net/proxy/proxy_config.cc (right): http://codereview.chromium.org/113811/diff/45/1027#newcode61 Line 61: if (url_scheme == "socks") { I will comment on this shortly, but some high-level ideas first: * If it is in fact windows-specific, it probably belongs in: net/proxy/proxy_config_service_win.cc with associated unit-test in: net/proxy/proxy_config_service_win_unittest.cc Otherwise, the unit-test for this case belongs in: net/proxy/proxy_config_unittest.cc To run the net unit unittests build and run "net_unittest[.exe]" Additionally to only run a subset of the tests you can do a prefix match: ./net_unittests --gtest_filter="ProxyConfigTest.*" http://codereview.chromium.org/113811/diff/45/1028 File net/proxy/proxy_server.cc (right): http://codereview.chromium.org/113811/diff/45/1028#newcode50 Line 50: // FIXME(arindam) defaulting socks to socks4 What is the fixme about? http://codereview.chromium.org/113811/diff/45/1028#newcode51 Line 51: if (LowerCaseEqualsASCII(begin, end, "socks")) Please add a corresponding test to proxy_server_unittest.cc
Please note that comments not replied to have been acknowledged and I will try to fix them by my next upload. For unit-tests, I found that we use a python httpd for http/ftp. Should a similar python socks-server implementation be needed for this, or checking simulated raw packets be sufficient ? http://codereview.chromium.org/113811/diff/45/1033 File net/base/socks_client_socket.h (right): http://codereview.chromium.org/113811/diff/45/1033#newcode17 Line 17: class SocksClientSocket : public ClientSocket { there were 2 reasons. 1. http_transactions unittest requires a MockSslClientSocket : public SslClientSocket, so I guess I would need one too. 2. for branching into socks4, socks5. However I realized that socks5 involves a few more states, but has more-or-less the same structure, so I am thinking of using the same class. http://codereview.chromium.org/113811/diff/45/1032 File net/base/socks_client_socket_impl.cc (right): http://codereview.chromium.org/113811/diff/45/1032#newcode171 Line 171: DidFinishDnsResolutionWithStatus(ok, referrer_, this); the documentation says that a lock might be needed when doing dns resolution. It begins with DidStartDnsResolution(hostname_, this); in the above state. http://codereview.chromium.org/113811/diff/45/1032#newcode262 Line 262: if (result < 8) What if the transport gave an incomplete buffer of 5 bytes ? Should not I wait for the result to reach 8 ? I understand that at this point I should perhaps increment bytes_received_ with result and then check if it reaches 8. Or are you saying that such a case is not possible by the socket implementation ? http://codereview.chromium.org/113811/diff/45/1034 File net/base/socks_client_socket_impl.h (right): http://codereview.chromium.org/113811/diff/45/1034#newcode30 Line 30: const ::GURL& referrer); this seemed to give me an error previously, the compiler assuming its net::GURL. I will explore it some more. http://codereview.chromium.org/113811/diff/45/1024 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/113811/diff/45/1024#newcode669 Line 669: s = reinterpret_cast<ClientSocket*>(socket_factory_->CreateSocksClientSocket( I dont understand why a reinterpret_cast<> is needed here explicitly. Further, the socket_factory_->CreateSSLClientSocket does not seem to need one although they are pretty similar in structure w.r.t. base class. http://codereview.chromium.org/113811/diff/45/1024#newcode670 Line 670: s, request_->url.HostNoBrackets(), request_->url.EffectiveIntPort(), I am not well familiar with IPv6, but is an a IPv6 endpoint not representable as a string (IP) + int (port) ? Also, the host here maybe the domain name instead of the actual resolved IP. http://codereview.chromium.org/113811/diff/45/1025 File net/http/http_network_transaction.h (right): http://codereview.chromium.org/113811/diff/45/1025#newcode286 Line 286: bool using_socks_proxy_; // True if using a socks proxy well, there is the subcase of tunneling which is enabled when its ssl and using a http proxy, while when using a direct connection or socks, tunneling is disabled for ssl. we could in that case go for an inline functions returning using_tunnel_ or using_proxy_. I think we already have enum ProxyType defined in ProxyServer. And Yes, this should make things easier to read. http://codereview.chromium.org/113811/diff/45/1028 File net/proxy/proxy_server.cc (right): http://codereview.chromium.org/113811/diff/45/1028#newcode50 Line 50: // FIXME(arindam) defaulting socks to socks4 when the user specifies a socks proxy without explicitly stating socks4 or socks5, what should we resolve it to ? I think this will become redundant in socks5 where we re-negotiate for the connection after auth. so we can just specify it to be socks4, no harm done.
http://codereview.chromium.org/113811/diff/45/1033 File net/base/socks_client_socket.h (right): http://codereview.chromium.org/113811/diff/45/1033#newcode17 Line 17: class SocksClientSocket : public ClientSocket { On 2009/05/27 06:53:21, Arindam wrote: > there were 2 reasons. > 1. http_transactions unittest requires a MockSslClientSocket : public > SslClientSocket, so I guess I would need one too. > 2. for branching into socks4, socks5. However I realized that socks5 involves a > few more states, but has more-or-less the same structure, so I am thinking of > using the same class. I say let's stick to just one class for now. If there is a need later, it is easy enough to break it out again. http://codereview.chromium.org/113811/diff/45/1032 File net/base/socks_client_socket_impl.cc (right): http://codereview.chromium.org/113811/diff/45/1032#newcode171 Line 171: DidFinishDnsResolutionWithStatus(ok, referrer_, this); On 2009/05/27 06:53:21, Arindam wrote: > the documentation says that a lock might be needed when doing dns resolution. It > begins with DidStartDnsResolution(hostname_, this); in the above state. > > The DidStartDnsResolution/DidFinishDnsResolutionWithStatus functions are for the DNS prefetcher, so it wouldn't impact correctness to remove. Go ahead and leave that in for now. I will think more about this issue (feels awkward that every location doing resolves needs to call this). http://codereview.chromium.org/113811/diff/45/1032#newcode262 Line 262: if (result < 8) On 2009/05/27 06:53:21, Arindam wrote: > What if the transport gave an incomplete buffer of 5 bytes ? Should not I wait > for the result to reach 8 ? > I understand that at this point I should perhaps increment bytes_received_ with > result and then check if it reaches 8. > > Or are you saying that such a case is not possible by the socket implementation > ? There are four cases you need to consider: 1. result < 0: the read failed (and socket is now closed) 2. result == 0: socket closed 3. result > 0: x bytes were read Be sure to check out "HttpNetworkTransaction::DoReadHeadersComplete" which does something very similar (and handles all the cases). Some very rough pseudo-code showing the cases you need to handle is: if (result < 0) { // FAIL! return socks_handshake_failed; } if (result == 0) { // FAIL -- the socket was closed (and we hadn't read enough bytes). return socks_handshake_failed; } // Otherwise we read |result| bytes from the socket, so append it to our running totals. bytes_read_so_far_.append(buffer_); // Did we read the full amount we want? if (bytes_read_so_far.size() < kHandshakResponseSize) { // Nope, keep reading. next_state_ = STATE_HANDSHAKE_READ; return OK; } // Did we read more than we were expecting? if (bytes_read_so_far.size() > kHandshakeResponseSize) { return socks_handshake_failed; } http://codereview.chromium.org/113811/diff/45/1034 File net/base/socks_client_socket_impl.h (right): http://codereview.chromium.org/113811/diff/45/1034#newcode30 Line 30: const ::GURL& referrer); On 2009/05/27 06:53:21, Arindam wrote: > this seemed to give me an error previously, the compiler assuming its net::GURL. > I will explore it some more. > You should not need to escape the scope. If compiler is complaining, it could be that there is an error somewhere and GURL is being forward-declared in the net namespace. http://codereview.chromium.org/113811/diff/45/1024 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/113811/diff/45/1024#newcode670 Line 670: s, request_->url.HostNoBrackets(), request_->url.EffectiveIntPort(), On 2009/05/27 06:53:21, Arindam wrote: > I am not well familiar with IPv6, but is an a IPv6 endpoint not representable as > a string (IP) + int (port) ? > Also, the host here maybe the domain name instead of the actual resolved IP. It is a matter of convenience -- once the host is reduced to a string, you need to re-cananonicalize it to see if it is a ipv6 literal. When you have a GURL object it is straightforward to query whether it is an IP address. http://codereview.chromium.org/113811/diff/45/1028 File net/proxy/proxy_server.cc (right): http://codereview.chromium.org/113811/diff/45/1028#newcode50 Line 50: // FIXME(arindam) defaulting socks to socks4 On 2009/05/27 06:53:21, Arindam wrote: > when the user specifies a socks proxy without explicitly stating socks4 or > socks5, what should we resolve it to ? > I think this will become redundant in socks5 where we re-negotiate for the > connection after auth. so we can just specify it to be socks4, no harm done. > Defaulting to v4 seems fine, I don't think you need a TODO associated with it. (We also control this format, so anything goes.) For what it is worth, we do the same thing in GetSchemeFromPacType() above (where we _don't_ control the format). For comapat reasons we consider: "SOCKS proxy:port" to mean: "SOCKS4 proxy:port" so it is distinct from "SOCKS5 proxy:port"
> For unit-tests, I found that we use a python httpd for http/ftp. Should > a similar python socks-server implementation be needed for this, or > checking simulated raw packets be sufficient ? Simulated packets is definitely the way to go for testing the SOCKS layer (no need for a SOCKS python server). While we do have a mock HTTP server, its purpose is for testing higher up layers of the stack (like url_requet and ui_tests). For the low-level layer which actually implements the HTTP protocol (http_network_transaction), the unit-tests exclusively use mock sockets. This approach gives the greatest flexibility since it allows replaying exact communications including socket failures. The same technique will work well for testing the SOCKS implementation. Here is how I picture it: ---------------------------------------------------------------------- (1) Add a new unit-test file [socks_client_socket_unittest.cc]. ---------------------------------------------------------------------- This is where the bulk of the SOCKS protocol tests will go, and where regression tests for bugs will live. The tests will create a SocksClientSocket instance, passing in mock dependencies for the transport socket and host resolver. Using the mock transport socket, the tests will play-back sequences of success (and failure) cases. The goal is to verify correctness, and pay particular attention to resilience of bad responses (which is the typical source of bugs). (Since we run all the unit-tests through memory checkers like valgrind/purify, simply getting code coverage for uncommon cases through tests is very valuable.) Some ideas for test cases: - A successful handshake followed by successful read/writes. - Failure resolving the host. - Failure on transport socket during handshake (write). - Failure on transport socket during handshake (read). - Failure because handshake response had too few bytes - Correct length of handshake response, but malformed (maybe the first byte isn't 0). - Multiple Socket::Read() are needed to read the handshake response. - Transport socket is closed right after handshake successfully completes. - Mismatched SOCKS versions. To create mock sockets, you include "net/base/socket_test_util.h" and use the mock socket classes it exposes. To see example usages, a good reference is "net/http/http_network_transaction_unittest.cc". ---------------------------------------------------------------------- (2) Add some new test cases to [http_network_transaction_unittest.cc]. ---------------------------------------------------------------------- Here we want to add a couple test of cases to make sure that SOCKS support is hooked up properly for HTTP transactions. These tests can be real short and simple, their main focus is verify that SOCKS is integrated for HTTP. Some ideas for test cases: - Start a HTTP transaction with proxy set to SOCKS -- verify that the SOCKS handshake was written/read to the mock socket (in which case we will assume SOCKS was correctly chosen). - Start a HTTP transaction with proxy set to SOCKS -- fail the communication with SOCKS server, see whether the transaction falls back to no proxy or outright fails.
I added a few unit tests, atleast for http_network_transaction as you suggested. I will be adding more unit tests for socksclientsocket, but can that be in a different CL ? http://codereview.chromium.org/113811/diff/45/1026 File net/http/http_network_transaction_unittest.cc (right): http://codereview.chromium.org/113811/diff/45/1026#newcode28 Line 28: struct MockConnect { these are changes by other users, I guess ;) http://codereview.chromium.org/113811/diff/1040/1041 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/113811/diff/1040/1041#newcode615 Line 615: if (using_proxy_ || using_tunnel_ || using_socks_proxy_) { I am thinking of changing the booleans in a different CL. Is that OK ? http://codereview.chromium.org/113811/diff/1040/1043 File net/http/http_network_transaction_unittest.cc (right): http://codereview.chromium.org/113811/diff/1040/1043#newcode3047 Line 3047: TEST_F(HttpNetworkTransactionTest, SOCKS4_BasicConnection) { adding a basic test to verify whether socks connection works. using the mocksslclientsocket defined in socket_test_util.cc http://codereview.chromium.org/113811/diff/1040/1052 File net/proxy/proxy_config_unittest.cc (right): http://codereview.chromium.org/113811/diff/1040/1052#newcode173 Line 173: // Only socks proxy present, others being blank. adding the test for proxy_server change http://codereview.chromium.org/113811/diff/1040/1053 File net/proxy/proxy_server_unittest.cc (left): http://codereview.chromium.org/113811/diff/1040/1053#oldcode165 Line 165: "socks://foopy", // not a valid scheme (needs to be socks4 or sock5). this is valid now since this gets defaulted http://codereview.chromium.org/113811/diff/1040/1053 File net/proxy/proxy_server_unittest.cc (right): http://codereview.chromium.org/113811/diff/1040/1053#newcode129 Line 129: // SOCKS proxy URIs (should default to SOCKS4) adding the test for proxy_server
> I will be adding more unit tests for socksclientsocket, but can that be in a > different CL ? OK. But can you add the new unit tests file as part of this CL? (its only contents will be the legal boiler plate, and a TODO about adding tests to it, and then adding this file to gyp.net). http://codereview.chromium.org/113811/diff/1040/1048 File net/base/client_socket_factory.cc (right): http://codereview.chromium.org/113811/diff/1040/1048#newcode43 Line 43: virtual SOCKSClientSocket* CreateSOCKSClientSocket( See comment in client_socket_factory.h about removing this. http://codereview.chromium.org/113811/diff/1040/1047 File net/base/client_socket_factory.h (right): http://codereview.chromium.org/113811/diff/1040/1047#newcode18 Line 18: class AddressList; Remove this (already forward declared). http://codereview.chromium.org/113811/diff/1040/1047#newcode19 Line 19: class HostResolver; Remove this (not used by this header). http://codereview.chromium.org/113811/diff/1040/1047#newcode36 Line 36: virtual SOCKSClientSocket* CreateSOCKSClientSocket( I suggest taking this out of ClientSocketFactory. Instead, you can just call "new SOCKSClientSocket" directly where you need it. The usefulness of CreateTCPClientSocket and CreateSSLClientSocket methods is that there are alternate implementations of those classes (platform specific ones, and mock vs actual). However as there is only 1 implementation for SOCKS (no mock or platform-specific one), it isn't useful to add this to the interface. http://codereview.chromium.org/113811/diff/1040/1054 File net/base/socket_test_util.cc (right): http://codereview.chromium.org/113811/diff/1040/1054#newcode99 Line 99: class MockSOCKSClientSocket : public net::SOCKSClientSocket { Callers needing a mock should instead use dependency injection: MockTCPClientSocket* transport_socket = ...; SOCKSClientSocket* mock_socks_socket = net SOCKSClientSocket(transport_socket, hostname, port, referrer); Therefore this class is not needed. http://codereview.chromium.org/113811/diff/1040/1054#newcode379 Line 379: SOCKSClientSocket* MockClientSocketFactory::CreateSOCKSClientSocket( See comment in client_socket_factory.h about removing this. http://codereview.chromium.org/113811/diff/1040/1055 File net/base/socket_test_util.h (right): http://codereview.chromium.org/113811/diff/1040/1055#newcode128 Line 128: virtual SOCKSClientSocket* CreateSOCKSClientSocket( See comment in client_socket_factory.h about removing this. http://codereview.chromium.org/113811/diff/1040/1049 File net/base/socks_client_socket_impl.cc (right): http://codereview.chromium.org/113811/diff/1040/1049#newcode24 Line 24: // The SOCKS4a implementation suggests to use an invalid IP in case the dns nit: upper case DNS http://codereview.chromium.org/113811/diff/1040/1049#newcode28 Line 28: // For SOCKS4, the client sends 8 bytes plus the size of the user-id. good comments. http://codereview.chromium.org/113811/diff/1040/1049#newcode36 Line 36: const std::string printAddress(char* buf) { How about NetAddressToString(), defined in "net/base/net_util.h" ? http://codereview.chromium.org/113811/diff/1040/1049#newcode215 Line 215: // We dont need to convert the port from host-order to n/w order. dont -> don't. Not sure what is meant by that comment, since we do write it into buffer in network byte order. http://codereview.chromium.org/113811/diff/1040/1049#newcode226 Line 226: buffer[2] = (nport >> 8) & 255; // Port in network byte order nit: I recommend writing 255 as 0xFF instead. http://codereview.chromium.org/113811/diff/1040/1049#newcode227 Line 227: buffer[3] = nport & 255; same comment as above. http://codereview.chromium.org/113811/diff/1040/1049#newcode260 Line 260: bytes_sent_ += result; You need to issue another Write() here or nothing will happen. When you call Socket::Write(buffer, length, callback), the callback is only called once. So if everything you want didn't get written, another Write() must be called. If more bytes need to be sent, must rewind the state such that it will issue another Write() for the remaining bytes in the buffer. You can see how this is done in HttpNetworkTransaction::DoWriteHeaders, HttpNetworkTransaction::DoWriteHeadersComplete() by jumping between states STATE_WRITE_HEADERS and STATE_WRITE_HEADERS_COMPLETE. You could organize this similarly -- move the code which builds the actual handshake data buffer out of DoHandshakeWrite() and into BuildHandshakeWriteBuffer(). Then in DoHandshakeWrite() you lazy-create that buffer, and then set the data to write as the |bytes_sent_| index into that buffer (and length as whatever remains). http://codereview.chromium.org/113811/diff/1040/1049#newcode292 Line 292: return ERR_IO_PENDING; Same comment as above. Setting the state to ERR_IO_PENDING isn't enough, you need to start another Read(). Best way is to rewind the state back to DoHandshakRead(). http://codereview.chromium.org/113811/diff/1040/1051 File net/base/socks_client_socket_impl.h (right): http://codereview.chromium.org/113811/diff/1040/1051#newcode23 Line 23: class SOCKSClientSocketImpl : public SOCKSClientSocket { So can this be renamed to just SOCKSClientSocket ? http://codereview.chromium.org/113811/diff/1040/1051#newcode31 Line 31: const GURL& referrer); Can you add a comment here explaining why we have a referrer parameter (since it is sort of a hack). Something like: // |referrer| is the referrer for the request that first created // this socket. This is used only once to notify the DNS prefetcher // on host resolve. The socket can later be re-used to issue // requests from a different referrer, but will not resolve host again. http://codereview.chromium.org/113811/diff/1040/1041 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/113811/diff/1040/1041#newcode544 Line 544: // Supporting SOCKS is issue http://crbug.com/469. How about removing this line, and the reference to SOCKS above? http://codereview.chromium.org/113811/diff/1040/1053 File net/proxy/proxy_server_unittest.cc (right): http://codereview.chromium.org/113811/diff/1040/1053#newcode130 Line 130: { // NOLINT What does NOLINT mean?
I had to re-do the minor changes at some places since on updating the tree I was greeted with a lot of conflicts. I also removed CreateSOCKSClientSocket from client_socket_factory, and further calls are via new SOCKSClientSocket() I also made some changes w.r.t. host resolver within socks client socket, and also in the way handshake read/writes are done (as you suggested). http://codereview.chromium.org/113811/diff/4001/5007 File net/base/client_socket_factory.h (right): http://codereview.chromium.org/113811/diff/4001/5007#newcode11 Line 11: class GURL; no longer necessary, will remove it. http://codereview.chromium.org/113811/diff/4001/5009 File net/base/socks_client_socket.h (right): http://codereview.chromium.org/113811/diff/4001/5009#newcode33 Line 33: SOCKSClientSocket(ClientSocket* transport_socket, I think we can safely remove the referrer, now that DidDnsResolution locks are removed. http://codereview.chromium.org/113811/diff/4001/5003 File net/http/http_network_transaction_unittest.cc (right): http://codereview.chromium.org/113811/diff/4001/5003#newcode3009 Line 3009: TEST_F(HttpNetworkTransactionTest, SOCKS4_HTTP_GET) { this makes a successful SOCKS4 HTTP GET request http://codereview.chromium.org/113811/diff/4001/5003#newcode3056 Line 3056: TEST_F(HttpNetworkTransactionTest, SOCKS4_SSL_GET) { this makes a successful SOCKS4 HTTPS GET request http://codereview.chromium.org/113811/diff/4001/5004 File net/proxy/proxy_config.cc (right): http://codereview.chromium.org/113811/diff/4001/5004#newcode61 Line 61: // This gets hit only on windows when using IE settings. I think the same happens for linux when using gconf. I will verify and confirm.
This is almost ready to be committed. Following are some more comments. http://codereview.chromium.org/113811/diff/5072/5080 File net/base/client_socket_factory.cc (right): http://codereview.chromium.org/113811/diff/5072/5080#newcode14 Line 14: #include "base/singleton.h" you can probably remove "client_socket_factory.cc" from this changelist. http://codereview.chromium.org/113811/diff/5072/5079 File net/base/client_socket_factory.h (right): http://codereview.chromium.org/113811/diff/5072/5079#newcode9 Line 9: #include "net/base/socks_client_socket.h" no need for this. in fact, "client_socket_factor.h" can be removed from this CL now. http://codereview.chromium.org/113811/diff/5072/5082 File net/base/socks_client_socket.cc (right): http://codereview.chromium.org/113811/diff/5072/5082#newcode57 Line 57: resolver_(new HostResolver()) { the HostResolver* needs to be a dependency that was passed in, rather than creating a new one here. So add HostResolver* as an argument to the constructor and let the caller provide it. I made this change in <http://codereview.chromium.org/118100> -- the goal was to have a funnel-point for caching and scheduling of dns resolves. http://codereview.chromium.org/113811/diff/5072/5082#newcode97 Line 97: CompletionCallback* callback) { line up the indentations. http://codereview.chromium.org/113811/diff/5072/5082#newcode108 Line 108: CompletionCallback* callback) { line up the indentations. http://codereview.chromium.org/113811/diff/5072/5082#newcode120 Line 120: // since Run may result in Read being called, clear user_callback_ up front. "since Run may result" ---> "Since Run() may result" http://codereview.chromium.org/113811/diff/5072/5082#newcode165 Line 165: rv = ERR_FAILED; how about: rv = ERR_UNEXPECTED; http://codereview.chromium.org/113811/diff/5072/5082#newcode176 Line 176: // DidStartDnsResolution(hostname_, this); You can delete this now. http://codereview.chromium.org/113811/diff/5072/5082#newcode177 Line 177: return resolver_.Resolve(HostResolver::RequestInfo(hostname_, port_), See comment made in the header file -- we want to use a RequestInfo that was passed in by the caller (since it may contain the necessary "referrer" information). http://codereview.chromium.org/113811/diff/5072/5082#newcode178 Line 178: &addresses_, &io_callback_); indent line continuations by 4 spaces. http://codereview.chromium.org/113811/diff/5072/5082#newcode185 Line 185: // DidFinishDnsResolutionWithStatus(ok, referrer_, this); You can delete this now. http://codereview.chromium.org/113811/diff/5072/5082#newcode216 Line 216: int16 nport = port_; please only 1 space around the equals. http://codereview.chromium.org/113811/diff/5072/5082#newcode249 Line 249: bytes_sent_ = 0; This doesn't really have to do with BuildHandshakeWRiteBuffer() -- how about moving it to the caller? http://codereview.chromium.org/113811/diff/5072/5082#newcode256 Line 256: if (!buffer_is_built_) instead of needing this boolean, how about. if (!buffer_.get()) http://codereview.chromium.org/113811/diff/5072/5082#newcode259 Line 259: user_buf_len_ = buffer_len_ - bytes_sent_; perhaps add a DCHECK_GT(user_buf_len_, 0); For good measure. http://codereview.chromium.org/113811/diff/5072/5082#newcode271 Line 271: return ERR_FAILED; return "result" instead. That way we propagate the reason of the error back up the stack, rather than masking with the unhelpful "ERR_FAILED". http://codereview.chromium.org/113811/diff/5072/5082#newcode276 Line 276: buffer_.reset(new char[kReadHeaderSize]); I think these next three lines would be more appropriate inside of DoHandshakeRead(). So what you could do instead is just set: buffer_.reset(NULL); And then within DoHandshakeRead() you would follow a similar model to how you lazy-call BuildHandshakeWriteBuffer(): DoHandshakeRead() { if (!buffer_.get()) { // Starting the first read. buffer_.reset(new char[kReadHeaderSize]); buffer_len_ = kReadHeaderSize; bytes_received_ = 0; } ... } http://codereview.chromium.org/113811/diff/5072/5082#newcode295 Line 295: return transport_->Read(user_buf_, user_buf_len_, &io_callback_); can you rename "user_buf" to something more like "handshake_buf" ? We generally use the naming "user_buf" for a buffer that was passed in by the user (and we are filling for them). However in this case, it is just an internal buffer we are using to read the handshake. http://codereview.chromium.org/113811/diff/5072/5082#newcode304 Line 304: return ERR_FAILED; return result; http://codereview.chromium.org/113811/diff/5072/5082#newcode306 Line 306: return ERR_FAILED; How about using ERR_INVALID_RESPONSE here instead. (we try to keep error codes as descriptive as possible, since it makes diagnosing problems easier). http://codereview.chromium.org/113811/diff/5072/5082#newcode319 Line 319: return ERR_UNEXPECTED; How about ERR_INVALID_RESPONSE. ERR_UNEXPECTED is reserved for code logic problems (think of it like an ASSERT() gone bad). http://codereview.chromium.org/113811/diff/5072/5082#newcode328 Line 328: return ERR_FAILED; add a TODO for adding a more specific error code to net_error_list.h http://codereview.chromium.org/113811/diff/5072/5082#newcode332 Line 332: return ERR_NAME_NOT_RESOLVED; add a TODO for adding a more specific error code to net_error_list.h http://codereview.chromium.org/113811/diff/5072/5082#newcode336 Line 336: return ERR_FAILED; add a TODO for adding a more specific error code to net_error_list.h http://codereview.chromium.org/113811/diff/5072/5082#newcode339 Line 339: return ERR_UNEXPECTED; ERR_INVALID_RESPONSE http://codereview.chromium.org/113811/diff/5072/5082#newcode344 Line 344: return ERR_FAILED; You can probably delete these two lines, since the default: will catch all. http://codereview.chromium.org/113811/diff/5072/5082#newcode350 Line 350: int SOCKSClientSocket::GetPeerName(struct sockaddr *name, socklen_t *namelen) { I don't think you actually need to provide a definition for this -- there is already one defined on the base class (and we aren't providing a better one here). http://codereview.chromium.org/113811/diff/5072/5081 File net/base/socks_client_socket.h (right): http://codereview.chromium.org/113811/diff/5072/5081#newcode15 Line 15: #include "net/base/client_socket.h" please order these. for example: #include "net/base/host_resolver.h" #include "net/base/client_socket.h" #include "net/base/completion_callback.h" #include "net/base/net_errors.h" http://codereview.chromium.org/113811/diff/5072/5081#newcode15 Line 15: #include "net/base/client_socket.h" order "client_socket.h" before "completion_callback.h" http://codereview.chromium.org/113811/diff/5072/5081#newcode18 Line 18: #include "googleurl/src/gurl.h" "googleurl" should be ordered after "base" and before "net" http://codereview.chromium.org/113811/diff/5072/5081#newcode25 Line 25: remove this empty line. http://codereview.chromium.org/113811/diff/5072/5081#newcode29 Line 29: // |referrer| is the referrer for the request that first created please remove the referrer field. i refactored the need for DidStartDnsResolution() in <http://codereview.chromium.org/125107> So what you want to do now is replace the {hostname, port, referrer} arguments by a single "const HostResolver::RequestInfo& info" parameter. This "info" parameter is what you will pass to HostResolver::Resolve(). You can extract the hostname and port with info.hostname(), info.port(). http://codereview.chromium.org/113811/diff/5072/5081#newcode72 Line 72: enum State { Please move this enum definition to the top of the "private:" section. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... http://codereview.chromium.org/113811/diff/5072/5081#newcode79 Line 79: STATE_NONE for consistency, end this with a comma (same as the SocksVersion enum). http://codereview.chromium.org/113811/diff/5072/5081#newcode87 Line 87: enum SocksVersion { Move this up. http://codereview.chromium.org/113811/diff/5072/5081#newcode109 Line 109: // call this is false. As the user_buf_ is filled withe the write data, typo: withe --> with. also i believe |buf_| is what contains the handshake write data. http://codereview.chromium.org/113811/diff/5072/5081#newcode111 Line 111: bool buffer_is_built_; I comment in the .cc file that this seems unecessary, can test buffer_.get() instead. http://codereview.chromium.org/113811/diff/5072/5081#newcode129 Line 129: virtual int GetPeerName(struct sockaddr *name, socklen_t *namelen); move this into the public section (in the ClientSocket methods section). http://codereview.chromium.org/113811/diff/5072/5081#newcode131 Line 131: }; as the last statement in the private section, add: DISALLOW_COPY_AND_ASSIGN(SOCKSClientSocket); http://codereview.chromium.org/113811/diff/5072/5075 File net/http/http_network_transaction_unittest.cc (right): http://codereview.chromium.org/113811/diff/5072/5075#newcode3053 Line 3053: EXPECT_FALSE(response == NULL); Nice job with adding some tests! These looks good. One final thing to do in this test, is do some verification on the contents of |response|. To check the BODY (payload of the http response), you can write something like: std::string response_text; rv = ReadTransaction(trans.get(), &response_text); EXPECT_EQ(OK, rv); EXPECT_EQ("Payload", response_text); Of course, you would additionally need to add a MockRead("Payload") entry as the second-to-last element of |data_reads|. http://codereview.chromium.org/113811/diff/5072/5075#newcode3073 Line 3073: MSVC_PUSH_DISABLE_WARNING(4309) I suggest defining the array as unsigned char[] and avoiding these suppressions. And then casting to a char* if needed by MockWrite/MockRead. http://codereview.chromium.org/113811/diff/5072/5075#newcode3107 Line 3107: EXPECT_FALSE(response == NULL); Same comment as for other test -- please add a verification of the response. http://codereview.chromium.org/113811/diff/5072/5078 File net/net.gyp (right): http://codereview.chromium.org/113811/diff/5072/5078#newcode123 Line 123: 'base/socks_client_socket.h', please order alphabetically (this goes after "socket.h") http://codereview.chromium.org/113811/diff/5072/5078#newcode426 Line 426: 'base/socks_client_socket_unittest.cc', please order alphabetically (this goes after sdch_filter_unittest.cc).
I was not able to find the HostResolver for http to pass along, so I am still re-creating that object at ctor time. Also, I moved socks_client_socket_unittests.cc to a different CL so that it is easier for me to manage. However the net.gyp dependence in this CL requires that file. http://codereview.chromium.org/113811/diff/5072/5080 File net/base/client_socket_factory.cc (right): http://codereview.chromium.org/113811/diff/5072/5080#newcode14 Line 14: #include "base/singleton.h" On 2009/06/19 07:02:49, eroman wrote: > you can probably remove "client_socket_factory.cc" from this changelist. Done. http://codereview.chromium.org/113811/diff/5072/5079 File net/base/client_socket_factory.h (right): http://codereview.chromium.org/113811/diff/5072/5079#newcode9 Line 9: #include "net/base/socks_client_socket.h" agreed. On 2009/06/19 07:02:49, eroman wrote: > no need for this. > > in fact, "client_socket_factor.h" can be removed from this CL now. http://codereview.chromium.org/113811/diff/5072/5082 File net/base/socks_client_socket.cc (right): http://codereview.chromium.org/113811/diff/5072/5082#newcode57 Line 57: resolver_(new HostResolver()) { I could not find how to pass this dependence from HttpNetworkTransaction. I believe the code is somewhere within the tcp connection initiated by http, but all that passes is a hostresolver::responseInfo. http://codereview.chromium.org/113811/diff/5072/5082#newcode97 Line 97: CompletionCallback* callback) { On 2009/06/19 07:02:49, eroman wrote: > line up the indentations. Done. http://codereview.chromium.org/113811/diff/5072/5082#newcode108 Line 108: CompletionCallback* callback) { On 2009/06/19 07:02:49, eroman wrote: > line up the indentations. Done. http://codereview.chromium.org/113811/diff/5072/5082#newcode120 Line 120: // since Run may result in Read being called, clear user_callback_ up front. On 2009/06/19 07:02:49, eroman wrote: > "since Run may result" ---> "Since Run() may result" Done. http://codereview.chromium.org/113811/diff/5072/5082#newcode165 Line 165: rv = ERR_FAILED; On 2009/06/19 07:02:49, eroman wrote: > how about: > > rv = ERR_UNEXPECTED; Done. http://codereview.chromium.org/113811/diff/5072/5082#newcode176 Line 176: // DidStartDnsResolution(hostname_, this); On 2009/06/19 07:02:49, eroman wrote: > You can delete this now. Done. http://codereview.chromium.org/113811/diff/5072/5082#newcode177 Line 177: return resolver_.Resolve(HostResolver::RequestInfo(hostname_, port_), On 2009/06/19 07:02:49, eroman wrote: > See comment made in the header file -- we want to use a RequestInfo that was > passed in by the caller (since it may contain the necessary "referrer" > information). Done. http://codereview.chromium.org/113811/diff/5072/5082#newcode178 Line 178: &addresses_, &io_callback_); On 2009/06/19 07:02:49, eroman wrote: > indent line continuations by 4 spaces. Done. http://codereview.chromium.org/113811/diff/5072/5082#newcode185 Line 185: // DidFinishDnsResolutionWithStatus(ok, referrer_, this); On 2009/06/19 07:02:49, eroman wrote: > You can delete this now. Done. http://codereview.chromium.org/113811/diff/5072/5082#newcode216 Line 216: int16 nport = port_; On 2009/06/19 07:02:49, eroman wrote: > please only 1 space around the equals. Done. http://codereview.chromium.org/113811/diff/5072/5082#newcode249 Line 249: bytes_sent_ = 0; On 2009/06/19 07:02:49, eroman wrote: > This doesn't really have to do with BuildHandshakeWRiteBuffer() -- how about > moving it to the caller? Done. http://codereview.chromium.org/113811/diff/5072/5082#newcode256 Line 256: if (!buffer_is_built_) On 2009/06/19 07:02:49, eroman wrote: > instead of needing this boolean, how about. > if (!buffer_.get()) Done. http://codereview.chromium.org/113811/diff/5072/5082#newcode259 Line 259: user_buf_len_ = buffer_len_ - bytes_sent_; On 2009/06/19 07:02:49, eroman wrote: > perhaps add a > DCHECK_GT(user_buf_len_, 0); > For good measure. Done. http://codereview.chromium.org/113811/diff/5072/5082#newcode271 Line 271: return ERR_FAILED; On 2009/06/19 07:02:49, eroman wrote: > return "result" instead. > > That way we propagate the reason of the error back up the stack, rather than > masking with the unhelpful "ERR_FAILED". Done. http://codereview.chromium.org/113811/diff/5072/5082#newcode276 Line 276: buffer_.reset(new char[kReadHeaderSize]); On 2009/06/19 07:02:49, eroman wrote: > I think these next three lines would be more appropriate inside of > DoHandshakeRead(). > > So what you could do instead is just set: > buffer_.reset(NULL); > > And then within DoHandshakeRead() you would follow a similar model to how you > lazy-call BuildHandshakeWriteBuffer(): > > DoHandshakeRead() { > > if (!buffer_.get()) { > // Starting the first read. > buffer_.reset(new char[kReadHeaderSize]); > buffer_len_ = kReadHeaderSize; > bytes_received_ = 0; > } > > ... > } Done. http://codereview.chromium.org/113811/diff/5072/5082#newcode295 Line 295: return transport_->Read(user_buf_, user_buf_len_, &io_callback_); On 2009/06/19 07:02:49, eroman wrote: > can you rename "user_buf" to something more like "handshake_buf" ? > > We generally use the naming "user_buf" for a buffer that was passed in by the > user (and we are filling for them). > > However in this case, it is just an internal buffer we are using to read the > handshake. Done. http://codereview.chromium.org/113811/diff/5072/5082#newcode304 Line 304: return ERR_FAILED; On 2009/06/19 07:02:49, eroman wrote: > return result; Done. http://codereview.chromium.org/113811/diff/5072/5082#newcode306 Line 306: return ERR_FAILED; On 2009/06/19 07:02:49, eroman wrote: > How about using ERR_INVALID_RESPONSE here instead. > (we try to keep error codes as descriptive as possible, since it makes > diagnosing problems easier). Done. http://codereview.chromium.org/113811/diff/5072/5082#newcode319 Line 319: return ERR_UNEXPECTED; On 2009/06/19 07:02:49, eroman wrote: > How about ERR_INVALID_RESPONSE. > > ERR_UNEXPECTED is reserved for code logic problems (think of it like an ASSERT() > gone bad). Done. http://codereview.chromium.org/113811/diff/5072/5082#newcode328 Line 328: return ERR_FAILED; On 2009/06/19 07:02:49, eroman wrote: > add a TODO for adding a more specific error code to net_error_list.h Done. http://codereview.chromium.org/113811/diff/5072/5082#newcode332 Line 332: return ERR_NAME_NOT_RESOLVED; On 2009/06/19 07:02:49, eroman wrote: > add a TODO for adding a more specific error code to net_error_list.h Done. http://codereview.chromium.org/113811/diff/5072/5082#newcode336 Line 336: return ERR_FAILED; On 2009/06/19 07:02:49, eroman wrote: > add a TODO for adding a more specific error code to net_error_list.h Done. http://codereview.chromium.org/113811/diff/5072/5082#newcode339 Line 339: return ERR_UNEXPECTED; On 2009/06/19 07:02:49, eroman wrote: > ERR_INVALID_RESPONSE Done. http://codereview.chromium.org/113811/diff/5072/5082#newcode344 Line 344: return ERR_FAILED; On 2009/06/19 07:02:49, eroman wrote: > You can probably delete these two lines, since the default: will catch all. Done. http://codereview.chromium.org/113811/diff/5072/5082#newcode350 Line 350: int SOCKSClientSocket::GetPeerName(struct sockaddr *name, socklen_t *namelen) { There were some issues with this. Something to do with failure to reinterpret SOCKSClientSocket to ClientSocket in linux since the GetPeerName() function was missing. http://codereview.chromium.org/113811/diff/5072/5081 File net/base/socks_client_socket.h (right): http://codereview.chromium.org/113811/diff/5072/5081#newcode15 Line 15: #include "net/base/client_socket.h" On 2009/06/19 07:02:49, eroman wrote: > please order these. for example: > > #include "net/base/host_resolver.h" > #include "net/base/client_socket.h" > #include "net/base/completion_callback.h" > > #include "net/base/net_errors.h" Done. http://codereview.chromium.org/113811/diff/5072/5081#newcode18 Line 18: #include "googleurl/src/gurl.h" On 2009/06/19 07:02:49, eroman wrote: > "googleurl" should be ordered after "base" and before "net" Done. http://codereview.chromium.org/113811/diff/5072/5081#newcode25 Line 25: On 2009/06/19 07:02:49, eroman wrote: > remove this empty line. Done. http://codereview.chromium.org/113811/diff/5072/5081#newcode29 Line 29: // |referrer| is the referrer for the request that first created On 2009/06/19 07:02:49, eroman wrote: > please remove the referrer field. > > i refactored the need for DidStartDnsResolution() in > <http://codereview.chromium.org/125107> > > So what you want to do now is replace the {hostname, port, referrer} arguments > by a single "const HostResolver::RequestInfo& info" parameter. > > This "info" parameter is what you will pass to HostResolver::Resolve(). > > You can extract the hostname and port with info.hostname(), info.port(). Done. http://codereview.chromium.org/113811/diff/5072/5081#newcode72 Line 72: enum State { On 2009/06/19 07:02:49, eroman wrote: > Please move this enum definition to the top of the "private:" section. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... Done. http://codereview.chromium.org/113811/diff/5072/5081#newcode79 Line 79: STATE_NONE On 2009/06/19 07:02:49, eroman wrote: > for consistency, end this with a comma (same as the SocksVersion enum). Done. http://codereview.chromium.org/113811/diff/5072/5081#newcode87 Line 87: enum SocksVersion { On 2009/06/19 07:02:49, eroman wrote: > Move this up. Done. http://codereview.chromium.org/113811/diff/5072/5081#newcode109 Line 109: // call this is false. As the user_buf_ is filled withe the write data, On 2009/06/19 07:02:49, eroman wrote: > typo: withe --> with. > > also i believe |buf_| is what contains the handshake write data. Done. http://codereview.chromium.org/113811/diff/5072/5081#newcode111 Line 111: bool buffer_is_built_; On 2009/06/19 07:02:49, eroman wrote: > I comment in the .cc file that this seems unecessary, can test buffer_.get() > instead. Done. http://codereview.chromium.org/113811/diff/5072/5081#newcode129 Line 129: virtual int GetPeerName(struct sockaddr *name, socklen_t *namelen); On 2009/06/19 07:02:49, eroman wrote: > move this into the public section (in the ClientSocket methods section). Done. http://codereview.chromium.org/113811/diff/5072/5081#newcode131 Line 131: }; On 2009/06/19 07:02:49, eroman wrote: > as the last statement in the private section, add: > > DISALLOW_COPY_AND_ASSIGN(SOCKSClientSocket); Done. http://codereview.chromium.org/113811/diff/5072/5075 File net/http/http_network_transaction_unittest.cc (right): http://codereview.chromium.org/113811/diff/5072/5075#newcode3053 Line 3053: EXPECT_FALSE(response == NULL); On 2009/06/19 07:02:49, eroman wrote: > Nice job with adding some tests! > These looks good. > > One final thing to do in this test, is do some verification on the contents of > |response|. > > To check the BODY (payload of the http response), you can write something like: > > std::string response_text; > rv = ReadTransaction(trans.get(), &response_text); > EXPECT_EQ(OK, rv); > EXPECT_EQ("Payload", response_text); > > Of course, you would additionally need to add a MockRead("Payload") entry as the > second-to-last element of |data_reads|. Done. http://codereview.chromium.org/113811/diff/5072/5075#newcode3073 Line 3073: MSVC_PUSH_DISABLE_WARNING(4309) On 2009/06/19 07:02:49, eroman wrote: > I suggest defining the array as unsigned char[] and avoiding these suppressions. > > And then casting to a char* if needed by MockWrite/MockRead. Done. http://codereview.chromium.org/113811/diff/5072/5075#newcode3107 Line 3107: EXPECT_FALSE(response == NULL); On 2009/06/19 07:02:49, eroman wrote: > Same comment as for other test -- please add a verification of the response. Done. http://codereview.chromium.org/113811/diff/5072/5078 File net/net.gyp (right): http://codereview.chromium.org/113811/diff/5072/5078#newcode123 Line 123: 'base/socks_client_socket.h', On 2009/06/19 07:02:49, eroman wrote: > please order alphabetically (this goes after "socket.h") Done. http://codereview.chromium.org/113811/diff/5072/5078#newcode426 Line 426: 'base/socks_client_socket_unittest.cc', On 2009/06/19 07:02:49, eroman wrote: > please order alphabetically (this goes after sdch_filter_unittest.cc). Done.
LGTM! (Please ready my last batch of comments though.) Let me know once you had addressed the final comments, and signed the contributor license agreement: <http://code.google.com/legal/individual-cla-v1.0.html> and I will add you to the AUTHORS file, and land this initial patch. I looking forward to reading the unit-tests next :) Also, in case you haven't already, try to test on linux while running "valgrind" to discover memory errors: $ cd src/tools/valgrind $ ./chrome_tests.sh -t net See <http://dev.chromium.org/developers/how-tos/using-valgrind> for more information. http://codereview.chromium.org/113811/diff/8001/9006 File net/base/socks_client_socket.cc (right): http://codereview.chromium.org/113811/diff/8001/9006#newcode8 Line 8: #include <ws2tcpip.h> delete http://codereview.chromium.org/113811/diff/8001/9006#newcode10 Line 10: #include <netdb.h> delete http://codereview.chromium.org/113811/diff/8001/9006#newcode14 Line 14: // #include "net/base/dns_resolution_observer.h" delete http://codereview.chromium.org/113811/diff/8001/9006#newcode115 Line 115: // since Run() may result in Read being called, Captitalize "since". http://codereview.chromium.org/113811/diff/8001/9005 File net/base/socks_client_socket.h (right): http://codereview.chromium.org/113811/diff/8001/9005#newcode55 Line 55: remove extra line. http://codereview.chromium.org/113811/diff/8001/9005#newcode114 Line 114: bool buffer_is_built_; this can be deleted now. http://codereview.chromium.org/113811/diff/8001/9001 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/113811/diff/8001/9001#newcode566 Line 566: DLOG(INFO) << " socks : " << using_socks_proxy_ I recommend removing this, since it will add a lot of noise to the logs. http://codereview.chromium.org/113811/diff/8001/9001#newcode647 Line 647: HostResolver::RequestInfo req_info(request_->url.HostNoBrackets(), This logic of constructing the HostResolver::RequestInfo could be moved to a function, say BuildHostResolverRequest(). That way it is not duplicated from DoInitConnection().
[adding some other people as FYI... speak now or forever hold your peace]
Glad to see this getting implemented, thanks for helping out! I've just got some stylistic nitpicks. http://codereview.chromium.org/113811/diff/8001/9006 File net/base/socks_client_socket.cc (right): http://codereview.chromium.org/113811/diff/8001/9006#newcode22 Line 22: static const char kUserId[] = ""; Perhaps it'd be clearer if you named this kEmptyUserId, so when people read the code below, it's obvious that the user-id is empty. http://codereview.chromium.org/113811/diff/8001/9006#newcode197 Line 197: void SOCKSClientSocket::BuildHandshakeWriteBuffer() { I wonder if this code would be clearer if you used a struct rather than writing bytes directly to the buffer? Perhaps do something like struct SOCKSWriteHeader { uint8 version; uint8 command_code; uint16 port; }; COMPILE_ASSERT(sizeof(SOCKSWriteHeader) == 32); to make sure the compiler isn't adding any strange padding. http://codereview.chromium.org/113811/diff/8001/9006#newcode197 Line 197: void SOCKSClientSocket::BuildHandshakeWriteBuffer() { As a member of Google's const-grouplet, I'm somewhat of a const nazi, but I think it'd be cleaner not to have side effects where possible. It seems like you're only modifying two things here: |buffer_| and |buffer_len_|. What if you made this member function const, ditched |buffer_len_|, changed |buffer_| to a std::string, and passed it in as an output param? http://codereview.chromium.org/113811/diff/8001/9006#newcode201 Line 201: int record_size = kWriteHeaderSize + arraysize(kUserId); Please include "base/basictypes.h" for the definition of this macro. http://codereview.chromium.org/113811/diff/8001/9006#newcode218 Line 218: buffer[3] = nport & 0xFF; This code doesn't seem very portable. You're assuming a byte ordering. You should probably use htons(). You never know what architecture people will want to run chromium on :) http://codereview.chromium.org/113811/diff/8001/9006#newcode224 Line 224: memcpy(&buffer[4], ai->ai_addr->sa_data + 2, 4); Rather than doing pointer arithmetic, why not cast the sockaddr into a sockaddr_in, and then use memcpy the in_addr? You're using a lot of magic numbers here which aren't necessarily immediately obvious. Using named constants or sizeof(struct in_addr) will help improve readability. http://codereview.chromium.org/113811/diff/8001/9006#newcode251 Line 251: handshake_buf_len_ = buffer_len_ - bytes_sent_; Why is |handshake_buf_len_| a member variable? It seems like it's calculated from |buffer_len_| and |bytes_sent_| each time. http://codereview.chromium.org/113811/diff/8001/9006#newcode255 Line 255: handshake_buf_len_); your formatting is off here. parameters should be aligned. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... http://codereview.chromium.org/113811/diff/8001/9006#newcode321 Line 321: switch (buffer[1]) { This switch statement isn't very clear to someone who hasn't memorized the SOCKS protocol or has a reference on hand. If you instead used a struct's field instead of buffer[1], a reader would immediately know this is the status field. Also, if you used named constants instead of 0x5A, etc. then readers would know what these magic numbers meant. http://codereview.chromium.org/113811/diff/8001/9006#newcode341 Line 341: // Note: we ignore the rest 6 bytes as specified by the SOCKS protocol Isn't this only ignored for SOCKS4 and not SOCKS4A? You should also s/rest/last/. http://codereview.chromium.org/113811/diff/8001/9005 File net/base/socks_client_socket.h (right): http://codereview.chromium.org/113811/diff/8001/9005#newcode26 Line 26: // Takes ownership of the transport_socket, which may already be connected. s/transport_socket/|transport_socket|/ Isn't it a problem if transport_socket isn't already connected? http://codereview.chromium.org/113811/diff/8001/9005#newcode29 Line 29: // |referrer| is the referrer for the request that first created Is this comment stale? I don't see |referer| anywhere. http://codereview.chromium.org/113811/diff/8001/9005#newcode36 Line 36: virtual ~SOCKSClientSocket(); It should be documented that the destructor calls Disconnect() on the socket. Actually, this is better documented in the ClientSocket interface itself, since it's true for all of our ClientSocket subtypes. http://codereview.chromium.org/113811/diff/8001/9005#newcode71 Line 71: SOCKS_4_UNRESOLVED, Please use kCamelCase enum names, as per the latest revision of the style guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Enumer... http://codereview.chromium.org/113811/diff/8001/9001 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/113811/diff/8001/9001#newcode145 Line 145: using_socks_proxy_(false), I'm sorry if this was already brought up, but have you considered using an enum instead of 3 bools? |using_proxy_|, |using_tunnel_|, and |using_socks_proxy_| are all disjoint. Also, |using_proxy_| is somewhat confusing now. Perhaps it should be called |using_http_proxy_| to distinguish it from a socks proxy. http://codereview.chromium.org/113811/diff/8001/9004 File net/net.gyp (right): http://codereview.chromium.org/113811/diff/8001/9004#newcode117 Line 117: 'base/socks_client_socket.h', .cc comes before .h.
http://codereview.chromium.org/113811/diff/8001/9001 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/113811/diff/8001/9001#newcode145 Line 145: using_socks_proxy_(false), On 2009/06/20 02:35:15, willchan wrote: > I'm sorry if this was already brought up, but have you considered using an enum > instead of 3 bools? |using_proxy_|, |using_tunnel_|, and |using_socks_proxy_| > are all disjoint. > > Also, |using_proxy_| is somewhat confusing now. Perhaps it should be called > |using_http_proxy_| to distinguish it from a socks proxy. Yeah, this has already come up. See: <http://codereview.chromium.org/113811/diff/45/1025#newcode286> I reckon some of these cleanups can be done as another pass.
A small change in this : When I am doing DNS resolution and the result is an IPv6 address, I am switching over to SOCKS4a (letting server resolve hostname) since SOCKS4/4a has no support for IPv6. Other comments have been replied to. I plan to revise the |using_socks_proxy_|, etc. on another pass. http://codereview.chromium.org/113811/diff/8001/9006 File net/base/socks_client_socket.cc (right): http://codereview.chromium.org/113811/diff/8001/9006#newcode8 Line 8: #include <ws2tcpip.h> I need this to resolve struct addrinfo http://codereview.chromium.org/113811/diff/8001/9006#newcode10 Line 10: #include <netdb.h> I need this to resolve struct addrinfo http://codereview.chromium.org/113811/diff/8001/9006#newcode14 Line 14: // #include "net/base/dns_resolution_observer.h" On 2009/06/20 00:27:40, eroman wrote: > delete Done. http://codereview.chromium.org/113811/diff/8001/9006#newcode22 Line 22: static const char kUserId[] = ""; On 2009/06/20 02:35:15, willchan wrote: > Perhaps it'd be clearer if you named this kEmptyUserId, so when people read the > code below, it's obvious that the user-id is empty. Done. http://codereview.chromium.org/113811/diff/8001/9006#newcode115 Line 115: // since Run() may result in Read being called, On 2009/06/20 00:27:40, eroman wrote: > Captitalize "since". Done. http://codereview.chromium.org/113811/diff/8001/9006#newcode197 Line 197: void SOCKSClientSocket::BuildHandshakeWriteBuffer() { a |buffer_| has many '\0' in it (For example the invalid IP has { 0, 0, 0, 127 }), and std::string would terminate it much before than the |buffer_len_|. http://codereview.chromium.org/113811/diff/8001/9006#newcode197 Line 197: void SOCKSClientSocket::BuildHandshakeWriteBuffer() { The idea is nice, but SOCKSWriteHeader has variable size, especially when it sends the domain name unresolved in the SOCKS4a extension. In that case COMPILE_ASSERT will not work. Or maybe are you suggesting to set the constant part (version, comm_code, port, ip) to a SOCKSWriteHeader and COMPILE_ASSERT its size ? http://codereview.chromium.org/113811/diff/8001/9006#newcode201 Line 201: int record_size = kWriteHeaderSize + arraysize(kUserId); On 2009/06/20 02:35:15, willchan wrote: > Please include "base/basictypes.h" for the definition of this macro. Done. http://codereview.chromium.org/113811/diff/8001/9006#newcode218 Line 218: buffer[3] = nport & 0xFF; I think GURL returns a big-endian (n/w ordered) port on calling url.EffectiveIntPort() .. atleast htons() keeps reversing it on my x86. http://codereview.chromium.org/113811/diff/8001/9006#newcode224 Line 224: memcpy(&buffer[4], ai->ai_addr->sa_data + 2, 4); On 2009/06/20 02:35:15, willchan wrote: > Rather than doing pointer arithmetic, why not cast the sockaddr into a > sockaddr_in, and then use memcpy the in_addr? You're using a lot of magic > numbers here which aren't necessarily immediately obvious. Using named > constants or sizeof(struct in_addr) will help improve readability. Done. http://codereview.chromium.org/113811/diff/8001/9006#newcode251 Line 251: handshake_buf_len_ = buffer_len_ - bytes_sent_; I do that to maintain consistency. Since everywhere I am storing the buffer + size, I figured I should do the same here too. http://codereview.chromium.org/113811/diff/8001/9006#newcode255 Line 255: handshake_buf_len_); On 2009/06/20 02:35:15, willchan wrote: > your formatting is off here. parameters should be aligned. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... Done. http://codereview.chromium.org/113811/diff/8001/9006#newcode321 Line 321: switch (buffer[1]) { On 2009/06/20 02:35:15, willchan wrote: > This switch statement isn't very clear to someone who hasn't memorized the SOCKS > protocol or has a reference on hand. If you instead used a struct's field > instead of buffer[1], a reader would immediately know this is the status field. > Also, if you used named constants instead of 0x5A, etc. then readers would know > what these magic numbers meant. Done. http://codereview.chromium.org/113811/diff/8001/9006#newcode341 Line 341: // Note: we ignore the rest 6 bytes as specified by the SOCKS protocol This is ignored for both socks4 and socks4a. > s/rest/last/. Done. http://codereview.chromium.org/113811/diff/8001/9005 File net/base/socks_client_socket.h (right): http://codereview.chromium.org/113811/diff/8001/9005#newcode26 Line 26: // Takes ownership of the transport_socket, which may already be connected. On 2009/06/20 02:35:15, willchan wrote: > s/transport_socket/|transport_socket|/ > Isn't it a problem if transport_socket isn't already connected? Done. http://codereview.chromium.org/113811/diff/8001/9005#newcode29 Line 29: // |referrer| is the referrer for the request that first created On 2009/06/20 02:35:15, willchan wrote: > Is this comment stale? I don't see |referer| anywhere. Done. http://codereview.chromium.org/113811/diff/8001/9005#newcode36 Line 36: virtual ~SOCKSClientSocket(); On 2009/06/20 02:35:15, willchan wrote: > It should be documented that the destructor calls Disconnect() on the socket. > Actually, this is better documented in the ClientSocket interface itself, since > it's true for all of our ClientSocket subtypes. Done. http://codereview.chromium.org/113811/diff/8001/9005#newcode55 Line 55: On 2009/06/20 00:27:40, eroman wrote: > remove extra line. Done. http://codereview.chromium.org/113811/diff/8001/9005#newcode71 Line 71: SOCKS_4_UNRESOLVED, On 2009/06/20 02:35:15, willchan wrote: > Please use kCamelCase enum names, as per the latest revision of the style guide: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Enumer... Done. http://codereview.chromium.org/113811/diff/8001/9005#newcode114 Line 114: bool buffer_is_built_; On 2009/06/20 00:27:40, eroman wrote: > this can be deleted now. Done. http://codereview.chromium.org/113811/diff/8001/9001 File net/http/http_network_transaction.cc (right): http://codereview.chromium.org/113811/diff/8001/9001#newcode566 Line 566: DLOG(INFO) << " socks : " << using_socks_proxy_ On 2009/06/20 00:27:40, eroman wrote: > I recommend removing this, since it will add a lot of noise to the logs. Done. http://codereview.chromium.org/113811/diff/8001/9001#newcode647 Line 647: HostResolver::RequestInfo req_info(request_->url.HostNoBrackets(), I think we should add it as a constructor to HostResolver. But maybe as another CL since it would require us to add another 2 files to this CL ? http://codereview.chromium.org/113811/diff/8001/9004 File net/net.gyp (right): http://codereview.chromium.org/113811/diff/8001/9004#newcode117 Line 117: 'base/socks_client_socket.h', On 2009/06/20 02:35:15, willchan wrote: > .cc comes before .h. Done.
Sorry for missing the note about using_proxy_, there was a lot of previous context I didn't read. Sorry if I commented on anything else that had already been discussed and decided. http://codereview.chromium.org/113811/diff/8001/9006 File net/base/socks_client_socket.cc (right): http://codereview.chromium.org/113811/diff/8001/9006#newcode197 Line 197: void SOCKSClientSocket::BuildHandshakeWriteBuffer() { On 2009/06/20 11:42:55, Arindam wrote: > a |buffer_| has many '\0' in it (For example the invalid IP has { 0, 0, 0, 127 > }), and std::string would terminate it much before than the |buffer_len_|. http://www.sgi.com/tech/stl/basic_string.html says: "Characters within a string are permitted to be null." http://codereview.chromium.org/113811/diff/8001/9006#newcode197 Line 197: void SOCKSClientSocket::BuildHandshakeWriteBuffer() { On 2009/06/20 11:42:55, Arindam wrote: > The idea is nice, but SOCKSWriteHeader has variable size, especially when it > sends the domain name unresolved in the SOCKS4a extension. In that case > COMPILE_ASSERT will not work. > > Or maybe are you suggesting to set the constant part (version, comm_code, port, > ip) to a SOCKSWriteHeader and COMPILE_ASSERT its size ? Yep! http://codereview.chromium.org/113811/diff/8001/9006#newcode218 Line 218: buffer[3] = nport & 0xFF; On 2009/06/20 11:42:55, Arindam wrote: > I think GURL returns a big-endian (n/w ordered) port on calling > url.EffectiveIntPort() .. atleast htons() keeps reversing it on my x86. Sorry, I misread it, and didn't notice your comment above that it's already in network order! My bad. http://codereview.chromium.org/113811/diff/8001/9006#newcode251 Line 251: handshake_buf_len_ = buffer_len_ - bytes_sent_; On 2009/06/20 11:42:55, Arindam wrote: > I do that to maintain consistency. Since everywhere I am storing the buffer + > size, I figured I should do the same here too. > > I think it makes sense to calculate it, but it seems like you could make it a local variable each time. http://codereview.chromium.org/113811/diff/8001/9006#newcode341 Line 341: // Note: we ignore the rest 6 bytes as specified by the SOCKS protocol On 2009/06/20 11:42:55, Arindam wrote: > This is ignored for both socks4 and socks4a. > > s/rest/last/. > Done. > > Hm, http://en.wikipedia.org/wiki/SOCKS says that it contains the port and ip address in socks4a, but I don't see that corroborated anywhere else, nor can I think of a good reason to use this information, so ok, it's fine as is. Maybe I shouldn't trust Wikipedia so much :P http://codereview.chromium.org/113811/diff/8012/9023#newcode33 Line 33: static const int kReadHeaderSize = 8; I'd add a COMPILE_ASSERT(sizeof(SOCKS4ServerResponse) == kReadHeaderSize, socks4_server_response_struct_wrong_size); to make sure the compiler isn't inserting any padding. You can't access the private struct from here, so you'll have to put the COMPILE_ASSERT in some member function (probably the DoHandshakeRead() makes the most sense) or move the struct into the .cc file instead of leaving it as a private type. I prefer the latter, since imho, trying to keep implementation details out of the header file is nice. In case you have to modify it later on, clients don't have to recompile. Although if you need it share it for a test, then it has to be in the header. http://codereview.chromium.org/113811/diff/8012/9023#newcode319 Line 319: Really nitpicky: s/kSOCKS4UnResolved/kSOCKS4Unresolved/.
http://codereview.chromium.org/113811/diff/8001/9006 File net/base/socks_client_socket.cc (right): http://codereview.chromium.org/113811/diff/8001/9006#newcode197 Line 197: void SOCKSClientSocket::BuildHandshakeWriteBuffer() { On 2009/06/20 16:41:59, willchan wrote: > On 2009/06/20 11:42:55, Arindam wrote: > > The idea is nice, but SOCKSWriteHeader has variable size, especially when it > > sends the domain name unresolved in the SOCKS4a extension. In that case > > COMPILE_ASSERT will not work. > > > > Or maybe are you suggesting to set the constant part (version, comm_code, > port, > > ip) to a SOCKSWriteHeader and COMPILE_ASSERT its size ? > > Yep! Done. http://codereview.chromium.org/113811/diff/8001/9006#newcode197 Line 197: void SOCKSClientSocket::BuildHandshakeWriteBuffer() { Did not know about std::string supporting '\0'. Tried it but had issues with const_casts and type castings on the string data ... I think I should implement it in the follow-up CL which should include other refactorings. http://codereview.chromium.org/113811/diff/8001/9006#newcode218 Line 218: buffer[3] = nport & 0xFF; actually, you were right. it needs the htons(), but buffer[2] = (nport >> 8) & 0xFF; buffer[3] = nport & 0xFF; replicates the same conversion for x86 and hence I never got an error. I fixed that in the newer patch. Done. http://codereview.chromium.org/113811/diff/8012/9023#newcode33 Line 33: static const int kReadHeaderSize = 8; On 2009/06/20 16:41:59, willchan wrote: > I'd add a COMPILE_ASSERT(sizeof(SOCKS4ServerResponse) == kReadHeaderSize, > socks4_server_response_struct_wrong_size); to make sure the compiler isn't > inserting any padding. > You can't access the private struct from here, so you'll have to put the > COMPILE_ASSERT in some member function (probably the DoHandshakeRead() makes the > most sense) or move the struct into the .cc file instead of leaving it as a > private type. I prefer the latter, since imho, trying to keep implementation > details out of the header file is nice. In case you have to modify it later on, > clients don't have to recompile. Although if you need it share it for a test, > then it has to be in the header. Done. http://codereview.chromium.org/113811/diff/8012/9023#newcode319 Line 319: On 2009/06/20 16:41:59, willchan wrote: > Really nitpicky: s/kSOCKS4UnResolved/kSOCKS4Unresolved/. Done.
lgtm! http://codereview.chromium.org/113811/diff/8001/9006 File net/base/socks_client_socket.cc (right): http://codereview.chromium.org/113811/diff/8001/9006#newcode197 Line 197: void SOCKSClientSocket::BuildHandshakeWriteBuffer() { On 2009/06/21 00:18:28, Arindam wrote: > Did not know about std::string supporting '\0'. Tried it but had issues with > const_casts and type castings on the string data ... I think I should implement > it in the follow-up CL which should include other refactorings. Ah yes. I guess it might involve some evil casts to get this to work with std::string. I'm fine with punting it to a followup cl. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
