4:32 a.m.
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
4:32 a.m.
New subject: [Bug 1406906] [PATCH 1/3] generator: 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.
Signed-off-by: Matteo Cafasso <noxdafox(a)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
4:32 a.m.
New subject: [Bug 1406906] [PATCH 2/3] python: check return value of object functions in handle.c
Signed-off-by: Matteo Cafasso <noxdafox(a)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
4:32 a.m.
New subject: [Bug 1406906] [PATCH 3/3] python: add regression test for RHBZ#1406906
Signed-off-by: Matteo Cafasso <noxdafox(a)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
7:43 a.m.
New subject: [Bug 1406906] [PATCH 3/3] python: add regression test for RHBZ#1406906
On Saturday, 6 May 2017 11:32:29 CEST Matteo Cafasso wrote:
Signed-off-by: Matteo Cafasso <noxdafox(a)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
8:24 a.m.
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
2718
days inactive
2721
days old
5 comments
2 participants
participants (2)
-
Matteo Cafasso -
Pino Toscano