Libguestfs - May 2017 - [PATCH] RHBZ#1406906: check return value of Python object functions

Verify the returned values of Python Object constructor functions
are not NULL before adding them to a collection.

This is particularly relevant when constructing Unicode strings in
Python 3 as they will return NULL if non UTF-8 characters are present.

Signed-off-by: Matteo Cafasso <noxdafox@gmail.com>
---
 generator/python.ml            | 102 ++++++++++++++++++++++++++---------------
 python/handle.c                |  34 +++++++++++---
 python/t/test830RHBZ1406906.py |  51 +++++++++++++++++++++
 3 files changed, 145 insertions(+), 42 deletions(-)
 create mode 100644 python/t/test830RHBZ1406906.py

diff --git a/generator/python.ml b/generator/python.ml
index cf0829489..4cae24757 100644
--- a/generator/python.ml
+++ b/generator/python.ml
@@ -155,12 +155,20 @@ and generate_python_structs ()      pr "PyObject
*\n";
     pr "guestfs_int_py_put_%s_list (struct guestfs_%s_list *%ss)\n"
typ typ typ;
     pr "{\n";
-    pr "  PyObject *list;\n";
+    pr "  PyObject *list, *element;\n";
     pr "  size_t i;\n";
     pr "\n";
     pr "  list = PyList_New (%ss->len);\n" typ;
-    pr "  for (i = 0; i < %ss->len; ++i)\n" typ;
-    pr "    PyList_SetItem (list, i, guestfs_int_py_put_%s
(&%ss->val[i]));\n" typ typ;
+    pr "  if (list == NULL)\n";
+    pr "    return NULL;\n";
+    pr "  for (i = 0; i < %ss->len; ++i) {\n" typ;
+    pr "    element = guestfs_int_py_put_%s (&%ss->val[i]);\n"
typ typ;
+    pr "    if (element == NULL) {\n";
+    pr "      Py_CLEAR (list);\n";
+    pr "      return NULL;\n";
+    pr "    }\n";
+    pr "    PyList_SetItem (list, i, element);\n";
+    pr "  }\n";
     pr "  return list;\n";
     pr "};\n";
     pr "#endif\n";
@@ -174,54 +182,72 @@ and generate_python_structs ()        pr "PyObject
*\n";
       pr "guestfs_int_py_put_%s (struct guestfs_%s *%s)\n" typ typ
typ;
       pr "{\n";
-      pr "  PyObject *dict;\n";
+      pr "  PyObject *dict, *value;\n";
       pr "\n";
       pr "  dict = PyDict_New ();\n";
+      pr "  if (dict == NULL)\n";
+      pr "    return NULL;\n";
       List.iter (
         function
         | name, FString ->
-            pr "  PyDict_SetItemString (dict, \"%s\",\n"
name;
-            pr "                        guestfs_int_py_fromstring
(%s->%s));\n"
-              typ name
+            pr "  value = guestfs_int_py_fromstring (%s->%s);" typ
name;
+            pr "  if (value == NULL)\n";
+            pr "    goto err;\n";
+            pr "  PyDict_SetItemString (dict, \"%s\",
value);\n" name;
         | name, FBuffer ->
-            pr "  PyDict_SetItemString (dict, \"%s\",\n"
name;
-            pr "                        guestfs_int_py_fromstringsize
(%s->%s, %s->%s_len));\n"
-              typ name typ name
+            pr "  value = guestfs_int_py_fromstringsize (%s->%s,
%s->%s_len);\n"
+              typ name typ name;
+            pr "  if (value == NULL)\n";
+            pr "    goto err;\n";
+            pr "  PyDict_SetItemString (dict, \"%s\",
value);\n" name;
         | name, FUUID ->
-            pr "  PyDict_SetItemString (dict, \"%s\",\n"
name;
-            pr "                        guestfs_int_py_fromstringsize
(%s->%s, 32));\n"
-              typ name
+            pr "  value = guestfs_int_py_fromstringsize (%s->%s,
32);\n"
+              typ name;
+            pr "  if (value == NULL)\n";
+            pr "    goto err;\n";
+            pr "  PyDict_SetItemString (dict, \"%s\",
value);\n" name;
         | name, (FBytes|FUInt64) ->
-            pr "  PyDict_SetItemString (dict, \"%s\",\n"
name;
-            pr "                        PyLong_FromUnsignedLongLong
(%s->%s));\n"
-              typ name
+            pr "  value = PyLong_FromUnsignedLongLong (%s->%s);\n"
typ name;
+            pr "  if (value == NULL)\n";
+            pr "    goto err;\n";
+            pr "  PyDict_SetItemString (dict, \"%s\",
value);\n" name;
         | name, FInt64 ->
-            pr "  PyDict_SetItemString (dict, \"%s\",\n"
name;
-            pr "                        PyLong_FromLongLong
(%s->%s));\n"
-              typ name
+            pr "  value = PyLong_FromLongLong (%s->%s);\n" typ
name;
+            pr "  if (value == NULL)\n";
+            pr "    goto err;\n";
+            pr "  PyDict_SetItemString (dict, \"%s\",
value);\n" name;
         | name, FUInt32 ->
-            pr "  PyDict_SetItemString (dict, \"%s\",\n"
name;
-            pr "                        PyLong_FromUnsignedLong
(%s->%s));\n"
-              typ name
+            pr "  value = PyLong_FromUnsignedLong (%s->%s);\n" typ
name;
+            pr "  if (value == NULL)\n";
+            pr "    goto err;\n";
+            pr "  PyDict_SetItemString (dict, \"%s\",
value);\n" name;
         | name, FInt32 ->
-            pr "  PyDict_SetItemString (dict, \"%s\",\n"
name;
-            pr "                        PyLong_FromLong
(%s->%s));\n"
-              typ name
+            pr "  value = PyLong_FromLong (%s->%s);\n" typ name;
+            pr "  if (value == NULL)\n";
+            pr "    goto err;\n";
+            pr "  PyDict_SetItemString (dict, \"%s\",
value);\n" name;
         | name, FOptPercent ->
-            pr "  if (%s->%s >= 0)\n" typ name;
-            pr "    PyDict_SetItemString (dict, \"%s\",\n"
name;
-            pr "                          PyFloat_FromDouble ((double)
%s->%s));\n"
-              typ name;
+            pr "  if (%s->%s >= 0) {\n" typ name;
+            pr "    value = PyFloat_FromDouble ((double)
%s->%s);\n" typ name;
+            pr "    if (value == NULL)\n";
+            pr "      goto err;\n";
+            pr "    PyDict_SetItemString (dict, \"%s\",
value);\n" name;
+            pr "  }\n";
             pr "  else {\n";
             pr "    Py_INCREF (Py_None);\n";
             pr "    PyDict_SetItemString (dict, \"%s\",
Py_None);\n" name;
             pr "  }\n"
         | name, FChar ->
-            pr "  PyDict_SetItemString (dict, \"%s\",\n"
name;
-            pr "                        guestfs_int_py_fromstringsize
(&%s->%s, 1));\n"
-              typ name
+            pr "  value = guestfs_int_py_fromstringsize (&%s->%s,
1);\n"
+              typ name;
+            pr "  if (value == NULL)\n";
+            pr "    goto err;\n";
+            pr "  PyDict_SetItemString (dict, \"%s\",
value);\n" name;
       ) cols;
       pr "  return dict;\n";
+      pr " err:\n";
+      pr "  Py_CLEAR (dict);\n";
+      pr "  return NULL;\n";
       pr "};\n";
       pr "#endif\n";
       pr "\n";
@@ -472,16 +498,20 @@ and generate_python_actions actions ()             pr
"  if (py_r == NULL) goto out;\n";
        | RStringList _ ->
            pr "  py_r = guestfs_int_py_put_string_list (r);\n";
-           pr "  guestfs_int_free_string_list (r);\n"
+           pr "  guestfs_int_free_string_list (r);\n";
+           pr "  if (py_r == NULL) goto out;\n";
        | RStruct (_, typ) ->
            pr "  py_r = guestfs_int_py_put_%s (r);\n" typ;
-           pr "  guestfs_free_%s (r);\n" typ
+           pr "  guestfs_free_%s (r);\n" typ;
+           pr "  if (py_r == NULL) goto out;\n";
        | RStructList (_, typ) ->
            pr "  py_r = guestfs_int_py_put_%s_list (r);\n" typ;
-           pr "  guestfs_free_%s_list (r);\n" typ
+           pr "  guestfs_free_%s_list (r);\n" typ;
+           pr "  if (py_r == NULL) goto out;\n";
        | RHashtable _ ->
            pr "  py_r = guestfs_int_py_put_table (r);\n";
-           pr "  guestfs_int_free_string_list (r);\n"
+           pr "  guestfs_int_free_string_list (r);\n";
+           pr "  if (py_r == NULL) goto out;\n";
        | RBufferOut _ ->
            pr "  py_r = guestfs_int_py_fromstringsize (r, size);\n";
            pr "  free (r);\n";
diff --git a/python/handle.c b/python/handle.c
index 88024e184..fa6578034 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -324,15 +324,23 @@ guestfs_int_py_get_string_list (PyObject *obj)
 PyObject *
 guestfs_int_py_put_string_list (char * const * const argv)
 {
-  PyObject *list;
+  PyObject *list, *item;
   size_t argc, i;

   for (argc = 0; argv[argc] != NULL; ++argc)
     ;

   list = PyList_New (argc);
+  if (list == NULL)
+    return NULL;
   for (i = 0; i < argc; ++i) {
-    PyList_SetItem (list, i, guestfs_int_py_fromstring (argv[i]));
+    item = guestfs_int_py_fromstring (argv[i]);
+    if (item == NULL) {
+      Py_CLEAR (list);
+      return NULL;
+    }
+
+    PyList_SetItem (list, i, item);
   }

   return list;
@@ -341,21 +349,35 @@ guestfs_int_py_put_string_list (char * const * const argv)
 PyObject *
 guestfs_int_py_put_table (char * const * const argv)
 {
-  PyObject *list, *item;
+  PyObject *list, *tuple, *item;
   size_t argc, i;

   for (argc = 0; argv[argc] != NULL; ++argc)
     ;

   list = PyList_New (argc >> 1);
+  if (list == NULL)
+    return NULL;
   for (i = 0; i < argc; i += 2) {
-    item = PyTuple_New (2);
-    PyTuple_SetItem (item, 0, guestfs_int_py_fromstring (argv[i]));
-    PyTuple_SetItem (item, 1, guestfs_int_py_fromstring (argv[i+1]));
+    tuple = PyTuple_New (2);
+    if (tuple == NULL)
+      goto err;
+    item = guestfs_int_py_fromstring (argv[i]);
+    if (item == NULL)
+      goto err;
+    PyTuple_SetItem (tuple, 0, item);
+    item = guestfs_int_py_fromstring (argv[i+1]);
+    if (item == NULL)
+      goto err;
+    PyTuple_SetItem (tuple, 1, item);
     PyList_SetItem (list, i >> 1, item);
   }

   return list;
+ err:
+  Py_CLEAR (list);
+  Py_CLEAR (tuple);
+  return NULL;
 }

 PyObject *
diff --git a/python/t/test830RHBZ1406906.py b/python/t/test830RHBZ1406906.py
new file mode 100644
index 000000000..6cef982f9
--- /dev/null
+++ b/python/t/test830RHBZ1406906.py
@@ -0,0 +1,51 @@
+# libguestfs Python bindings
+# Copyright (C) 2017 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+import os
+import sys
+import shutil
+import tempfile
+import unittest
+
+import guestfs
+
+
+class Test830RHBZ1406906(unittest.TestCase):
+    def setUp(self):
+        self.tempdir = tempfile.mkdtemp()
+
+    def tearDown(self):
+        shutil.rmtree(self.tempdir)
+
+    def test_rhbz1406906(self):
+        g = guestfs.GuestFS(python_return_dict=True)
+
+        g.add_drive_scratch(512 * 1024 * 1024)
+        g.launch()
+
+        g.part_disk("/dev/sda", "mbr")
+        g.mkfs("ext4", "/dev/sda1")
+        g.mount("/dev/sda1", "/")
+
+        # touch file with illegal unicode character
+        open(os.path.join(self.tempdir, "\udcd4"),
"w").close()
+
+        g.copy_in(self.tempdir, "/")
+
+        if sys.version_info.major == 3:
+            with self.assertRaises(UnicodeDecodeError):
+                g.find("/")  # segfault here on Python 3
--
2.11.0

On Tuesday, 9 May 2017 23:05:46 CEST Matteo Cafasso
wrote:
> Verify the returned values of Python Object constructor functions
> are not NULL before adding them to a collection.
> 
> This is particularly relevant when constructing Unicode strings in
> Python 3 as they will return NULL if non UTF-8 characters are present.
> 
> Signed-off-by: Matteo Cafasso <noxdafox@gmail.com>
> ---
This mostly looks good (after my refactoring, this patch is readable
now).  There are a couple of issues.
> diff --git a/python/handle.c b/python/handle.c
> index 88024e184..fa6578034 100644
> --- a/python/handle.c
> +++ b/python/handle.c
> @@ -341,21 +349,35 @@ guestfs_int_py_put_string_list (char * const * const
argv)
>  PyObject *
>  guestfs_int_py_put_table (char * const * const argv)
>  {
> -  PyObject *list, *item;
> +  PyObject *list, *tuple, *item;
>    size_t argc, i;
> 
>    for (argc = 0; argv[argc] != NULL; ++argc)
>      ;
> 
>    list = PyList_New (argc >> 1);
> +  if (list == NULL)
> +    return NULL;
>    for (i = 0; i < argc; i += 2) {
> -    item = PyTuple_New (2);
> -    PyTuple_SetItem (item, 0, guestfs_int_py_fromstring (argv[i]));
> -    PyTuple_SetItem (item, 1, guestfs_int_py_fromstring (argv[i+1]));
> +    tuple = PyTuple_New (2);
> +    if (tuple == NULL)
> +      goto err;
> +    item = guestfs_int_py_fromstring (argv[i]);
> +    if (item == NULL)
> +      goto err;
> +    PyTuple_SetItem (tuple, 0, item);
> +    item = guestfs_int_py_fromstring (argv[i+1]);
> +    if (item == NULL)
> +      goto err;
> +    PyTuple_SetItem (tuple, 1, item);
>      PyList_SetItem (list, i >> 1, item);
This PyList_SetItem must be changed too, otherwise 'tuple' is not used.
This was caught by running the test suite for the python binding
(test090RetValues fails with this current version of your patch), so
please make sure to run it and that it still works.
> diff --git a/python/t/test830RHBZ1406906.py
b/python/t/test830RHBZ1406906.py
> new file mode 100644
> index 000000000..6cef982f9
> --- /dev/null
> +++ b/python/t/test830RHBZ1406906.py
> @@ -0,0 +1,51 @@
> +# libguestfs Python bindings
> +# Copyright (C) 2017 Red Hat Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
USA.
> +
> +import os
> +import sys
> +import shutil
> +import tempfile
> +import unittest
> +
> +import guestfs
> +
> +
> +class Test830RHBZ1406906(unittest.TestCase):
> +    def setUp(self):
> +        self.tempdir = tempfile.mkdtemp()
> +
> +    def tearDown(self):
> +        shutil.rmtree(self.tempdir)
> +
> +    def test_rhbz1406906(self):
> +        g = guestfs.GuestFS(python_return_dict=True)
> +
> +        g.add_drive_scratch(512 * 1024 * 1024)
> +        g.launch()
> +
> +        g.part_disk("/dev/sda", "mbr")
> +        g.mkfs("ext4", "/dev/sda1")
> +        g.mount("/dev/sda1", "/")
> +
> +        # touch file with illegal unicode character
> +        open(os.path.join(self.tempdir, "\udcd4"),
"w").close()
> +
> +        g.copy_in(self.tempdir, "/")
> +
> +        if sys.version_info.major == 3:
Elsewhere (python/t/tests_helper.py.in) we use
  sys.version_info >= (3, 0):
already, so it'd be good to use a single style thorough.
> +            with self.assertRaises(UnicodeDecodeError):
> +                g.find("/")  # segfault here on Python 3
Besides the check that it does not crash, IMHO it should also check
something regarding the return value, i.e. that it is returned or not.
A possible idea could be to create a subdirectory in /, change copy_in
to use that subdirectory as destination, list using 'ls' its content,
and compare to a list (empty if the file with such name is not
supported, or with the name if it is).

Also, this check should also be done for Python 2.

-- 
Pino Toscano