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

[Pino Toscano]

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20170511/bddef4a8/attachment.sig>