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

Dawid Zamirski posted 1 patch 6 years, 4 months ago
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(-)
[libvirt] [PATCH php v2] Fix crash in VIRT_HASH_CURRENT_KEY_INFO macro
Posted by Dawid Zamirski 6 years, 4 months ago
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.
---

v1: https://www.redhat.com/archives/libvir-list/2017-December/msg00151.html

Changes since v1:
 * use zend_ulong in php_libvirt_hash_key_info struct so that no type
   cast is needed

 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..aea43a2 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;
+    zend_ulong index;
     unsigned int type;
 } php_libvirt_hash_key_info;
 
diff --git a/src/util.h b/src/util.h
index ecb3a1f..fcd4075 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, &_info.index, &_pos); \
+    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); \
     } while(0)
 
 #  define VIRT_ARRAY_INIT(_name) do {\
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH php v2] Fix crash in VIRT_HASH_CURRENT_KEY_INFO macro
Posted by Michal Privoznik 6 years, 4 months ago
On 12/06/2017 05:54 PM, 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.
> ---
> 
> v1: https://www.redhat.com/archives/libvir-list/2017-December/msg00151.html
> 
> Changes since v1:
>  * use zend_ulong in php_libvirt_hash_key_info struct so that no type
>    cast is needed
> 
>  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(-)

ACKed and pushed.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list