zend_hash.c fishy code

Yep correct. We always prefer to also check p->h so that we minimize
the chances for reaching the memcmp()...

At 04:33 PM 1/29/2006, Sara Golemon wrote:

I wonder If I am completely missing the point, but the following piece
of code seems fishy to me:

ZEND_API int zend_hash_del_key_or_index(HashTable *ht, char *arKey, uint
nKeyLength, ulong h, int flag)
{
uint nIndex;
Bucket *p;

  ...

   while (p != NULL) {
           if ((p->h == h) && ((p->nKeyLength == 0) || /* Numeric

index */
((p->nKeyLength == nKeyLength) &&
(!memcmp(p->arKey, arKey, nKeyLength))))) {
HANDLE_BLOCK_INTERRUPTIONS();
...

If I am not completely mistaken this means: If this is a bucket with a
numeric index with the hash value of the key we want to delete, then
delete it, even when we wanted to delete a key with a string as index.
There's a fairly important bit in that first ...:
if (flag == HASH_DEL_KEY) {
h = zend_inline_hash_func(arKey, nKeyLength);
}

When deleting a numeric index nKeyLength is passed as zero and h
contains the numeric index so this if only matches when p->h == h
and p->nKeyLength == 0 (indicating a numericly keyed bucket).

When deleting a string index, nKeyLength is non-zero and h as passed
is unimportant since it's overridden with the actual hash of arKey
and nKeyLength. The if then only evaluates true when the bucket
hash matches the computed hash and the real key data in the bucket
matches the real key data passed.

Yes, comparing p->h to h seems unnecessary since p->arKey is being
compared to arKey anyway, but that seemingly redundant comparison
saves a more costly memcmp() when the hashes differ.

-Sara

Hello Sara,

you are missing my point. My point is that when a hashtable contains
these two elements

example:

BUCKET_ENTRY for h=15
--- Bucket1 : key == numeric -> h= numeric hash value == 15
-------- Bucket2: key == some string key, with a hash value equal
to 15

Lets assume we want to delete the key "THI_HASH_A_HASH_FUNCTION_VALUE_OF_15"
then the code in question will first hash it and gets h==15. The next
thing it will do is go through the linked list of buckets for h==15.
Unfortunately the check there is broken. It will first check, that p->h
is == 15 and then check if p->nKeyLength==0 which obviously
is for our Bucket1. Unfortunately this is already enough to evaluate to
true. But it is not what we intended. We wanted to delete bucket2.
But we end up with Bucket1 deleted.

Stefan