Web2socket proxy.

Web2socket proxy.

Proxy that listens for websocket connections, reads hostname:port, initiates new plain connection to hostname:port, and forwards all subsequent communication.

BUG=chromium-os:9667
TEST=Manual

[Nikita (slow), 2010-12-01 20:22:46]

Even though it's not ready yet, we should find initial reviewer.

[Yuta Kitamura, 2010-12-02 06:09:20]

Maybe someone from WebSocket team can help to get this patch reviewed.

Note that the WebSocket protocol is still under active discussion and likely to
change. Currently I'm writing the new framing protocol based on hybi draft 03,
and probably some changes to the server will be needed when my change is
committed to WebKit/Chromium.

[Denis Lagno, 2010-12-02 09:35:16]

CCing pfeldman@, author of net/server

[tyoshino (SeeGerritForStatus), 2010-12-03 07:53:33]

Took a quick look only at WebSocket protocol processing part.

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
File net/web2socket_proxy/web2socket.h (right):

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
net/web2socket_proxy/web2socket.h:5: #ifndef
CHROME_WEB2SOCKET_PROXY_WEB2SOCKET_H_
#ifndef NET_WEB2SOCKET_PROXY_WEB2SOCKET_H_

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
File net/web2socket_proxy/web2socket_conn.cc (right):

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
net/web2socket_proxy/web2socket_conn.cc:29: rv += (isascii(s[i]) &&
isspace(s[i]));
If you don't have to keep this code independent from Chromium tree, how about
using functions defined in base/string_util.h that are locale independent
instead of C library?

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
net/web2socket_proxy/web2socket_conn.cc:40: rv += tolower(*begin);
ditto. ToLowerASCII

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
net/web2socket_proxy/web2socket_conn.cc:75: if (!isascii(*pos) ||
!isdigit(*pos)) {
How about isascii(*pos) && isdigit(*pos)?
But if so, *pos will be never ':' below.

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
net/web2socket_proxy/web2socket_conn.cc:86: if (*port <= 0 || *port >= (2 <<
16)) {
Up to your design, but how about handling erroneous port number (>= 2^16) as
failure.

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
net/web2socket_proxy/web2socket_conn.cc:185: uint8_t key3[8]; // Notation (key3)
matches websocket RFC.
Two space before inline comment

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
net/web2socket_proxy/web2socket_conn.cc:191: memcpy(key3, term_pos -
sizeof(key3), sizeof(key3));
how about swapping these lines to remove "- sizeof(key3)"

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
net/web2socket_proxy/web2socket_conn.cc:283: snprintf(buf, sizeof(buf),
check return value?

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
net/web2socket_proxy/web2socket_conn.cc:358: if (buf_size < 1) {
We can remove if-clause?

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
File net/web2socket_proxy/web2socket_serv.cc (right):

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
net/web2socket_proxy/web2socket_serv.cc:9: #include <string.h>
vertical space here?

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
File net/web2socket_proxy/web2socket_serv.h (right):

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
net/web2socket_proxy/web2socket_serv.h:10: #include <stdint.h>
C system file go first

[Denis Lagno, 2010-12-03 16:28:50]

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
File net/web2socket_proxy/web2socket.h (right):

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
net/web2socket_proxy/web2socket.h:5: #ifndef
CHROME_WEB2SOCKET_PROXY_WEB2SOCKET_H_
On 2010/12/03 07:53:33, tyoshino wrote:
> #ifndef NET_WEB2SOCKET_PROXY_WEB2SOCKET_H_

Done.

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
File net/web2socket_proxy/web2socket_conn.cc (right):

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
net/web2socket_proxy/web2socket_conn.cc:29: rv += (isascii(s[i]) &&
isspace(s[i]));
On 2010/12/03 07:53:33, tyoshino wrote:
> If you don't have to keep this code independent from Chromium tree, how about
> using functions defined in base/string_util.h that are locale independent
> instead of C library?

actually we don't need to be locale-independent here since protocol strictly
specifies that there is sequence of octets in UTF-8.

And actually protocol (the version used ATM) specifies that we should count
spaces as 0x20 octets and not any whitespace as isspace does.  Since it is UTF-8
so just counting 0x20 octets does the job.

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
net/web2socket_proxy/web2socket_conn.cc:40: rv += tolower(*begin);
On 2010/12/03 07:53:33, tyoshino wrote:
> ditto. ToLowerASCII

Done.

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
net/web2socket_proxy/web2socket_conn.cc:75: if (!isascii(*pos) ||
!isdigit(*pos)) {
On 2010/12/03 07:53:33, tyoshino wrote:
> How about isascii(*pos) && isdigit(*pos)?
> But if so, *pos will be never ':' below.

actually !x || !y is !(x && y), not just (x && y).
Anyway I reorganized/simplified code here utilizing IsAsciiDigit from
base/string_util.h

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
net/web2socket_proxy/web2socket_conn.cc:86: if (*port <= 0 || *port >= (2 <<
16)) {
On 2010/12/03 07:53:33, tyoshino wrote:
> Up to your design, but how about handling erroneous port number (>= 2^16) as
> failure.

Done.

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
net/web2socket_proxy/web2socket_conn.cc:185: uint8_t key3[8]; // Notation (key3)
matches websocket RFC.
On 2010/12/03 07:53:33, tyoshino wrote:
> Two space before inline comment

Done.

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
net/web2socket_proxy/web2socket_conn.cc:191: memcpy(key3, term_pos -
sizeof(key3), sizeof(key3));
On 2010/12/03 07:53:33, tyoshino wrote:
> how about swapping these lines to remove "- sizeof(key3)"

Done.

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
net/web2socket_proxy/web2socket_conn.cc:283: snprintf(buf, sizeof(buf),
On 2010/12/03 07:53:33, tyoshino wrote:
> check return value?

Done.

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
net/web2socket_proxy/web2socket_conn.cc:358: if (buf_size < 1) {
On 2010/12/03 07:53:33, tyoshino wrote:
> We can remove if-clause?

It shouldn't happen in practice, but just a safety check in case libevent calls
us back with empty buffer (kind of spurious wakeup).

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
File net/web2socket_proxy/web2socket_serv.cc (right):

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
net/web2socket_proxy/web2socket_serv.cc:9: #include <string.h>
On 2010/12/03 07:53:33, tyoshino wrote:
> vertical space here?

Done.

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
File net/web2socket_proxy/web2socket_serv.h (right):

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
net/web2socket_proxy/web2socket_serv.h:10: #include <stdint.h>
On 2010/12/03 07:53:33, tyoshino wrote:
> C system file go first

Done.

[tyoshino (SeeGerritForStatus), 2010-12-06 10:02:51]

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
File net/web2socket_proxy/web2socket_conn.cc (right):

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
net/web2socket_proxy/web2socket_conn.cc:29: rv += (isascii(s[i]) &&
isspace(s[i]));
On 2010/12/03 16:28:50, Denis Lagno wrote:
> On 2010/12/03 07:53:33, tyoshino wrote:
> > If you don't have to keep this code independent from Chromium tree, how
about
> > using functions defined in base/string_util.h that are locale independent
> > instead of C library?
> 
> actually we don't need to be locale-independent here since protocol strictly
> specifies that there is sequence of octets in UTF-8.
> 

e.g. isdigit may recognize some characters other than '0' - '9' on system using
some code page. It's safe to use base/string_util.h to make the implementation
accept only input that follows the spec strictly.

> And actually protocol (the version used ATM) specifies that we should count
> spaces as 0x20 octets and not any whitespace as isspace does.  Since it is
UTF-8
> so just counting 0x20 octets does the job.

Yes, that's best.

http://codereview.chromium.org/5484001/diff/11001/net/web2socket_proxy/web2so...
net/web2socket_proxy/web2socket_conn.cc:75: if (!isascii(*pos) ||
!isdigit(*pos)) {
On 2010/12/03 16:28:50, Denis Lagno wrote:
> On 2010/12/03 07:53:33, tyoshino wrote:
> > How about isascii(*pos) && isdigit(*pos)?
> > But if so, *pos will be never ':' below.
> 
> actually !x || !y is !(x && y), not just (x && y).
> Anyway I reorganized/simplified code here utilizing IsAsciiDigit from
> base/string_util.h

Oops, I put wrong comment. You're right. Anyway, thanks for reorganizing.

[Denis Lagno, 2010-12-06 18:54:03]

Guys, I've prepared kind of design doc for this proxy: 

https://docs1.google.com/a/google.com/document/d/15rubsYT_DJJ12MFcHWxCqxYCBH9...

You may take a look at it and raise questions before it is sent to chromium-dev@

[tyoshino (SeeGerritForStatus), 2010-12-07 02:59:22]

On 2010/12/06 18:54:03, Denis Lagno wrote:
> Guys, I've prepared kind of design doc for this proxy: 
> 
>
https://docs1.google.com/a/google.com/document/d/15rubsYT_DJJ12MFcHWxCqxYCBH9...
> 
> You may take a look at it and raise questions before it is sent to
chromium-dev@

Could you publish it using chromium.org account? Since this code review is
public, we should align the permission, IMO.

[Denis Lagno, 2010-12-08 12:44:16]

Done:
https://docs.google.com/document/d/1646AjOBqCFnjdSxfNAbyfLNomnYvp9Wa3R7DjJuR-...

[Sergey Ulanov, 2011-02-10 03:53:32]

Just wondering: is this moving anywhere? Are there any alternative design
proposals for raw TCP connections?

[Denis Lagno, 2011-02-10 11:09:23]

On 2011/02/10 03:53:32, sergeyu wrote:
> Just wondering: is this moving anywhere? Are there any alternative design
> proposals for raw TCP connections?


As far as I can tell the current settlement on this topic is that if someone
wants this functionality he should spend resources on setup/administration
of proxy like that in his local/corporate network