Segmentation fault merging keys of maps containing maps about ext-ds HOT 7 CLOSED

@rtheunissen

The problem is here I think:

void ds_htable_put(ds_htable_t *table, zval *key, zval *value)
{
   ds_htable_bucket_t *bucket;

   // Attempt to find the bucket or initialize it as a new bucket.
   bool found = ds_htable_lookup_or_next(table, key, &bucket);

   // If found, destruct the current value so that we can replace it.
   if (found) {
       zval_ptr_dtor(&bucket->value);
   }

   if (value) {
       ZVAL_COPY(&bucket->value, value);
   }
}

If found is true but value is NULL, then a stale zval will be in bucket->value, every further operation will keep destroying an already-destroyed zval.

I think, add ZVAL_UNDEF(&bucket->value);:

    // If found, destruct the current value so that we can replace it.
    if (found) {
        zval_ptr_dtor(&bucket->value);
        ZVAL_UNDEF(&bucket->value);
    }

At least this fixes the issue for me.

from ext-ds.

I've added them here and tagged the tests as v1.5.0: php-ds/tests@c16f1ad

from ext-ds.

I've reduced this to:

$map = new Map();
$map->put('issue #200', new Map());
$map->keys()->merge($map->keys());

Invalid read of size 4
==206==    at 0x56602C: zval_ptr_dtor (in /usr/local/bin/php)
==206==    by 0x5B5B61F: ds_htable_clear_buffer.part.0 (ds_htable.c:484)
==206==    by 0x5B5C16B: ds_htable_clear_buffer (ds_htable.c:479)
==206==    by 0x5B5C16B: ds_htable_free (ds_htable.c:508)
==206==    by 0x5B5DF57: ds_set_free (ds_set.c:263)
==206==    by 0x5B6352B: php_ds_set_free_object (php_set_handlers.c:60)
==206==    by 0x6115EF: zend_objects_store_del (in /usr/local/bin/php)

from ext-ds.

DS_HTABLE_FOREACH_BUCKET(table, bucket) {
    DS_HTABLE_BUCKET_DELETE(bucket);
}
DS_HTABLE_FOREACH_END();

#define DS_HTABLE_BUCKET_DELETE(b)                          \
    DTOR_AND_UNDEF(&(b)->value);                            \
    DTOR_AND_UNDEF(&(b)->key);                              \
    DS_HTABLE_BUCKET_NEXT((b)) = DS_HTABLE_INVALID_INDEX

Removing DTOR_AND_UNDEF(&(b)->value) hides the issue, so it's related to the destructing of the values in the hash table of the set, which comes from cloning the hash table of the map when ->keys() is called. As far as I can tell, the code looks good to me. The table clone uses ZVAL_COPY which increments the refcount correctly to my understanding, so I'm not sure why this bug is happening. I'll need to take a closer look.

Thank you for reporting this.

from ext-ds.

Filed a PR: #202

from ext-ds.

Maybe this is already done, but should there be regressions tests added ffor the two issues @nielsdos merged PRs for?

from ext-ds.

Thanks for your work on this!

from ext-ds.