STORM-1112 Support SOCKS 5 proxy in the viewer (take 2)
Review Request #374 - Created June 29, 2011 and submitted
| Log Linden | Reviewers | ||
| http://bitbucket.org/log_linden/viewer-socks5 | viewer | ||
| STORM-1112 | oz.linden, Monty.Linden, stone.linden | ||
| None | viewer-development | ||
This is a continuation of Robin Cornelius's SOCKS 5 contribution, shown in https://codereview.secondlife.com/r/232/ . I have tried to address all of the comments on that code review and do as much cleanup as possible. The diff includes everything that was submitted by Robin, as well as my work. Major changes since I started working: * Changed SOCKS 5 proxy control channel to use the existing LLSocket class, which is a thin wrapper around APR sockets. * Worked with the Linden Lab UX team to revamp the proxy controls. * Proxy credentials are now stored in the LLSecAPI password storage, which is the same that is used for users' Second Life Credentials instead of as being stored in the clear as a preference.
I've tested exclusively on Linux so far. I'm working on a more extensive test plan that includes setting up a gateway with a restrictive firewall to verify that all traffic is going through the proxy. Test builds and screenshots of the changed UI elements are available from the project page, located here: https://wiki.secondlife.com/wiki/User:Log_Linden/Socks5Viewer
Expand AllReview request changed
Updated (July 8, 2011, 11:55 a.m.)
-
- added Diff r3
Change the logging macros in llproxy.cpp to use the new LL_WARNS() style. Disabled the proxy by default.
Posted (July 10, 2011, 12:55 a.m.)
It looks like you have the name= the same for the "Proxy Settings:" text as for "Software updates:" in the panel_preferences_setup.xml file. These need to be different for translations to work.
Review request changed
Updated (July 12, 2011, 11:20 a.m.)
- changed from Support SOCKS 5 proxy in the viewer. to STORM-1112 Support SOCKS 5 proxy in the viewer (take 2)
Posted (July 12, 2011, 2:35 p.m.)
-
indra/llmessage/llcurl.cpp (Diff revision 3) -
Correct whitespace.
-
indra/llmessage/llpacketring.h (Diff revision 3) -
A few things mostly on coding style here: 1. You'll mostly see method declarations separate from member declarations. It's often due to access controls but it's also a good organizational thing. I'd move the method up here. 2. This is really a private/protected method. No external user should call this. Making it so is compatible with 1. 3. Terrible name. SendPacket and doSendPacket. This is some sort of sendPacketImpl or routePacket, I think.
-
indra/llmessage/llpacketring.cpp (Diff revision 3) -
With the proxy code, we could get data larger than NET_BUFFER_SIZE. We don't detect this condition in receive_packet. Might just log initially. So much wrong with it....
-
indra/llmessage/llpacketring.cpp (Diff revision 3) -
Magic number from original code review. Need these definitions (10) in the socks protocol definition set.
-
indra/llmessage/llpacketring.cpp (Diff revision 3) -
Not necessary now but something for a follow-on jira. This would have been a good place for a scatter/gather i/o using recvmsg to avoid the data copying.
-
indra/llmessage/llpacketring.cpp (Diff revision 3) -
Isn't packet_size '10' too large at this point?
-
indra/llmessage/llpacketring.cpp (Diff revision 3) -
I just looked into get_sender() and get_receiving_interface(). *weep* They use statics. So no thread safety. If we do improve the receive_packet interface to ever to scatter/gather, *this* will go.
-
indra/llmessage/llpacketring.cpp (Diff revision 3) -
Another perfect opportunity for scatter/gather i/o.
-
indra/llmessage/llpacketring.cpp (Diff revision 3) -
Caller may still reasonably try to write a full NET_BUFFER_SIZE of data in which case this smashes memory.
Posted (July 12, 2011, 11:50 p.m.)
This finishes the first-pass review. Not ready to ship yet.
-
indra/llmessage/llproxy.h (Diff revision 3) -
Extra semicolon.
-
indra/llmessage/llproxy.cpp (Diff revision 3) -
Prefer sizeof(password_reply), it avoids an unverifiable duplicated name/type binding.
-
indra/llmessage/llproxy.cpp (Diff revision 3) -
The security minded would do several things at this point: 1. Scribble over password_auth before deleting it. 2. Scribble over mSocksPassword now that we've used it once. 3. Narrow the window from retrieving the password to this point as much as is practical.
-
indra/llmessage/llproxy.cpp (Diff revision 3) -
See above.
-
indra/llmessage/llproxy.cpp (Diff revision 3) -
Whitespace.
-
indra/llmessage/llproxy.cpp (Diff revision 3) -
Is anyone doing validation of the strings? What happens if they're over 255 bytes? Invalid utf-8 encodings? Do we care? Are these in the test plan?
-
indra/llmessage/llproxy.cpp (Diff revision 3) -
This block isn't reachable due to preceding 'if' clause. Also leaves 'rv' set to success if it failed to write all data for some reason.
-
indra/llmessage/llproxy.cpp (Diff revision 3) -
Possible to have legal short reads here.
-
indra/llmessage/net.cpp (Diff revision 3) -
Does net.cpp really need llproxy.h? I don't think it should.
-
indra/newview/app_settings/settings.xml (Diff revision 3) -
Is this still storing username and password in the settings file? Add this to the test plan.
-
indra/newview/llfloaterpreference.cpp (Diff revision 3) -
Whitespace (indentation & if) Also, this thing is now 'prefs_proxy', yes?
-
indra/newview/llfloaterpreference.cpp (Diff revision 3) -
whitespace
-
indra/newview/llfloaterpreference.cpp (Diff revision 3) -
Defensive programming on socksAuth being NULL?
-
indra/newview/llstartup.cpp (Diff revision 3) -
This may be too late. GPU/features table has already been loaded by HTTP, I believe. Then there's webkit and our browser. Need to check on this. (And test plan it.)
-
indra/newview/llstartup.cpp (Diff revision 3) -
There are a bunch of common strings shared between this and the UI code ("Web", "BrowserProxyEnabled", etc.) that feel like they should be #defines somewhere. Somewhere not llproxy.h but application/ ui/interface oriented. -
indra/newview/llstartup.cpp (Diff revision 3) -
Left over from the previous review, I think, here's a place where the logic can be unsynchronized. 'use_socks_proxy' is actually identical to the 'else if' test above, isn't it? These should be consistent if I'm reading it correctly.
-
indra/newview/llstartup.cpp (Diff revision 3) -
All the credential code wants its own home, doesn't it?
-
indra/newview/llstartup.cpp (Diff revision 3) -
What does 'payload' do?
-
indra/newview/llstartup.cpp (Diff revision 3) -
I'd put this early return after startProxy(). We're in notification code here, long after we know we're okay. And what happens with an unrecognized status code?
-
indra/newview/llxmlrpctransaction.cpp (Diff revision 3) -
These combined with 328 feel like they should be a utility method on mCurlRequest. Or the whole block a method or free function in LLProxy...
-
indra/newview/llxmlrpctransaction.cpp (Diff revision 3) -
Whitespace.
-
indra/newview/skins/default/xui/en/panel_preferences_setup.xml (Diff revision 3) -
Bad name?
-
indra/newview/skins/default/xui/en/panel_preferences_setup.xml (Diff revision 3) -
Bad 'label_selected' value?
Review request changed
Updated (July 13, 2011, 1:50 p.m.)
-
- added Diff r4
Implemented changes to address Monty's first group of comments, as well of some of the trivial comments in the second group. Working on more fixes for the second group..
Review request changed
Updated (July 26, 2011, 10:20 a.m.)
-
- added Diff r5
Added proxy support to other places libcurl is used in the viewer. This is still a WIP, but I wanted to update to match what is available on bitbucket.
Review request changed
Updated (July 28, 2011, 11:06 a.m.)
-
- added Diff r6
More cleanup to address Monty's comments. More fixes to come.
Review request changed
Updated (Aug. 25, 2011, 11:15 a.m.)
-
- added Diff r7
* Improved threading code * Lots of cleanup and bugfixing.
Posted (Aug. 30, 2011, 4:53 p.m.)
-
indra/llcommon/llsingleton.h (Diff revision 7) -
why are we getting the data from within the destructor of the singleton itself? Why not just do if (mInitState != DELETED) instead? Seems like it would be less confusing.
-
indra/llmessage/llcurl.h (Diff revision 7) -
maybe a little comment as to what "Easy" is in this context. I know you just moved the code to the header, but now that it is more than an implementation detail of LLCurl, I think our expectation of documentation has increased.
-
indra/llmessage/llcurl.cpp (Diff revision 7) -
if time_out is unused now, let's remove it from the signature
-
indra/llmessage/llproxy.h (Diff revision 7) -
do all these #defines and socks structs need to be in the header?
-
indra/llmessage/llproxy.h (Diff revision 7) -
I think we need something a bit more obvious to call out locking methods, maybe just a heavier comment header?
-
indra/llmessage/llproxy.cpp (Diff revision 7) -
control-flow wise, it might be clearer to move the final check for status to a separate if, with appropriate comment along the lines of "if any of the above failed..."
-
indra/llmessage/llproxy.cpp (Diff revision 7) -
can we reflect the potentially long blocking period in the name of the method? tcp_wait_for_handshake or similar?
Other reviews