[Date Prev][Date Next]
RE: commit: ldap/servers/slapd/back-ldbm attr.c
> -----Original Message-----
> From: Hallvard B Furuseth [mailto:email@example.com]
Pierangelo and I were just having a conversation about the correct fix for
this... Definitely the last commits are unsafe, particularly on platforms
where sizeof(int) != sizeof(pointer).
> 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 is probably fine.
Problem (b) is true per the ANSI spec, but is not significant here. The
elements of the AVL tree are all homogeneous - in back-ldap they are
connection pointers, and the connections are all elements of a single static
array. The AttributeDescriptions being compared in back-bdb and back-ldbm are
all created by the same malloc function, as is most of the memory used by
slapd. It all comes from one heap, which is a single array. Also for this
reason, problem (a) is not relevant; the range of addresses being compared
will never be so far apart.
-- Howard Chu
Chief Architect, Symas Corp. Director, Highland Sun
Symas: Premier OpenSource Development and Support