I am sending a version 4 of the patch which is the same with Amos patch
but is a little smaller. IT is easier to see the changes.
But applying this patch I found some problems in icp* related code. This
problems has as result an assertion failure inside icp code.
I am attaching a patch which is required to allow icp works with
isAnyAddr patch, but maybe fixes bugs.
From what I can understand reading about the icp protocol, it supports
only the IPv4 protocol: It contains in icp messages header a 32bit
number which represents the ip address of sender. This is mean that only
ipv4 addresses can used.
The icp-isanyaddr.patch:
1) Use the ip::Address::SetIPv4 method to always convert to IPv4 the
ip addresses used to set the hostid field of the ICP header message.
2) Inside neighbors_init() function there is the line:
theIcpPublicHostID.GetInAddr( *((struct
in_addr*)&echo_hdr.shostid) );
The above line sets the hostid field of the header of the icp echo
message, to theIcpPublciHostID. The problem is that this ipaddress is
not initialized yet. Will be initialized inside
icpIncomingConnectionOpened when this function called.
The icp-isanyadd.patch replaces the above line with a simple:
echo_hdr.shostid = 0;
which is absolutely equivalent (but probably not the correct).
The icp-isanyaddr.patch is not a finished patch but I am posting it here
to show two (possible) problems:
- The icp does not work with IPv4 so we may add checks to prevent
using ipv6 addresses
- The echo_hdr message initialized with an IP address which is not
initialized yet.
On 07/23/2011 05:53 AM, Amos Jeffries wrote:
On 23/07/11 04:18, Tsantilas Christos wrote:
On 07/22/2011 02:27 PM, Amos Jeffries wrote:
On 22/07/11 22:53, Tsantilas Christos wrote:
I did a small research and I am finding many problems with the current
implementation and use of the Ip::Address::IsAnyAddr() method.
To summarize the problem:
- Currently the ip::Address::IsAnyAddr() return true only for ipv6
anyaddr (0000:0000:0000:0000:0000:0000:0000:0000). It return false when
we have an ipv4 anyaddr (0000:0000:0000:0000:0000:FFFF:0000:0000)
This is not normal.
- The ip::Address::IsIPv6 and ip::Address::IsIPv4 methods return
true in
the case the IsAnyAddr return true, this is mean that the
ip::Address::IsIPv4 return false in the case of ipv4 anyaddr.
This is a bug.
Searching in the code I am finding other bugs too. For example:
- inside Ip::Address::SetIPv4(). If we have an ip address which is
(ipv6) anyaddr and we call SetIPv4 for this ip address to convert it to
ipv4 ip address, the conversion will done, but the ip address will not
considred as anyaddr any more, because the IsAnyAddr will return false,
for ipv4 anyaddr.
- inside cache_cf.cc file inside dump_generic_http_port function there
is the code:
if (s->s.IsAnyAddr() && !s->s.IsIPv6())
storeAppendPrintf(e, " ipv4");
The if condition in the above statement can never be true. But the s->s
can be an ipv4 anyaddr.
Also in the code I am finding other places where my sense is that the
code will not work as expected in the case we are listening to an ipv4
anyaddr ip address.
I am attaching a patch here which:
- Ip::Address::IsAnyAddr() return true for both ipv6 or ipv4 anyaddr.
- IsIPv6 return true only in the case of ipv6 anyaddr. If the ip
address
is an ipv4 anyaddr return false.
- IsIPv4 return true only in the case of ipv4 anyaddr. If the ip
address
is an ipv6 anyaddr return false.
From what I can see looking the code this is what expected by these
methods.
I did not do extended tests, but looks that this is the best
solution...
Hmm. Okay, but not quite far enough. As Alex pointed out IsNoAddr() also
need to be kept in sync. Also IsLocalhost().
The IsLocalhost() is already implemented.
About the IsNoAddr I am suggesting to not change it. Currently someone
uses the SetNoAddr to set it and use the IsNoAddr method to check if we
do not have any valid ipaddress. I think does not make sense to have one
ipv4 non valid address and one ipv6 non valid address.
The localhost and anyaddr are both valid addresses which can be ipv6 or
ipv4.
The only use would be for logging display and parser testing when
reading 255.255.255.255. Minor but useful. Particularly if IsNoAddr() is
true for both.
Amos
=== modified file 'src/ip/Address.cc'
--- src/ip/Address.cc 2011-07-04 04:28:59 +0000
+++ src/ip/Address.cc 2011-07-25 14:28:42 +0000
@@ -173,19 +173,19 @@
bool
Ip::Address::IsIPv4() const
{
- return IsAnyAddr() || IsNoAddr() || IN6_IS_ADDR_V4MAPPED( &m_SocketAddr.sin6_addr );
+ return IN6_IS_ADDR_V4MAPPED( &m_SocketAddr.sin6_addr );
}
bool
Ip::Address::IsIPv6() const
{
- return IsAnyAddr() || IsNoAddr() || !IN6_IS_ADDR_V4MAPPED( &m_SocketAddr.sin6_addr );
+ return !IN6_IS_ADDR_V4MAPPED( &m_SocketAddr.sin6_addr );
}
bool
Ip::Address::IsAnyAddr() const
{
- return IN6_IS_ADDR_UNSPECIFIED( &m_SocketAddr.sin6_addr );
+ return IN6_IS_ADDR_UNSPECIFIED( &m_SocketAddr.sin6_addr ) || IN6_ARE_ADDR_EQUAL( &m_SocketAddr.sin6_addr, &v4_anyaddr); ;
}
/// NOTE: Does NOT clear the Port stored. Ony the Address and Type.
@@ -237,6 +237,11 @@
return true;
}
+ if ( IsNoAddr() ) {
+ m_SocketAddr.sin6_addr = v4_noaddr;
+ return true;
+ }
+
if ( IsIPv4())
return true;
@@ -279,7 +284,8 @@
Ip::Address::IsNoAddr() const
{
// IFF the address == 0xff..ff (all ones)
- return IN6_ARE_ADDR_EQUAL( &m_SocketAddr.sin6_addr, &v6_noaddr );
+ return IN6_ARE_ADDR_EQUAL( &m_SocketAddr.sin6_addr, &v6_noaddr )
+ || IN6_ARE_ADDR_EQUAL( &m_SocketAddr.sin6_addr, &v4_noaddr );
}
void
=== modified file 'src/icp_v2.cc'
--- src/icp_v2.cc 2011-07-16 15:21:48 +0000
+++ src/icp_v2.cc 2011-07-25 14:45:17 +0000
@@ -746,73 +746,77 @@
Comm::SetSelect(icpOutgoingConn->fd, COMM_SELECT_READ, icpHandleUdp, NULL, 0);
fd_note(icpOutgoingConn->fd, "Outgoing ICP socket");
icpGetOutgoingIpAddress();
}
}
// Ensure that we have the IP address(es) to use for Host ID.
// The outgoing address is used as 'private' host ID used only on packets we send
static void
icpGetOutgoingIpAddress()
{
struct addrinfo *xai = NULL;
theIcpPrivateHostID.SetEmpty();
theIcpPrivateHostID.InitAddrInfo(xai);
if (getsockname(icpOutgoingConn->fd, xai->ai_addr, &xai->ai_addrlen) < 0)
debugs(50, DBG_IMPORTANT, "ERROR: Unable to identify ICP host ID to use for " << icpOutgoingConn
<< ": getsockname: " << xstrerror());
else
theIcpPrivateHostID = *xai;
theIcpPrivateHostID.FreeAddrInfo(xai);
+
+ theIcpPrivateHostID.SetIPv4();
}
static void
icpIncomingConnectionOpened(int errNo)
{
if (!Comm::IsConnOpen(icpIncomingConn))
fatal("Cannot open ICP Port");
Comm::SetSelect(icpIncomingConn->fd, COMM_SELECT_READ, icpHandleUdp, NULL, 0);
for (const wordlist *s = Config.mcast_group_list; s; s = s->next)
ipcache_nbgethostbyname(s->key, mcastJoinGroups, NULL); // XXX: pass the icpIncomingConn for mcastJoinGroups usage.
debugs(12, DBG_IMPORTANT, "Accepting ICP messages on " << icpIncomingConn->local);
fd_note(icpIncomingConn->fd, "Incoming ICP port");
if (Config.Addrs.udp_outgoing.IsNoAddr()) {
icpOutgoingConn = icpIncomingConn;
debugs(12, DBG_IMPORTANT, "Sending ICP messages from " << icpOutgoingConn->local);
icpGetOutgoingIpAddress();
}
// Ensure that we have the IP address(es) to use for Host ID.
// The listening address is used as 'public' host ID which can be used to contact us
struct addrinfo *xai = NULL;
theIcpPublicHostID.InitAddrInfo(xai); // reset xai
if (getsockname(icpIncomingConn->fd, xai->ai_addr, &xai->ai_addrlen) < 0)
debugs(50, DBG_IMPORTANT, "ERROR: Unable to identify ICP host ID to use for " << icpIncomingConn
<< ": getsockname: " << xstrerror());
else
theIcpPublicHostID = *xai;
theIcpPublicHostID.FreeAddrInfo(xai);
+
+ theIcpPublicHostID.SetIPv4();
}
/**
* icpConnectionShutdown only closes the 'in' socket if it is
* different than the 'out' socket.
*/
void
icpConnectionShutdown(void)
{
if (!Comm::IsConnOpen(icpIncomingConn))
return;
debugs(12, DBG_IMPORTANT, "Stop receiving ICP on " << icpIncomingConn->local);
/** Release the 'in' socket for lazy closure.
* in and out sockets may be sharing one same FD.
* This prevents this function from executing repeatedly.
*/
icpIncomingConn = NULL;
=== modified file 'src/neighbors.cc'
--- src/neighbors.cc 2011-07-20 07:35:53 +0000
+++ src/neighbors.cc 2011-07-25 13:46:23 +0000
@@ -555,41 +555,42 @@
debugs(15, DBG_IMPORTANT, " Ignoring " <<
neighborTypeStr(thisPeer) << " " << thisPeer->host <<
"/" << thisPeer->http_port << "/" <<
thisPeer->icp.port);
neighborRemove(thisPeer);
}
}
}
peerRefreshDNS((void *) 1);
if (echo_hdr.opcode == ICP_INVALID) {
echo_hdr.opcode = ICP_SECHO;
echo_hdr.version = ICP_VERSION_CURRENT;
echo_hdr.length = 0;
echo_hdr.reqnum = 0;
echo_hdr.flags = 0;
echo_hdr.pad = 0;
- theIcpPublicHostID.GetInAddr( *((struct in_addr*)&echo_hdr.shostid) );
+ //theIcpPublicHostID.GetInAddr( *((struct in_addr*)&echo_hdr.shostid) );
+ echo_hdr.shostid = 0;
sep = getservbyname("echo", "udp");
echo_port = sep ? ntohs((u_short) sep->s_port) : 7;
}
first_ping = Config.peers;
}
int
neighborsUdpPing(HttpRequest * request,
StoreEntry * entry,
IRCB * callback,
void *callback_data,
int *exprep,
int *timeout)
{
const char *url = entry->url();
MemObject *mem = entry->mem_obj;
peer *p = NULL;
int i;
int reqnum = 0;
