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

["Pierangelo Masarati" <ando@sys-net.it>]

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

Howard and I discussed the problem, recently; we came out with
the conplusion that we definitely need a portable way to do
this comparison, which is basically

#if SAFE
#define PTRCMP(x,y) ((x) - (y))
#else
#define PTRCMP(x,y) ((x) > (y) ? 1 : ((x) < (y) ? -1 : 0)
#endif

where SAFE is: ptrdiff_t is:
  - declared
  - large enough to contain +/- the space addressable
    by our pointers
  - its cast to int (since AVL cmp funcs return int)
    doesn't lose the sign bit

If there are no objections I might start
implementing this soon.

Of course, we may simply stay with !SAFE;
we'd basically waste one comparison ...

Ando

-- 
Pierangelo Masarati
mailto:pierangelo.masarati@sys-net.it