Use zend_string for DBA path #10698
Use zend_string for DBA path #10698
Conversation
ext/dba/dba.c
Outdated
| ZEND_ASSERT(info->path); | ||
| // Cannot use zend_string_release_ex(info->path, info->flags&DBA_PERSISTENT); as this fails GC assertion? | ||
| zend_string_free(info->path); | ||
| info->path = NULL; |
There was a problem hiding this comment.
Which test is failing the assertion?
If the failing assertion is ZEND_ASSERT(GC_FLAGS(s) & IS_STR_PERSISTENT);, there is a discrepancy between the persistent flag in info->flags and whether info->path is actually persistent.
zend_string_release() (non _ex variant) would be the right function to use here. zend_string_release_ex() is an optimization for when the persistent argument is known at compile time (removes a branch).
zend_string_free() is also suitable, as long as refcount is 1, but this seems less maintainable.
There was a problem hiding this comment.
The failling assertion is:
php: /home/girgias/Dev/php-src/Zend/zend_rc_debug.c:38: void ZEND_RC_MOD_CHECK(const zend_refcounted_h *): Assertion `(zval_gc_flags(p->u.type_info) & ((1<<7)|(1<<8))) != (1<<7)' failed.
There was a problem hiding this comment.
Ah, thank you! I didn't have the ZEND_RC_DEBUG flag.
ZEND_RC_MOD_CHECK is complaining that we should not touch the refcount of a PERSISTENT string, under the expectation that PERSISTENT strings are not refcounted.
We can disable this check by using GC_MAKE_PERSISTENT_LOCAL(s) in php_dba_zend_string_dup_safe when persistent is true.
We also need to change php_dba_zend_string_dup_safe so that PERSISTENT strings are duplicated.
There was a problem hiding this comment.
Thank you for the explanations! Will do this now. :)
ext/dba/dba.c
Outdated
| /* replace the path info with the real path of the opened file */ | ||
| pefree(info->path, persistent); | ||
| info->path = pestrndup(ZSTR_VAL(opened_path), ZSTR_LEN(opened_path), persistent); | ||
| zend_string_release_ex(info->path, persistent); |
There was a problem hiding this comment.
We don't need zend_string_release_ex() here, we can use zend_string_release(), as it knows whether the str is persistent or not
ext/dba/dba.c
Outdated
| @@ -724,7 +725,7 @@ static void php_dba_open(INTERNAL_FUNCTION_PARAMETERS, bool persistent) | |||
|
|
|||
| info = pemalloc(sizeof(dba_info), persistent); | |||
| memset(info, 0, sizeof(dba_info)); | |||
| info->path = pestrdup(ZSTR_VAL(path), persistent); | |||
| info->path = zend_string_dup(path, persistent); | |||
There was a problem hiding this comment.
After looking at #9768, I think that we shouldn't use zend_string_dup when persistent might be true.
There was a problem hiding this comment.
This is confusing, but okay I see the issue :-/
Doing this because I'm struggling to do the conversion from resource to opaque object for persistent resources.
Which also makes me wonder if there isn't an existing underlying bug somewhere because I cannot use
zend_string_release_ex(info->path, info->flags&DBA_PERSISTENT);indba_close()as the refcount seems to already be 0 for persistent resources. Something I really don't understand. So if anyone has any ideas...