[GonoszTopi]

A crappy or malicious client sending bad, or in some way specially crafted data, or maybe even plain junk, can cause an assertion failure in debug-enabled clients and can add invalid data to the routing table of release clients. Following is a patch to immediately stop processing packets if they turn out to be invalid. The patch is created against the 0.49b source, and affects CKademliaUDPListener::AddContact_KADEMLIA2(...)

--- KademliaUDPListener.cpp~    2008-08-01 19:31:00.000000000 +0200
+++ KademliaUDPListener.cpp     2008-11-28 01:37:34.000000000 +0100
@@ -512,6 +512,12 @@
                *puOutContactID = uID;
        uint16 uTCPPort = byteIO.ReadUInt16();
        uint8 uVersion = byteIO.ReadByte();
+       if (uVersion == 0)
+       {
+               CString strError;
+               strError.Format(_T("***NOTE: Received invalid Kademlia2 version (%u) in %hs"), uVersion, __FUNCTION__);
+               throw strError;
+       }
        if (pnOutVersion != NULL)
                *pnOutVersion = uVersion;
 
@@ -527,7 +533,11 @@
                        if (pTag->IsInt() && (uint16)pTag->GetInt() > 0)
                                uUDPPort = (uint16)pTag->GetInt();
                        else
-                               ASSERT( false );
+                       {
+                               CString strError;
+                               strError.Format(_T("***NOTE: Received invalid port (zero or non-integer) in %hs"), __FUNCTION__);
+                               throw strError;
+                       }
                }
 
                if (!pTag->m_name.Compare(TAG_KADMISCOPTIONS))
@@ -543,8 +553,9 @@
                                                ASSERT( false );
                                }
                        }
-                       else
-                               ASSERT( false );
+                       // Just ignore silently invalid KadMiscOptions
+                       //else
+                       //      ASSERT( false );
                }
 
                delete pTag;

There has been reports [1][2] for aMule that these assertions were triggered.

[Some Support]

Thanks for your hint, but i would say the handling in the original code is correct. There is no need to stop processing the packet if we receive an invalid or zero port tag, as we may still fetch the proper one later (or before). Depends on your view how error tolerant you intend to be.
However if we receive no port tag, the client is not added to the routing table because IsGoodIPPort() will filter it for the missing port. If the port is wrong or corrupted we will notice in the following third part of the handshake when we expect an answer from a packet we send to the given UDP port. Keep in mind that a client can send any UDP port it wants anyway, there is no point trying to do this by intentionally creating an invalid tag).
KadVersion 0 means Kad1 and is therefore indeed inappropriate for a Kad2 contact, but no harm done here - we will just treat the client as Kad1 in the future like it requests. (Edit: Actually there is a bug in this case which leads to an invalid answer packet (Kad1 with Kad2 opcode), so this really needs to be changed)

So your changes are fine, but there is no real bug in the current code.

[GonoszTopi]

I see. Thanks for the answer.