On 12/06/2017 12:17 AM, Dawid Zamirski wrote:

The PHP7 variant of the macro wasn't safe if the hash key was not a string type. This was found when running php script with just libvirt_connect call under xdebug session which segfaulted. This patch makes the following changes:

* make sure that tmp_name is initialized to NULL * set the key name only when zend_hash_get_current_key_ex did set it to something which happens only when type is HASH_KEY_IS_STRING * stash the key index in out php_libvirt_hash_key_info struct because it wasn't there before and separate variable had to be used. --- src/libvirt-connection.c | 8 +++----- src/libvirt-php.c | 6 ++---- src/libvirt-php.h | 1 + src/util.h | 16 +++++++++------- 4 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/src/libvirt-connection.c b/src/libvirt-connection.c index 181b266..2d59d82 100644 --- a/src/libvirt-connection.c +++ b/src/libvirt-connection.c @@ -131,8 +131,6 @@ PHP_FUNCTION(libvirt_connect) HashPosition pointer; int array_count;

- zend_ulong index; - unsigned long libVer;

if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "|sba", &url, &url_len, &readonly, &zcreds) == FAILURE) { @@ -176,13 +174,13 @@ PHP_FUNCTION(libvirt_connect) VIRT_FOREACH(arr_hash, pointer, data) { if (Z_TYPE_P(data) == IS_STRING) { php_libvirt_hash_key_info info; - VIRT_HASH_CURRENT_KEY_INFO(arr_hash, pointer, index, info); + VIRT_HASH_CURRENT_KEY_INFO(arr_hash, pointer, info);

if (info.type == HASH_KEY_IS_STRING) { PHPWRITE(info.name, info.length); } else { - DPRINTF("%s: credentials index %d\n", PHPFUNC, (int)index); - creds[j].type = index; + DPRINTF("%s: credentials index %d\n", PHPFUNC, info.index); + creds[j].type = info.index; creds[j].result = (char *)emalloc(Z_STRLEN_P(data) + 1); memset(creds[j].result, 0, Z_STRLEN_P(data) + 1); creds[j].resultlen = Z_STRLEN_P(data); diff --git a/src/libvirt-php.c b/src/libvirt-php.c index ef057fe..efbef58 100644 --- a/src/libvirt-php.c +++ b/src/libvirt-php.c @@ -1921,7 +1921,6 @@ long get_next_free_numeric_value(virDomainPtr domain, char *xpath) HashPosition pointer; // int array_count; zval *data; - unsigned long index; long max_slot = -1;

xml = virDomainGetXMLDesc(domain, VIR_DOMAIN_XML_INACTIVE); @@ -1934,7 +1933,7 @@ long get_next_free_numeric_value(virDomainPtr domain, char *xpath) VIRT_FOREACH(arr_hash, pointer, data) { if (Z_TYPE_P(data) == IS_STRING) { php_libvirt_hash_key_info info; - VIRT_HASH_CURRENT_KEY_INFO(arr_hash, pointer, index, info); + VIRT_HASH_CURRENT_KEY_INFO(arr_hash, pointer, info);

if (info.type != HASH_KEY_IS_STRING) { long num = -1; @@ -2439,7 +2438,6 @@ void parse_array(zval *arr, tVMDisk *disk, tVMNetwork *network) zval *data; php_libvirt_hash_key_info key; HashPosition pointer; - unsigned long index;

arr_hash = Z_ARRVAL_P(arr); //array_count = zend_hash_num_elements(arr_hash); @@ -2451,7 +2449,7 @@ void parse_array(zval *arr, tVMDisk *disk, tVMNetwork *network)

VIRT_FOREACH(arr_hash, pointer, data) { if ((Z_TYPE_P(data) == IS_STRING) || (Z_TYPE_P(data) == IS_LONG)) { - VIRT_HASH_CURRENT_KEY_INFO(arr_hash, pointer, index, key); + VIRT_HASH_CURRENT_KEY_INFO(arr_hash, pointer, key); if (key.type == HASH_KEY_IS_STRING) { if (disk != NULL) { if ((Z_TYPE_P(data) == IS_STRING) && strcmp(key.name, "path") == 0) diff --git a/src/libvirt-php.h b/src/libvirt-php.h index 8d13a6b..f24a329 100644 --- a/src/libvirt-php.h +++ b/src/libvirt-php.h @@ -137,6 +137,7 @@ typedef struct tVMNetwork { typedef struct _php_libvirt_hash_key_info { char *name; unsigned int length; + unsigned int index;

This needs to be zend_ulong type. Thing is, zend_hash_get_current_key_ex() expects zend_ulong *. In both version 7 and 5. If we do that we can drop the typecast done ...

unsigned int type; } php_libvirt_hash_key_info;

diff --git a/src/util.h b/src/util.h index ecb3a1f..72cfa91 100644 --- a/src/util.h +++ b/src/util.h @@ -135,12 +135,14 @@

# define VIRT_FOREACH_END(_dummy)

-# define VIRT_HASH_CURRENT_KEY_INFO(_ht, _pos, _idx, _info) \ +# define VIRT_HASH_CURRENT_KEY_INFO(_ht, _pos, _info) \ do { \ - zend_string *tmp_key_info; \ - _info.type = zend_hash_get_current_key_ex(_ht, &tmp_key_info, &_idx, &_pos); \ - _info.name = ZSTR_VAL(tmp_key_info); \ - _info.length = ZSTR_LEN(tmp_key_info); \ + zend_string *tmp_name = NULL; \ + _info.type = zend_hash_get_current_key_ex(_ht, &tmp_name, (zend_ulong *) &_info.index, &_pos); \

.. here,

+ if (tmp_name) { \ + _info.name = ZSTR_VAL(tmp_name); \ + _info.length = ZSTR_LEN(tmp_name); \ + } \ } while(0)

# define VIRT_ARRAY_INIT(_name) do { \ @@ -213,9 +215,9 @@ # define VIRT_FOREACH_END(_dummy) \ }}

-# define VIRT_HASH_CURRENT_KEY_INFO(_ht, _pos, _idx, _info) \ +# define VIRT_HASH_CURRENT_KEY_INFO(_ht, _pos, _info) \ do { \ - _info.type = zend_hash_get_current_key_ex(_ht, &_info.name, &_info.length, &_idx, 0, &_pos); \ + _info.type = zend_hash_get_current_key_ex(_ht, &_info.name, &_info.length, &_info.index, 0, &_pos); \

but not here. Does that work for you? Michal