Alex Rousskov wrote:
On 05/01/2010 12:07 AM, Amos Jeffries wrote:
Alex Rousskov wrote:
Current IpAddress comparison operators are broken from logic point
of view. For example, if a and b are IpAddress objects, then
* both (a < b) and (a > b) are true if exactly one address is NoAddr;
Sorry.
(a > b) looks correct.
(a < b) should return false on that test instead of true. Or using the
AnyAddr test like <= operator does.
* (ip1 == ip2) may be true even if the addresses have different ports
We have to ignore ports in the boolean operators due to all the code
which does ACL matching on IPs. The squid.conf provided address has a
zero port.
If we include port in the comparison we will have to resort to
duplicating every address being ACL tested and set its port to zero
before doing each the test.
It's better to check the addr, then check the addr.GetPort() as needed
for the minority of things which check the port.
Hi Amos,
I appreciate ACL needs, but I think it is not acceptable to ignore
ports in these operators because they should compare IpAddresses and not
some arbitrary parts of those objects. I see two options:
A) Remove all overloaded comparison operators from IpAddress. Add
compareWithoutPort() and compareWithPort() methods that return +1/0/-1
as expected, following all traditional logic rules. This option will
force folks to think which comparison to use. It will add more work to
those who want to use C++ operators (e.g., in std::map)
B) Fix all overloaded comparison operators in IpAddress so that they
apply to the entire object and follow all traditional logic rules. Add
compareWithoutPort() method. Change ACLs to use compareWithoutPort().
This option may caught some by surprise because they might think that
IpAddress does not include port at all and it would take more work to
replace all current incorrect uses.
Long-term, the right thing to do is to have portless and portfull
address classes (one using the other) so that the code that does not
care about ports is not forced to ignore them. It is non-trivial to
design this right though and the change would be rather big, so let's
not go there for now.
Post is not the only field of the sockaddr_in6 being ignored by the
"Address" boolean operators. There is family etc.
This object in abstract is primarily a raw address. It's Get*()
operators can retrieve the address-related details which 'happen' to be
passed with it for testing.
The shortest path to conformity is to drop the boolean address operators
and make the code use a.matchIPaddr(b) <> 0 without the convenience of
(a < b) and memcmp() for matching the whole thing.
There may be more inconsistencies; I have not checked all possible
combinations.
These bugs make it impossible to reliably sort or compare addresses
using C++ operators. However, I do not know whether some code already
relies on this broken behavior. Does it? In other words, should we just
fix the operators or is there more to it?
None that I know of. +1 to fix IpAddress.
I disagree that ignoring ports in overloaded comparison operators is OK
so our notions of a fix differ. Will any of the above two options work
for you?
Well I meant make < consistent with <= and add the unit tests.
Amos
--
Please be using
Current Stable Squid 2.7.STABLE9 or 3.1.1
