[opensource-dev] Review Request: VWR-20801 Implement SOCKS 5 Proxy for the viewer
Monty Brandenberg
monty at lindenlab.com
Fri Apr 1 11:38:02 PDT 2011
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/232/#review533
-----------------------------------------------------------
indra/llmessage/llcurl.cpp
<http://codereview.secondlife.com/r/232/#comment425>
General comment about a need for consistent white space, comma style, etc.
Question: can this block be removed and the proxy setup allowed to occur in prepRequest()? That seems to be a required invocation.
We'll probably need to release note some of the
issues with libcurl and proxies:
http://curl.haxx.se/docs/knownbugs.html
65, 35, 34, 23 and 12 can apply. 35 (blocking connections) may be the most significant at the moment.
indra/llmessage/llpacketring.cpp
<http://codereview.secondlife.com/r/232/#comment426>
Style point (open to argument, should be in our coding standards):
System headers before app headers. One exception for very correct coding is to have the module interface header first:
#include "linden_common.h"
#include "llpacketring.h"
#include <...>
#include "ll...."
indra/llmessage/llpacketring.cpp
<http://codereview.secondlife.com/r/232/#comment427>
Correct define for buffer is 'NET_BUFFER_SIZE' rather than 'MAX_BUFFER_SIZE'.
I'm soon to go on an internal rampage about c-style casts. Lindens use them lazily and heedlessly. These need to stop and you can help. Correct types where possible, most specific c++-style cast where not possible. C-style cast really should never be necessary.
The '10' bothers me as a magic number.
Four bytes are required before the length
of the header is known. Or an ATYP check
after accepting 10 bytes.
Question: are there race conditions in here with respect to toggling the proxy
enable? Delayed state around negotiating
the proxy information (dns name resolution,
say) or other problems of that type?
indra/llmessage/llsocks5.h
<http://codereview.secondlife.com/r/232/#comment429>
Negative constants should generally be put
in parens ('(-2)') to avoid unintended
expressions.
indra/llmessage/llsocks5.h
<http://codereview.secondlife.com/r/232/#comment430>
On #define names.... these are inviting
collision with other names given their
generic prefixes. They're fine for
internal constants, less so for API but
we can probably live with this.
indra/llmessage/llsocks5.h
<http://codereview.secondlife.com/r/232/#comment428>
'octects' -> 'octets'
indra/llmessage/llsocks5.h
<http://codereview.secondlife.com/r/232/#comment438>
Not a 'flag' field in the protocol but a 'reserved' field, correct?
indra/llmessage/llsocks5.h
<http://codereview.secondlife.com/r/232/#comment439>
Ditto. Also a 'reserved' field.
indra/llmessage/llsocks5.h
<http://codereview.secondlife.com/r/232/#comment445>
I don't see a dtor(). I think there
should be one and it should stop any
running proxy explicitly.
indra/llmessage/llsocks5.h
<http://codereview.secondlife.com/r/232/#comment433>
Are arguments convertible to 'const &'?
indra/llmessage/llsocks5.h
<http://codereview.secondlife.com/r/232/#comment431>
Final semi-colon not needed.
const method.
(These occur again but I won't repeat myself.)
indra/llmessage/llsocks5.h
<http://codereview.secondlife.com/r/232/#comment432>
Style: naming convention: initialLowerStudlyCapsMethodNames()
indra/llmessage/llsocks5.cpp
<http://codereview.secondlife.com/r/232/#comment434>
These initializations should probably move up into the static definitions. Or maybe
in both. Hmmm...
(I'm also a fan of initializers rather than assignments for plain-old-data given in declaration order but that's me.)
indra/llmessage/llsocks5.cpp
<http://codereview.secondlife.com/r/232/#comment440>
Should use INVALID_HANDLE or whatever instead of 0?
indra/llmessage/llsocks5.cpp
<http://codereview.secondlife.com/r/232/#comment435>
I like the most direct association between
a buffer and a buffer size. Here, the
programmer is maintaining the relationship
between 'socks_auth_request' and
'socks_auth_request_t'. Let the compiler
do it with sizeof(socks_auth_request).
indra/llmessage/llsocks5.cpp
<http://codereview.secondlife.com/r/232/#comment437>
Hasn't the viewer converted over to using
the LL_WARNS/LL_WARNS2/LL_ENDL macro style
of logging? I know the once common code
lags behind this but it's okay to do the
newer thing here.
indra/llmessage/llsocks5.cpp
<http://codereview.secondlife.com/r/232/#comment436>
Since we're doing c++ here, new char [] and
delete [] may be in order.
indra/llmessage/llsocks5.cpp
<http://codereview.secondlife.com/r/232/#comment446>
Does this actually have *anything* to do
with NAT? This seems useful for bouncing
from sim host to sim host (not requiring
a new proxy setup) but should be nat-
transparent due to udp header rewriting.
indra/llmessage/llsocks5.cpp
<http://codereview.secondlife.com/r/232/#comment442>
Any new attempt to startProxy() basically
has an assumption that we're going to
stop any existing proxy first. So why
not just call stopProxy() here? Might be
cleaner/more correct.
indra/llmessage/llsocks5.cpp
<http://codereview.secondlife.com/r/232/#comment443>
Note that as currently written, a -1
value would test true in all the
'if (hProxyControlChannel)' code.
indra/llmessage/llsocks5.cpp
<http://codereview.secondlife.com/r/232/#comment447>
If the handshake fails, should we call
stopProxy() to cleanup the handle and
other bits?
indra/llmessage/llsocks5.cpp
<http://codereview.secondlife.com/r/232/#comment444>
There is something 'not right' with having
two startProxy() methods, I think. I'm
not certain what the fix is.
indra/llmessage/llsocks5.cpp
<http://codereview.secondlife.com/r/232/#comment441>
INVALID_HANDLE and previous 'if' check here.
indra/llmessage/net.h
<http://codereview.secondlife.com/r/232/#comment448>
Should we be doing this or using lliosocket
or apr socket abstraction?
indra/llmessage/net.cpp
<http://codereview.secondlife.com/r/232/#comment449>
Not the correct test. INVALID_HANDLE.
indra/llmessage/net.cpp
<http://codereview.secondlife.com/r/232/#comment450>
Leaks 'handle' on return.
indra/llmessage/net.cpp
<http://codereview.secondlife.com/r/232/#comment451>
Also leaks 'handle'
indra/llmessage/net.cpp
<http://codereview.secondlife.com/r/232/#comment452>
dataout not const
No EINTR or partial read/write processing.
(apr or lliosocket may help)
indra/llmessage/net.cpp
<http://codereview.secondlife.com/r/232/#comment453>
Incorrect test. -1 is the test value.
indra/llmessage/net.cpp
<http://codereview.secondlife.com/r/232/#comment454>
Always bugged me to no end that we have
one datum in host format (port) and the
other in network format (addr). Why do
we do this to ourselves?
indra/llmessage/net.cpp
<http://codereview.secondlife.com/r/232/#comment456>
Leaks 'handle' on error.
indra/llmessage/net.cpp
<http://codereview.secondlife.com/r/232/#comment455>
Absolutely no code should use select().
The standard compile-time limit on fd_set
is 1024 descriptors. This will cause
problems in common code if used in the
wrong way. Just use poll() and be done
with it.
select() returns int anyway. Shouldn't
be made into a U32. Can return -1, all
syscalls need EINTR processing, etc.
indra/llmessage/net.cpp
<http://codereview.secondlife.com/r/232/#comment457>
Leaks 'handle' on error.
- Monty
On March 28, 2011, 4:46 a.m., Robin Cornelius wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/232/
> -----------------------------------------------------------
>
> (Updated March 28, 2011, 4:46 a.m.)
>
>
> Review request for Viewer.
>
>
> Summary
> -------
>
> VWR-20801 - Add ability to use SOCKS 5 proxy to the viewer. This allows the UDP and/or the http requests to be sent via a SOCKS 5 proxy. This also allows http proxies to be used for other http operations such as caps etc as required. All the proxy settings have been unified on a single proxy floater accessable from preferences.
>
>
> This addresses bug VWR-20801.
> http://jira.secondlife.com/browse/VWR-20801
>
>
> Diffs
> -----
>
> indra/llmessage/CMakeLists.txt 65ff7415f171
> indra/llmessage/llcurl.cpp 65ff7415f171
> indra/llmessage/llpacketring.h 65ff7415f171
> indra/llmessage/llpacketring.cpp 65ff7415f171
> indra/llmessage/llsocks5.h PRE-CREATION
> indra/llmessage/llsocks5.cpp PRE-CREATION
> indra/llmessage/net.h 65ff7415f171
> indra/llmessage/net.cpp 65ff7415f171
> indra/newview/app_settings/settings.xml 65ff7415f171
> indra/newview/llfloaterpreference.h 65ff7415f171
> indra/newview/llfloaterpreference.cpp 65ff7415f171
> indra/newview/llstartup.h 65ff7415f171
> indra/newview/llstartup.cpp 65ff7415f171
> indra/newview/llviewerfloaterreg.cpp 65ff7415f171
> indra/newview/llxmlrpctransaction.cpp 65ff7415f171
> indra/newview/skins/default/xui/en/floater_preferences_proxy.xml PRE-CREATION
> indra/newview/skins/default/xui/en/notifications.xml 65ff7415f171
> indra/newview/skins/default/xui/en/panel_preferences_setup.xml 65ff7415f171
>
> Diff: http://codereview.secondlife.com/r/232/diff
>
>
> Testing
> -------
>
> Verified login and in world interaction with proxy disabled, verified login and in world interactionvia socks 5 proxy. Code has been tested on Windows very recently and has also worked fine on linux, but i'm not currently in a position to retest that or Mac at all. Much more testing is needed to verify this does not break anything unexpectedly and also works as expected when enabled. To test requires a working socks 5 proxy.
>
>
> Thanks,
>
> Robin
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20110401/9481e14c/attachment-0001.htm
More information about the opensource-dev
mailing list