9:42 a.m.
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
9:42 a.m.
New subject: [PATCH 2/4] php: move common code in helper functions
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
9:42 a.m.
New subject: [PATCH 3/4] php: fix invalid memory access with stringlist params
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
9:42 a.m.
New subject: [PATCH 4/4] php: fix memory leak in OStringList optargs
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
7:54 a.m.
New subject: [PATCH 4/4] php: fix memory leak in OStringList optargs
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
3606
days inactive
3607
days old
4 comments
2 participants
participants (2)
-
Pino Toscano -
Richard W.M. Jones