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.
So, attached is a slight update:
* moving the IsIPv4/6 to base purely on the v4-mapped or not.
* making both protocols ANYADDR match the same test
* making both protocols NOADDR match the same test
* polishing all Is*() tests to be independent of each others results
Amos
