Libguestfs - Feb 2015 - [PATCH 1/4] php: fix invalid memory access with OptString

OptString maps to a "s!" argument, which makes zend_parse_parameters
not
touch the variables (char* and length) when NULL is passed as parameter.
Hence, set both to NULL/0, and check for non-NULL char* variable before
checking its length.
---
 generator/php.ml | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/generator/php.ml b/generator/php.ml
index 0ef6541..1384451 100644
--- a/generator/php.ml
+++ b/generator/php.ml
@@ -192,11 +192,13 @@ PHP_FUNCTION (guestfs_last_error)
         | String n | Device n | Mountable n | Pathname n
         | Dev_or_Path n | Mountable_or_Path n
         | FileIn n | FileOut n | Key n
-        | OptString n
         | BufferIn n
         | GUID n ->
             pr "  char *%s;\n" n;
             pr "  int %s_size;\n" n
+        | OptString n ->
+            pr "  char *%s = NULL;\n" n;
+            pr "  int %s_size;\n" n
         | StringList n
         | DeviceList n ->
             pr "  zval *z_%s;\n" n;
@@ -310,7 +312,7 @@ PHP_FUNCTION (guestfs_last_error)
         | String n | Device n | Mountable n | Pathname n
         | Dev_or_Path n | Mountable_or_Path n
         | FileIn n | FileOut n | Key n
-        | OptString n | GUID n ->
+        | GUID n ->
             (* Just need to check the string doesn't contain any ASCII
              * NUL characters, which won't be supported by the C API.
              *)
@@ -319,6 +321,15 @@ PHP_FUNCTION (guestfs_last_error)
             pr "    RETURN_FALSE;\n";
             pr "  }\n";
             pr "\n"
+        | OptString n ->
+            (* Just need to check the string doesn't contain any ASCII
+             * NUL characters, which won't be supported by the C API.
+             *)
+            pr "  if (%s != NULL && strlen (%s) != %s_size)
{\n" n n n;
+            pr "    fprintf (stderr, \"libguestfs: %s: parameter
'%s' contains embedded ASCII NUL.\\n\");\n" shortname n;
+            pr "    RETURN_FALSE;\n";
+            pr "  }\n";
+            pr "\n"
         | BufferIn n -> ()
         | StringList n
         | DeviceList n ->
-- 
1.9.3

Simple code motion.
---
 generator/php.ml | 95 +++++++++++++++++++++++++-------------------------------
 1 file changed, 42 insertions(+), 53 deletions(-)

diff --git a/generator/php.ml b/generator/php.ml
index 1384451..69f627f 100644
--- a/generator/php.ml
+++ b/generator/php.ml
@@ -91,6 +91,45 @@ and generate_php_c ()  
 static int res_guestfs_h;
 
+/* Convert array to list of strings.
+ * http://marc.info/?l=pecl-dev&m=112205192100631&w=2
+ */
+static char**
+get_stringlist (zval *val)
+{
+  char **ret;
+  HashTable *a;
+  int n;
+  HashPosition p;
+  zval **d;
+  size_t c = 0;
+
+  a = Z_ARRVAL_P (val);
+  n = zend_hash_num_elements (a);
+  ret = safe_emalloc (n + 1, sizeof (char *), 0);
+  for (zend_hash_internal_pointer_reset_ex (a, &p);
+       zend_hash_get_current_data_ex (a, (void **) &d, &p) == SUCCESS;
+       zend_hash_move_forward_ex (a, &p)) {
+    zval t = **d;
+    zval_copy_ctor (&t);
+    convert_to_string (&t);
+    ret[c] = Z_STRVAL (t);
+    c++;
+  }
+  ret[c] = NULL;
+  return ret;
+}
+
+static void
+guestfs_efree_stringlist (char **p)
+{
+  size_t c = 0;
+
+  for (c = 0; p[c] != NULL; ++c)
+    efree (p[c]);
+  efree (p);
+}
+
 static void
 guestfs_php_handle_dtor (zend_rsrc_list_entry *rsrc TSRMLS_DC)
 {
@@ -333,30 +372,7 @@ PHP_FUNCTION (guestfs_last_error)
         | BufferIn n -> ()
         | StringList n
         | DeviceList n ->
-            (* Convert array to list of strings.
-             * http://marc.info/?l=pecl-dev&m=112205192100631&w=2
-             *)
-            pr "  {\n";
-            pr "    HashTable *a;\n";
-            pr "    int n;\n";
-            pr "    HashPosition p;\n";
-            pr "    zval **d;\n";
-            pr "    size_t c = 0;\n";
-            pr "\n";
-            pr "    a = Z_ARRVAL_P (z_%s);\n" n;
-            pr "    n = zend_hash_num_elements (a);\n";
-            pr "    %s = safe_emalloc (n + 1, sizeof (char *), 0);\n"
n;
-            pr "    for (zend_hash_internal_pointer_reset_ex (a,
&p);\n";
-            pr "         zend_hash_get_current_data_ex (a, (void **)
&d, &p) == SUCCESS;\n";
-            pr "         zend_hash_move_forward_ex (a, &p)) {\n";
-            pr "      zval t = **d;\n";
-            pr "      zval_copy_ctor (&t);\n";
-            pr "      convert_to_string (&t);\n";
-            pr "      %s[c] = Z_STRVAL (t);\n" n;
-            pr "      c++;\n";
-            pr "    }\n";
-            pr "    %s[c] = NULL;\n" n;
-            pr "  }\n";
+            pr "  %s = get_stringlist (z_%s);\n" n n;
             pr "\n"
         | Bool _ | Int _ | Int64 _ -> ()
         | Pointer (t, n) ->
@@ -391,28 +407,7 @@ PHP_FUNCTION (guestfs_last_error)
             pr "   * positively check that it gave us an array, otherwise
ignore it.\n";
             pr "   */\n";
             pr "  if (optargs_t_%s != NULL && Z_TYPE_P
(optargs_t_%s) == IS_ARRAY) {\n" n n;
-            pr "    char **r;\n";
-            pr "    HashTable *a;\n";
-            pr "    int n;\n";
-            pr "    HashPosition p;\n";
-            pr "    zval **d;\n";
-            pr "    size_t c = 0;\n";
-            pr "\n";
-            pr "    a = Z_ARRVAL_P (optargs_t_%s);\n" n;
-            pr "    n = zend_hash_num_elements (a);\n";
-            pr "    r = safe_emalloc (n + 1, sizeof (char *), 0);\n";
-            pr "    for (zend_hash_internal_pointer_reset_ex (a,
&p);\n";
-            pr "         zend_hash_get_current_data_ex (a, (void **)
&d, &p) == SUCCESS;\n";
-            pr "         zend_hash_move_forward_ex (a, &p)) {\n";
-            pr "      zval t = **d;\n";
-            pr "      zval_copy_ctor (&t);\n";
-            pr "      convert_to_string (&t);\n";
-            pr "      r[c] = Z_STRVAL (t);\n";
-            pr "      c++;\n";
-            pr "    }\n";
-            pr "    r[c] = NULL;\n";
-            pr "\n";
-            pr "    optargs_s.%s = r;\n" n;
+            pr "    optargs_s.%s = get_stringlist (optargs_t_%s);\n"
n n;
             pr "    optargs_s.bitmask |= %s_%s_BITMASK;\n"
c_optarg_prefix uc_n;
             pr "  }\n";
         ) optargs;
@@ -458,13 +453,7 @@ PHP_FUNCTION (guestfs_last_error)
         | BufferIn n -> ()
         | StringList n
         | DeviceList n ->
-            pr "  {\n";
-            pr "    size_t c = 0;\n";
-            pr "\n";
-            pr "    for (c = 0; %s[c] != NULL; ++c)\n" n;
-            pr "      efree (%s[c]);\n" n;
-            pr "    efree (%s);\n" n;
-            pr "  }\n";
+            pr "  guestfs_efree_stringlist (%s);\n" n;
             pr "\n"
         | Bool _ | Int _ | Int64 _ | Pointer _ -> ()
         ) args;
-- 
1.9.3

Make sure to copy the strings we add to the char** array, otherwise they
are stale pointers which we'll try to free later.

Also, properly destruct the temporary zval.
---
 generator/php.ml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/generator/php.ml b/generator/php.ml
index 69f627f..28903d0 100644
--- a/generator/php.ml
+++ b/generator/php.ml
@@ -113,7 +113,8 @@ get_stringlist (zval *val)
     zval t = **d;
     zval_copy_ctor (&t);
     convert_to_string (&t);
-    ret[c] = Z_STRVAL (t);
+    ret[c] = estrndup (Z_STRVAL(t), Z_STRLEN (t));
+    zval_dtor (&t);
     c++;
   }
   ret[c] = NULL;
-- 
1.9.3

Make sure to free the char** created to convert the arguments.
---
 generator/php.ml | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/generator/php.ml b/generator/php.ml
index 28903d0..b49bf60 100644
--- a/generator/php.ml
+++ b/generator/php.ml
@@ -458,6 +458,16 @@ PHP_FUNCTION (guestfs_last_error)
             pr "\n"
         | Bool _ | Int _ | Int64 _ | Pointer _ -> ()
         ) args;
+      List.iter (
+        function
+        | OBool n | OInt n | OInt64 n | OString n -> ()
+        | OStringList n ->
+            let uc_n = String.uppercase n in
+            pr "  if ((optargs_s.bitmask & %s_%s_BITMASK) !=
0)\n"
+              c_optarg_prefix uc_n;
+            pr "    guestfs_efree_stringlist ((char **)
optargs_s.%s);\n" n;
+            pr "\n"
+      ) optargs;
 
       (* Check for errors. *)
       (match errcode_of_ret ret with
-- 
1.9.3

On Tue, Feb 10, 2015 at 04:42:55PM +0100, Pino Toscano
wrote:
> Make sure to free the char** created to convert the arguments.
> ---
>  generator/php.ml | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/generator/php.ml b/generator/php.ml
> index 28903d0..b49bf60 100644
> --- a/generator/php.ml
> +++ b/generator/php.ml
> @@ -458,6 +458,16 @@ PHP_FUNCTION (guestfs_last_error)
>              pr "\n"
>          | Bool _ | Int _ | Int64 _ | Pointer _ -> ()
>          ) args;
> +      List.iter (
> +        function
> +        | OBool n | OInt n | OInt64 n | OString n -> ()
> +        | OStringList n ->
> +            let uc_n = String.uppercase n in
> +            pr "  if ((optargs_s.bitmask & %s_%s_BITMASK) !=
0)\n"
> +              c_optarg_prefix uc_n;
> +            pr "    guestfs_efree_stringlist ((char **)
optargs_s.%s);\n" n;
> +            pr "\n"
> +      ) optargs;
>  
>        (* Check for errors. *)
>        (match errcode_of_ret ret with
ACK series.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW