# zend_hash.c fishy code

- Thread: https://externals.io/message/21726
- Original archive: https://news-web.php.net/php.internals/21726
- Started by Stefan Esser on 2006-01-29T21:01:20+00:00
- Messages: 5
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #21726 — Stefan Esser — 2006-01-29T21:01:20+00:00

URL: https://externals.io/message/21726#21726

Good Morning,

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.

Stefan

---

## #21728 — Sara Golemon — 2006-01-30T00:33:43+00:00

URL: https://externals.io/message/21726#21728 · In reply to: #21726

> 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

---

## #21729 — Andi Gutmans — 2006-01-30T03:49:28+00:00

URL: https://externals.io/message/21726#21729 · In reply to: #21728

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:
> > […]
>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

---

## #21733 — Stefan Esser — 2006-01-30T09:08:30+00:00

URL: https://externals.io/message/21726#21733 · In reply to: #21728

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

---

## #21735 — Sara Golemon — 2006-01-30T17:08:50+00:00

URL: https://externals.io/message/21726#21735 · In reply to: #21733

> 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.
>
Ah, yes, well, when you put it that way.  I can absolutely see that taking 
place.  What's worse, since DJBX33A isn't collision resistant (just 
collision avoidant) it'd be possible (in theory) to deliberately clobber a 
record by taking advantage of this (though not trivial).

-Sara