Re: commit: ldap/servers/slapd/back-ldbm attr.c

[Hallvard B Furuseth <h.b.furuseth@usit.uio.no>]

hyc@OpenLDAP.org writes:
> Modified Files:
> 	attr.c  1.33 -> 1.34

Also back-ldap/bind.c 1.52 -> 1.53, back-bdb/attr.c 1.13 -> 1.14

> Log Message:
> Fix AVL comparisons

> -       return desc - a->ai_desc;
> +       return (unsigned)desc - (unsigned)a->ai_desc;

I don't know what this is supposed to fix, but:

(a) Both new and original code gives the wrong result or signals an
    overflow for pointers to memory locations that are more than
    INT_MAX bytes or so apart.

(b) Pointer comparison is only defined between pointers to the same
    array.  The result of the original code is undefined if desc and
    a->ai_desc point to the results of different malloc operations.

(c) The result of casting a pointer to an integer is implementation-
    defined.  I think it might e.g. return 42 for any pointer.

There is no way to fix all three problems.

This fixes (a) and (c):

    if (desc < a->ai_desc)
        return -1;
    return (desc != a-ai_desc);

This has a better chance than the committed code of fixing (a) and (b),
but can still fail if pointers are wider than unsigned long:

    if ((unsigned long)desc < (unsigned long)a->ai_desc)
        return -1;
    return ((unsigned long)desc != (unsigned long)a-ai_desc);

Take your pick.  I vote for fixing (a) and (c) since that at least fixes
these problems completely, which the fix for (b) is not guaranteed.  If
we meet an implementation where (b) is a problem, we can fix (b), or
rewrite these functions completely so they don't compare pointers at
all, with or without casts to integers.

-- 
Hallvard