Libguestfs - May 2017 - [Bug 1406906] [PATCH 0/3] Fix segmentation fault in Python bindings

This series addresses the issue where non UTF8 file names in a guest image lead
to libguestfs segfault with Python 3 APIs.

The core issue is the APIs are not checking the return value when constructing a
new PyObject. Therefore NULL pointers are added to Python collections (lists and
dictionaries) crashing the application.

Few notes regarding the comments on the previous patch.

- Added a regression test file
- Removed the Exception string setter functions as Python APIs already set them
in case of error
  https://docs.python.org/3.6/c-api/exceptions.html#exception-handling (third
paragraph)
- Fixed functions in python/handle.c
- Correctly clear and decrement the refcount of the list/dictionary object
before returning on error

Matteo Cafasso (3):
  generator: check return value of Python object functions
  python: check return value of object functions in handle.c
  python: add regression test for RHBZ#1406906

 generator/python.ml            | 119 +++++++++++++++++++++++++----------------
 python/handle.c                |  48 +++++++++++++----
 python/t/test830RHBZ1406906.py |  55 +++++++++++++++++++
 3 files changed, 165 insertions(+), 57 deletions(-)
 create mode 100644 python/t/test830RHBZ1406906.py

--
2.11.0

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

Signed-off-by: Matteo Cafasso <noxdafox@gmail.com>
---
 generator/python.ml | 119 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 72 insertions(+), 47 deletions(-)

diff --git a/generator/python.ml b/generator/python.ml
index 0162733f9..ace15d3e0 100644
--- a/generator/python.ml
+++ b/generator/python.ml
@@ -152,12 +152,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";
@@ -171,75 +179,88 @@ 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 "#ifdef HAVE_PYSTRING_ASSTRING\n";
-            pr "                        PyString_FromString
(%s->%s));\n"
-              typ name;
+            pr "  value = PyString_FromString (%s->%s);\n" typ
name;
             pr "#else\n";
-            pr "                        PyUnicode_FromString
(%s->%s));\n"
-              typ name;
-            pr "#endif\n"
+            pr "  value = PyUnicode_FromString (%s->%s);\n" typ
name;
+            pr "#endif\n";
+            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 "#ifdef HAVE_PYSTRING_ASSTRING\n";
-            pr "                        PyString_FromStringAndSize
(%s->%s, %s->%s_len));\n"
+            pr "  value = PyString_FromStringAndSize (%s->%s,
%s->%s_len);\n"
               typ name typ name;
             pr "#else\n";
-            pr "                        PyBytes_FromStringAndSize
(%s->%s, %s->%s_len));\n"
+            pr "  value = PyBytes_FromStringAndSize (%s->%s,
%s->%s_len);\n"
               typ name typ name;
-            pr "#endif\n"
+            pr "#endif\n";
+            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 "#ifdef HAVE_PYSTRING_ASSTRING\n";
-            pr "                        PyString_FromStringAndSize
(%s->%s, 32));\n"
-              typ name;
+            pr "  value = PyString_FromStringAndSize (%s->%s,
32);\n" typ name;
             pr "#else\n";
-            pr "                        PyBytes_FromStringAndSize
(%s->%s, 32));\n"
-              typ name;
-            pr "#endif\n"
+            pr "  value = PyBytes_FromStringAndSize (%s->%s,
32);\n" typ name;
+            pr "#endif\n";
+            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 "#ifdef HAVE_PYSTRING_ASSTRING\n";
-            pr "  PyDict_SetItemString (dict, \"%s\",\n"
name;
-            pr "                        PyString_FromStringAndSize
(&%s->%s, 1));\n"
-              typ name;
+            pr "  value = PyString_FromStringAndSize (&%s->%s,
1);\n" typ name;
             pr "#else\n";
-            pr "  PyDict_SetItemString (dict, \"%s\",\n"
name;
-            pr "                        PyUnicode_FromStringAndSize
(&%s->%s, 1));\n"
+            pr "  value = PyUnicode_FromStringAndSize (&%s->%s,
1);\n"
               typ name;
-            pr "#endif\n"
+            pr "#endif\n";
+            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";
@@ -508,16 +529,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
-       | RHashtable _ ->
+           pr "  guestfs_free_%s_list (r);\n" typ;
+           pr "  if (py_r == NULL) goto out;\n";
+       | RHashtable n ->
            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 "#ifdef HAVE_PYSTRING_ASSTRING\n";
            pr "  py_r = PyString_FromStringAndSize (r, size);\n";
--
2.11.0

Signed-off-by: Matteo Cafasso <noxdafox@gmail.com>
---
 python/handle.c | 48 ++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 10 deletions(-)

diff --git a/python/handle.c b/python/handle.c
index 806408f91..1cfb3eb61 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -337,19 +337,27 @@ 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) {
 #ifdef HAVE_PYSTRING_ASSTRING
-    PyList_SetItem (list, i, PyString_FromString (argv[i]));
+    item = PyString_FromString (argv[i]);
 #else
-    PyList_SetItem (list, i, PyUnicode_FromString (argv[i]));
+    item = PyUnicode_FromString (argv[i]);
 #endif
+    if (item == NULL) {
+      Py_CLEAR (list);
+      return NULL;
+    }
+
+    PyList_SetItem (list, i, item);
   }

   return list;
@@ -358,24 +366,44 @@ 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);
+    tuple = PyTuple_New (2);
+    if (tuple == NULL)
+      goto err;
 #ifdef HAVE_PYSTRING_ASSTRING
-    PyTuple_SetItem (item, 0, PyString_FromString (argv[i]));
-    PyTuple_SetItem (item, 1, PyString_FromString (argv[i+1]));
+    item = PyString_FromString (argv[i]);
+    if (item == NULL)
+      goto err;
+    PyTuple_SetItem (tuple, 0, item);
+    item = PyString_FromString (argv[i+1]);
+    if (item == NULL)
+      goto err;
+    PyTuple_SetItem (tuple, 1, item);
 #else
-    PyTuple_SetItem (item, 0, PyUnicode_FromString (argv[i]));
-    PyTuple_SetItem (item, 1, PyUnicode_FromString (argv[i+1]));
+    item = PyUnicode_FromString (argv[i]);
+    if (item == NULL)
+      goto err;
+    PyTuple_SetItem (tuple, 0, item);
+    item = PyUnicode_FromString (argv[i+1]);
+    if (item == NULL)
+      goto err;
+    PyTuple_SetItem (tuple, 1, item);
 #endif
-    PyList_SetItem (list, i >> 1, item);
+    PyList_SetItem (list, i >> 1, tuple);
   }

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

Signed-off-by: Matteo Cafasso <noxdafox@gmail.com>
---
 python/t/test830RHBZ1406906.py | 55 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 python/t/test830RHBZ1406906.py

diff --git a/python/t/test830RHBZ1406906.py b/python/t/test830RHBZ1406906.py
new file mode 100644
index 000000000..62caf8ce1
--- /dev/null
+++ b/python/t/test830RHBZ1406906.py
@@ -0,0 +1,55 @@
+# 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 shutil
+import tempfile
+import unittest
+
+import guestfs
+
+
+class Test830RHBZ1406906(unittest.TestCase):
+    def setUp(self):
+        self.filename = "rhbz1406906.img"
+        self.tempdir = tempfile.mkdtemp()
+        self.guestfs = guestfs.GuestFS(python_return_dict=True)
+
+        self.guestfs.disk_create(self.filename, "raw", 512 * 1024 *
1024)
+        self.guestfs.add_drive(self.filename)
+        self.guestfs.launch()
+
+    def tearDown(self):
+        self.guestfs.close()
+        os.unlink(self.filename)
+        shutil.rmtree(self.tempdir)
+
+    def test_rhbz1406906(self):
+        self.guestfs.part_disk("/dev/sda", "mbr")
+        self.guestfs.mkfs("ext3", "/dev/sda1",
blocksize=1024)
+        self.guestfs.mount("/dev/sda1", "/")
+
+        # touch file with illegal unicode character
+        open(os.path.join(self.tempdir, "\udcd4"),
"w").close()
+
+        self.guestfs.copy_in(self.tempdir, "/")
+
+        # segfault here on Python 3
+        try:
+            self.guestfs.find("/")
+        except UnicodeDecodeError:
+            pass
--
2.11.0

On Saturday, 6 May 2017 11:32:29 CEST Matteo Cafasso
wrote:
> Signed-off-by: Matteo Cafasso <noxdafox@gmail.com>
> ---
>  python/t/test830RHBZ1406906.py | 55
++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 python/t/test830RHBZ1406906.py
> 
> diff --git a/python/t/test830RHBZ1406906.py
b/python/t/test830RHBZ1406906.py
> new file mode 100644
> index 000000000..62caf8ce1
> --- /dev/null
> +++ b/python/t/test830RHBZ1406906.py
> @@ -0,0 +1,55 @@
> +# 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 shutil
> +import tempfile
> +import unittest
> +
> +import guestfs
> +
> +
> +class Test830RHBZ1406906(unittest.TestCase):
> +    def setUp(self):
> +        self.filename = "rhbz1406906.img"
> +        self.tempdir = tempfile.mkdtemp()
> +        self.guestfs = guestfs.GuestFS(python_return_dict=True)
Usually we call the handle 'g'. Also, it would be better IMHO to
create the handle locally in the test_rhbz1406906 function itself.
> +        self.guestfs.disk_create(self.filename, "raw", 512 *
1024 * 1024)
I'd just use a scratch disk, so the disk is automatically removed.
> +        self.guestfs.add_drive(self.filename)
> +        self.guestfs.launch()
> +
> +    def tearDown(self):
> +        self.guestfs.close()
> +        os.unlink(self.filename)
> +        shutil.rmtree(self.tempdir)
> +
> +    def test_rhbz1406906(self):
> +        self.guestfs.part_disk("/dev/sda", "mbr")
> +        self.guestfs.mkfs("ext3", "/dev/sda1",
blocksize=1024)
A simple ext4 with no options should be ok too. (This is not an issue,
just keeping the test simple with no extra options.)
> +        self.guestfs.mount("/dev/sda1", "/")
> +
> +        # touch file with illegal unicode character
> +        open(os.path.join(self.tempdir, "\udcd4"),
"w").close()
> +
> +        self.guestfs.copy_in(self.tempdir, "/")
> +
> +        # segfault here on Python 3
> +        try:
> +            self.guestfs.find("/")
> +        except UnicodeDecodeError:
> +            pass
If you expect an exception to happen, and want to check for that,
please use assertRaises, e.g.:

  self.assertRaises(UnicodeDecodeError, g.find, "/")

Does it crash also with Python 2?

-- 
Pino Toscano

On Saturday, 6 May 2017 11:32:26 CEST Matteo Cafasso
wrote:
> This series addresses the issue where non UTF8 file names in a guest
> image lead to libguestfs segfault with Python 3 APIs.
All the patches are basically parts of the same fix, so I'd squash them
together in a single patch that changes the generator, the manual
parts, and add a test to prove the fix works.
> The core issue is the APIs are not checking the return value when
> constructing a new PyObject. Therefore NULL pointers are added to
> Python collections (lists and dictionaries) crashing the application.
All these #ifdef HAVE_PYSTRING_ASSTRING blocks make both the OCaml, and
the generated C code hard to read -- I've just send a cleanup patch for
this: https://www.redhat.com/archives/libguestfs/2017-May/msg00051.html

Aside this, and the notes left for patch #3, the changes would look
correct to me.

Thanks,
-- 
Pino Toscano