[pio-trackers] [ pio-Patches-1757614 ] Tunneled connections

[SourceForge.net <no...@so...>]

Patches item #1757614, was opened at 2007-07-20 16:49
Message generated for change (Comment added) made by shevek
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305095&aid=1757614&group_id=5095

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: None
Group: None
Status: Open
Resolution: None
Priority: 4
Private: No
Submitted By: Bas Wijnen (shevek)
Assigned to: Bas Wijnen (shevek)
Summary: Tunneled connections

Initial Comment:
This patch implements tunneled connections (or actually, it contains the code from the earlier patch, changed to apply to the new code).

There are a few functions which allow connections to be tunneled over other connections, instead of using a new tcp connect function.  The good thing about this is that it is guaranteed to work: if you have a connection, you may not be able to create a new one (due to firewalls, for example), but you can use the one you have.

This patch doesn't actually use the functions yet.  I have tested it previously, and it worked.  This patch obviously doesn't break anything either, since no new code is actually called.  I would like to have it included anyway, so that the functionality can be used later.

There has been some discussion about whether tunneling is acceptable, in particular for the meta-server (which should remain low-traffic).  That discussion is not relevant for this patch, because it is easy to add an option to allow (or disallow) tunneling.  That option must however be implemented in the parts that call these functions, so they are of no concern here.

----------------------------------------------------------------------

>Comment By: Bas Wijnen (shevek)
Date: 2007-08-11 12:58

Message:
Logged In: YES 
user_id=42389
Originator: YES

Session *parent, *prev, *next: Yes, I suppose this is my own version of
GList.  I find creating linked lists so trivial that it doesn't usually
occur to me to use helper functions for it. ;-)  Given the rest of the
code, it's probably a good idea to rewrite it to use GList though.

net_write: I wrote this before I knew about g_strdup_printf.  It will
indeed become much more readable with that.

read_ready: The loop will always terminate at end-of-string.  In case of
malformed data, an empty string (which is also malformed data) is sent to
the state machine.  That seems acceptable to me.

In net_new, a normal connection is initialised.  For normal connections,
parent is NULL and next is the first child.  prev is only used for tunneled
connections.  So it does indeed not need to be initialised there.  However,
this code will change anyway when adapting it to GList.

net_add uses g_malloc because I didn't see a reason to use g_malloc0.  Is
there any?  We don't want to implicitly initialise values that way IMO.

net_add_new: I used a static variable for speed reasons (in the usual
case, this is O(1) while starting from 0 is O(n)).  However, speed is not
actually a concern here at all, so I don't really mind starting from 0. 
That does have the benefit of shorter strings and thus slightly less
network traffic.

About net_add and net_add_new: Perhaps they should be named better. :-) 
To make a tunneled connection, both sides should first agree that the
connection should be made.  This can be done for example by a meta-server
telling a server that it wants to tunnel a client connection.  (For this, a
new command is needed in the meta-server protocol.)  To do this, the
meta-server will call net_add_new.  That will result in a session with an
id.  This id is sent in the request, "client-tunnel 3", for example.  The
server will then call net_add to complete the connection.  It will use the
id it received from the meta-server.

How about net_tunnel_connect and net_tunnel_accept, to mirror the tcp/ip
function names?

I don't currently have code using the tunnels, but if you look at patch
#1464509 (closed), you can see an example.

I'll rewrite the patch and post a new version.

----------------------------------------------------------------------

Comment By: Roland Clobus (rclobus)
Date: 2007-08-08 21:17

Message:
Logged In: YES 
user_id=831677
Originator: NO

I've read the patch and have several questions:
* network.h
  Session *parent, *prev, *next: Are you implementing your own version of
GList?
* network.c (net_write)
  I have trouble understanding the code in net_write: what do all those
memcpy's do? Can't you use a g_strdup_printf instead?
* network.c (read_ready)
  When bad formed data is sent, the while loop will not terminate. That
could be a security bug.
* network.c (net_new)
  No initialization for ses->prev is needed?
* network.c (net_add)
  You use g_malloc instead of g_malloc0 as net_new does. Intentionally?
* network.c (net_add_new)
  Instead of using a static variable, can't you always start at zero when
looking for a usable id?
* network.h (net_add and net_add_new)
  Which of the two would I normally use to create a tunneled connection?
Could the other function become a file static function?
 
Do you have a script/code snippet that allows me to see what this patch
will actually do?

----------------------------------------------------------------------

Comment By: Bas Wijnen (shevek)
Date: 2007-07-21 11:16

Message:
Logged In: YES 
user_id=42389
Originator: YES

Thanks for reminding, I saw it and forgot it. :-)

Those things are now also implemented: tunnels with duplicate ids (on the
same parent) can never happen, and when a parent closes, all tunneled
connections are also closed.

It shouldn't have much effect yet, but there are some changes to the
current code and there is no benefit for the user yet, so I suggest adding
it after the release.  I've lowered the priority for that reason.
File Added: tunnel2.patch

----------------------------------------------------------------------

Comment By: Roland Clobus (rclobus)
Date: 2007-07-20 20:34

Message:
Logged In: YES 
user_id=831677
Originator: NO

This patch contains a few times 'FIXME'. It is not ready yet?

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305095&aid=1757614&group_id=5095