patchmldonkey, a multi-networks file-sharing client - Patches: patch #3717, Improvement of patch 3672...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #3717: Improvement of patch 3672 (additonally bugfix for wrong Hi/Lo-ID in serverlist)

Submitter:  Fritz Mock <surround>
Submitted:  Thu 03 Feb 2005 11:39:51 PM UTC
   
 
Category:  Bug fixes Priority:  5 - Normal
Status:  Done Assigned to:  None
Open/Closed:  Closed

Jump to the original submission

Sat 05 Mar 2005 12:41:45 AM UTC, comment #15: 

for the record: the discussion was continued on irc in private and we agreed that firewalled support for mldonkey is not realy clean and complete, but as long as there is no better patch my applied version is left as is.

Amorphous <amorphous>
Fri 04 Mar 2005 08:03:53 PM UTC, comment #14: 


I prefere posts for code fragments ... and I prefere a chat  discussions. What should we do? ;)

I'll try to shorten the discussion ...

---
- mldonkey will continue to collect LowID sources and send callback requests to servers. That's certainly a heavy and very malicious bug.
---
yep we should not do that if low_id server_cid && not (!!force_high_id || !!force_client_high_id)
---
Ok, so we do not collect LowId-Sources and callback-requests, if the server tells us we are a HighID ... the condition will never result in true as long as the left-hand side of && is false.

---
If that's what you intend, you have formulated your code wrong [...] your code is exactly the opposite from what you have written as text.
---
No. Either you misunderstood the "not" or you mixed "server_cid" and "client_ip None"
this "not (!!force_high_id || !!force_client_high_id)" is true when force_high_id and force_client_high_id are false.
---

Hm  ... yes, and to be precise this "not (!!force_high_id || !!force_client_high_id)" will always be false if any of both options is set to true.

We can skipp the remaing && .. and say:

We will only send the server_cid if both options are set to false.

And I didn't mix something up ... server_cid contains our (Low)ID and client_ip is obviously our IP ... did you mix it up?


This isn't like ...
---
If these options are set to false we don't know if we are firewalled.
---
It is more like "we know we are firewalled, if both options are set to false"
... and in any other case we will send out our IP.



---
Why would we want to use different aproaches to decide about the firewalled-thingy in different places of the code?
---
because there are more cases than firewalled and not firewalled. for example the case got low ID and is not firewalled or the case user doesn't change any option and mldonkey must see for itself if it is firewalled or not.
---
Yes there are more cases than firewalled or not firewalled ... but there are only these two rules "play ed2k like firewalled" and "play like not firewalled" and the decision which rules to use is always the same, isn't it? ;)


---
Show me how you are going to fix mldonkey, without throwing in ever more options for each individual location and show me how you are going to create something rock-solid.
---
That is a not so easy task with the edonkey network.
---
It is not that difficult as you expect it to be ...

Beeing firewalled itself is the rule and not something defineable by rules!

But we might want to further discuss it based on a patch, that's easier.

Fritz Mock <surround>
Fri 04 Mar 2005 02:10:00 PM UTC, comment #13: 

---
- mldonkey will continue to collect LowID sources and send callback requests to servers. That's certainly a heavy and very malicious bug.
---
yep we should not do that if low_id server_cid && not (!!force_high_id || !!force_client_high_id)


---
- as mentioned in the last post, mldonkey will perform a DOS attack against servers, if it is firewalled and the option force_high_id is set true. One more heavy and very malicious bug.
---
it should not try to connect too often anyway, like it is regulated with clients.


---
- if firewalled mldonkey will not behave well, if become_master_delay or imidiate_master (not sure if both of them still do something in the code) are not set properly. Once mldonkey is not connected to a server (or even if we never connect to servers), it still has to be able to perform right, if it is firewalled.
---
as far as i understand it (not sure if i got it all right) we can only know (without the user manually telling us) that we are firewalled if we have a master-server that gives us a low ID (we could remeber that for the case that we get disconnected and for a restart). but with your patch in even that case mldonkey would not send it's low ID and therefore tell it's edonkey-peers it is not firewalled.
from your patch:
---
if !!firewalled_mode
&& (donkeyIsLowID server_cid)
&& (Ip.any != server_cid)
then server_cid
else client_ip None
---
"server_cid" is in words in this case: send low id and therefore say i'm firewalled
"client_ip None" is in words: send our ip and therefore say i'm not firewalled, but if force_client_ip returns a ip like 10.0.0.0 perhaps the peer will know we are firewalled (i'm not sure, perhaps 0.0.0.0 is better for such a case; what does emule do if it knows it is firewalled but has no low id from an server?)
perhaps we should add an option force_low_id that if true is an alias to force_client_ip=true and client_ip=0.0.0.0 ?
oh and this all is only the edonkey network (overnet and kademlia implementations have afaik no special handling if we are firewalled, wich they should)


---
If that's what you intend, you have formulated your code wrong [...] your code is exactly the opposite from what you have written as text.
---
No. Either you misunderstood the "not" or you mixed "server_cid" and "client_ip None"
this "not (!!force_high_id || !!force_client_high_id)" is true when force_high_id and force_client_high_id are false.


---
Why would we want to use different aproaches to decide about the firewalled-thingy in different places of the code?
---
because there are more cases than firewalled and not firewalled. for example the case got low ID and is not firewalled or the case user doesn't change any option and mldonkey must see for itself if it is firewalled or not.


---
Show me how you are going to fix mldonkey, without throwing in ever more options for each individual location and show me how you are going to create something rock-solid.
---
That is a not so easy task with the edonkey network. But your patches are very use- and helpfull (especialy the source-management ones), don't let it irritate you if I don't like something from them.


---
Your changes are certainly not a basis for further attempt to fix the other problems mldonkey has as firewalled client.
---
Mh i'm not sure i see that, i think they are no worse than your's.


If you are familiar with irc, i think it would be easier if we talk that through on irc. If you wanna contact me there, my nick is Amorphous or Morphous (just say my name to see if I'm near the computer).

Amorphous <amorphous>
Fri 04 Mar 2005 10:04:14 AM UTC, comment #12: 


Oh, shit happened ... the last post was from me Amorphous.

The crucial point is, that mldonkey will not behave correctly as firewalled client. (Besides the one place original patch is about.)
- mldonkey will continue to collect LowID sources and send callback requests to servers. That's certainly a heavy and very malicious bug.
- as mentioned in the last post, mldonkey will perform a DOS attack against servers, if it is firewalled and the option force_high_id is set true. One more heavy and very malicious bug.
- mldonkey will still send out a probably valid ip, for almost any combination of the options client_ip, force_client_ip and use_server_ip and fool other clients into unneccessary connection attempts to nirvana. One more malicious bug, although not that heavy like the above.
- if firewalled mldonkey will not behave well, if become_master_delay or imidiate_master (not sure if both of them still do something in the code) are not set properly. Once mldonkey is not connected to a server (or even if we never connect to servers), it still has to be able to perform right, if it is firewalled.

These are 5 to 6 options that have to be derived from an attribute "I am firewalled" and at least 3 "rules" on how to play the ed2k-game, that depend on "I am firewalled".

---
We don't derive from these that we are firewalled, we only derive from them if we are not firewalled. If these options are set to false we don't know if we are firewalled.
---

If that's what you intend, you have formulated your code wrong ... your code exctly tells mldonkey, it should only behave like firewalled if both options are set to false and in any other case mldonkey should consider himself as not firewalled ... your code is exactly the opposite from what you have written as text.



                       let send_this_id =
                           if not (!!force_high_id || !!force_client_high_id)
                               && low_id server_cid
                               && Ip.any <> server_cid
                               then
                             server_cid
                           else
                             client_ip None


This code derives "I am firewaleld" from the combination of force_high_id and force_client_high_id.

And indeed, the code has nothing nothing to do with connecting to servers, but it has something in common with it.
Both things have in common that the must differ between firewalled/non-firewalled and your condition is insufficent to do that for both situations.
Your aproach to decide about firewalled/non-firewalled might solve one specific problem, but is unusable in the other place(s).
Why would we want to use different aproaches to decided about the firewalled-thingy in different places of the code? 

Show me how you are going to fix mldonkey, without throwing in ever more options for each individual location and show me how you are going to create something rock-solid.

Your changes are certainly not a basis for further attempt to fix the other problems mldonkey has as firewalled client.

Fritz Mock <surround>
Fri 04 Mar 2005 12:38:43 AM UTC, comment #11: 

---
What's your point about using a combination of force_high_id and force_client_high_id to know we are firewalled?
---
We don't derive from these that we are firewalled, we only derive from them if we are not firewalled. If these options are set to false we don't know if we are firewalled. And the part of the code i changed has nothing to do with servers, it has only to do with clients and determines if we send them our ip or our low-id we got from an server.


---
If you want to use mldonkey behind a firewall, ther are currently at least 5 options that have to be set correctly (most of them different from the defaults.)
---
To my understanding mldonkey will work correctly as firewalled and as directly-connected with the defaults (on the assuption, that the donkey-server does determine correctly if we are firewalled or not). Wich options have to be set different from the default in your oppinion?


May i ask who you are? I like to connect a conversation to a name.

Amorphous <amorphous>
Thu 03 Mar 2005 11:33:40 PM UTC, comment #10: 


I took a quick look into your changes Amorphous ...

What's your point about using a combination of force_high_id and force_client_high_id to know we are firewalled?
Why do you dislike a single option that tells mldonkey, that it is behind a firewall?

Your patch is seriously flawed ...

Let's see, how your "synonym" for "we are firewalled" will play out, if we consequently use it, whenever we need to know weather or not we are firewalled ...

Here we go: We want to disconnect from servers if we get a low id and force_high_id is true ... of course, we don't want to do that if we are firewalled, since we cannot get a high id and would end up in an endless connecting-disconnecting circle, that harms servers.

The following is exactly the sentence above as code.

if
( lowid cid && !!force_high_id )
&& not
( not (!!force_high_id || !!force_client_high_id) )
then (...disconnet & connect another server)

... the second part of the condition is irrelevant. since if fore_high_id is true, then we are never firewalled ... by your definition.
In other words, you define "I am firewalled" in a way that makes a prevention from an endless connection-disconnection circle impossible. Certainly this is the wrong definition of "I am firewalled".


With "not (!!force_high_id || !!force_client_high_id)" you trapped yourself into an obvious caveeat:
If mldonkey "knows" it is firewalled, it has (amongst many other things) to force the option force_high_id to false!


More generalized one can say, there is no way to derive the predicate "I am firewalled" from releated options, since to prevent malfunction, this predicate must be dominant over all other releated options!

This is an ultimate rule, there is no way to come around it.

Beeing firewalled itself is the rule and not something defineable by rules!


Besides that, per defaults mldonkey should work properly as a none-firewalled client, but a comp-sci masters degree should not be neccessary to properly set up a firewalled-client. If you want to use mldonkey behind a firewall, ther are currently at least 5 options that have to be set correctly (most of them different from the defaults.) If not done right one or the other thing is screwed up somehow. That's an nearly impossible task for a normal user ... even you Amorphous would have stumbled into a screwed up configuration with force_high_id ;)

Anonymous
Wed 02 Mar 2005 03:37:37 PM UTC, comment #9: 

I adapted your patch a bit. Please check the link for the changes:
https://opensvn.csie.org/viewcvs.cgi?root=mlnet&rev=208&view=rev
Please report if it seems ok.

Amorphous <amorphous>
Sun 20 Feb 2005 02:56:33 AM UTC, comment #8: 

applied 2-5-28-1

spiralvoice <spiralvoice>
Group administrator
Mon 07 Feb 2005 10:24:40 AM UTC, comment #7: 


@Amorphous

If I am not firewalled, I probably not want to use force_high_id = true. (Maybe the route to most servers often has delays, this might be the case if you're in the other end of the world.) It is pretty fine, to have a lowid from servers and have other clients requesting callbacks and then tell every client in on a connect or connect reply that we have some IP and Port where they can connect.

Fritz Mock <surround>
Mon 07 Feb 2005 10:21:54 AM UTC, comment #6: 

@devein:

Serers ARE NOT RELIABLE when detecting if a client is firewalled or not.

Exactly that's where the original patch did fail. The IP/ID assigned by the server was used in any case. People (non firewalled) experienced a performance loss, because they were mistakenly treated as firewalled clients.

A server detects if a client is firewalled or not, by simply trying to contact the client. If that fails, they assign a low-id, otherwise a high-id.
I give two simple examples where servers autodetection fails.
- You get falsivly assigned a low-id, simply if there is a network congestion.
- You get falsivly assigned a high-id, if the route between server and client is ok, but the route to vast majority of other lcients is not.
(For example server and client are on the same network, the ISP does not block 4662 internally, but to all external networks. ... and yes, it is NOT an pointless example! The server could use an unblocked port, but the client does not. The server could even just exist, to give the users from that ISP the chance to do non firewalled file-sharing with each other. Note that this example is simplified, you can imagine that with an route between both over many hops all sorts of constellations are posiible.)

But there is even some more ...

When using the servers asigned ID/IP to detect, wheather or not we are firewalled, how do you want to know, if you're firewalled or not, without beeing connected to a server?
Have you even take a look at the patch? When do we fallback to send 10.0.0.0 as ID/IP? We need to do that, anytime we don't have a master-server. What would you do directly after startup?
Would you continue to send out your stored (as IP) old and stale ID when your current masterserver disconnects you!? Or would we go to sleep until you connect to the next server and it becomes a master server after become_master_server's delay?

Weather or not we use firewall-auto-detection over ID/IP, we would ultimatly need a value to remember that state, once we have no connected servers!
And if we would do autodetection we need to give the users an option to override what mldonkey has detected, for all cases where autodetection fails.

Fritz Mock <surround>
Mon 07 Feb 2005 05:05:08 AM UTC, comment #5: 

There is an option force_high_id and IMHO that should be used, instead of a new option firewalled_mode. If it is off we should autodetect if we are firewalled. Or is there something this wouldn't solve?

Amorphous <amorphous>
Sun 06 Feb 2005 12:47:18 PM UTC, comment #4: 

imho the best solution would be to interpret assigned IP from mldonkey server (default "use_server_ip" is true, AFAIR).

edk servers ARE ABLE to detect if client is behind a firewall or has an external IP.

how emule is handling this?



devein
Sun 06 Feb 2005 01:13:23 AM UTC, comment #3: 


Basically you're right White_FrosT and it was one of my first thoughts ... but I didn't want to mess around in too many places, before a few people have tested that patch.

And there is another point ... I am not realy sure, if and how we would interfere with other networks. I would keep changes restricted to the donkey protocol ... i.e. leave the other two options as they are and override them with defaults in donkey if firewalled.

Fritz Mock <surround>
Fri 04 Feb 2005 10:23:44 AM UTC, comment #2: 

Why not set 'client_ip' and 'force_client_ip' to fixed values as proposed when 'firewalled_mode=true' ? Would make the system some more tamper proof...

White_FrosT
Thu 03 Feb 2005 11:49:15 PM UTC, comment #1: 


This patch is against 2.5.28o ... of course.

Fritz Mock <surround>
Thu 03 Feb 2005 11:39:51 PM UTC, original submission:  


The Patch 3672 is improper in some not so common cases.

When a server assigns (falsivley) a LowId, then in #3672 we send the (falsivly) assigned LowID to other clients in the connect requests and mark ourselfs as firewalled, even if we can be contacted from outside. (A network congestion for example can lead to a falsivly assigned LowID.)

To avoid this, the attached patch sends only the LowID assigned from a server to other clients, when the newly added option firewalled_mode is set to true. Otherwise the configured (or autodetected) IP is sent in connect requests.

Non-Firewalled users should leave that option unchanged! (the default is false.)
If firewalled_mode = false, mldonkey behaves exactly the same way it did prior the patch #3672 (!)

This should solve the problems some non-firewalled people experienced with this patch.

The patch also includes a fic for the entirely wrong HighID/LowID output in the "Connected Servers" page in the webinterface.

Last but not least, to avoid any misunderstanding ... in order to have a firewalled mldonkey work properly, one has to set the following options:

client_ip = 10.0.0.0
force_client_ip = true
firewalled_mode = true

(I amnot sure about become_master_delay and/or immidiate_master ... during my testing I have experienced, that directly after startup we obviously have no master_server, thus we cannot send out any ID. This case is handled "halfway" properly by sending the client_ip, which should be set to an IP from private networkranges, optimally with the lst byte set to zero. I guess become_master_delay and immidiate_master could be used, to keep that period as short as possible. Not a perfect solution, but that period just lasts for the first few minutes.)


... certainly, there are some more bugs in mldonkey, regarding lowid-support, but at least server and client connects should be ok now. ;)

Fritz Mock <surround>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #8365:  lowid.patch added by surround (4KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

CC list is empty

 

There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

 

Follow 3 latest changes.

Date Changed by Updated Field Previous Value => Replaced by
2005-02-20 spiralvoice StatusNone Done
    Open/ClosedOpen Closed
2005-02-03 surround Attached File- Added lowid.patch, #4139

Back to the top

Powered by Savane 3.13-f762.
Corresponding source code