Re: IpAddress comparison operators lie

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