[libvirt] [libvirt-php PATCH] Fix crash in VIRT_HASH_CURRENT_KEY_INFO macro

Michal Privoznik mprivozn at redhat.com
Wed Dec 6 15:21:17 UTC 2017


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




More information about the libvir-list mailing list