Re: IpAddress comparison operators lie

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.

>> 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?

Thank you,

Alex.

> Looks like a few more sets of cases to testBooleans() to catch the
> ANYADDR / NOADDR cases as well.
>
> Amos
Received on Sat May 01 2010 - 06:38:08 MDT