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...
On 07/12/2011 02:26 AM, Alex Rousskov wrote:
On 07/11/2011 01:35 PM, Tsantilas Christos wrote:
On 07/11/2011 08:52 AM, Amos Jeffries wrote:
The change you made was the right fix, but it now appears we are going
to have to do a deeper check of side effects before it can go in. I
think the DNS issues that popped up immediately are due to IsIPv6()
being true when IsAnyAddr().
If IsIPv6 is the only problem, it has an easy solution. Just removing
the IsAnyAddr check from IsIPv6 should be enough:
Ip::Address::IsIPv6() const
{
return IsNoAddr() || !IN6_IS_ADDR_V4MAPPED(&m_SocketAddr.sin6_addr );
}
But there are many many checks with IsAnyAddr in squid in various
places, we may have other problems...
Disconnecting the Is*() from each other will have to be the first
minimal step. Then checking the code which ises them to see that no
other assumptions are made. :(
We need to decide whether a single address can be both IPv4 and IPv6 at
the same time. If yes, then IsIPv*() can return true for IsNoAddr().
Otherwise, IsIPv*() should assert that the address is defined and the
caller has to be more careful.
There are about 40 calls for IsIPv4() and 30 calls for IsIPv6(). Do we
have a lot of code that thinks that "not IPv4" implies "IPv6" and "not
IPv6" implies "IPv4"? Perhaps we should rename IsIP* to IsIp* and
carefully check each case for IsNoAddr()?
Thank you,
Alex.
=== modified file 'src/ip/Address.cc'
--- src/ip/Address.cc 2011-07-04 04:28:59 +0000
+++ src/ip/Address.cc 2011-07-22 10:20:34 +0000
@@ -173,19 +173,19 @@
bool
Ip::Address::IsIPv4() const
{
- return IsAnyAddr() || IsNoAddr() || IN6_IS_ADDR_V4MAPPED( &m_SocketAddr.sin6_addr );
+ return IsNoAddr() || 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 IsNoAddr() || !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.
