From b111d50347076336b3e655178d967f8e5c8c9913 Mon Sep 17 00:00:00 2001 From: Prashanth Pai Date: Mon, 30 May 2016 15:08:48 +0530 Subject: Add validation decorators As glfs and glfd are pointers to memory locations, passing invalid values of glfs and glfd to the libgfapi C library can result in segfault. This patch introduces decorators that validate glfs and glfd before calling correspoding C APIs. Change-Id: I4e86bd8e436e23cd41f75f428d246939c820bb9c Signed-off-by: Prashanth Pai --- gluster/exceptions.py | 4 ++ gluster/gfapi.py | 78 +++++++++++++++++++++++++------- gluster/utils.py | 62 +++++++++++++++++++++++++ test/functional/libgfapi-python-tests.py | 23 ++++++++++ test/unit/gluster/test_gfapi.py | 31 +++++++++++++ test/unit/gluster/test_utils.py | 70 ++++++++++++++++++++++++++++ 6 files changed, 252 insertions(+), 16 deletions(-) create mode 100644 gluster/utils.py create mode 100644 test/unit/gluster/test_utils.py diff --git a/gluster/exceptions.py b/gluster/exceptions.py index 2bb4732..962e69f 100644 --- a/gluster/exceptions.py +++ b/gluster/exceptions.py @@ -16,3 +16,7 @@ class LibgfapiException(Exception): pass + + +class VolumeNotMounted(LibgfapiException): + pass diff --git a/gluster/gfapi.py b/gluster/gfapi.py index 6fed798..121d442 100755 --- a/gluster/gfapi.py +++ b/gluster/gfapi.py @@ -17,6 +17,7 @@ import stat import errno from gluster import api from gluster.exceptions import LibgfapiException +from gluster.utils import validate_mount, validate_glfd # TODO: Move this utils.py python_mode_to_os_flags = {} @@ -53,18 +54,24 @@ class File(object): self.fd = fd self.originalpath = path self._mode = mode - self._closed = False + self._validate_init() def __enter__(self): - if self.fd is None: - # __enter__ should only be called within the context - # of a 'with' statement when opening a file through - # Volume.open() - raise ValueError("I/O operation on closed file") + # __enter__ should only be called within the context + # of a 'with' statement when opening a file through + # Volume.fopen() + self._validate_init() return self def __exit__(self, type, value, tb): - self.close() + if self.fd: + self.close() + + def _validate_init(self): + if self.fd is None: + raise ValueError("I/O operation on invalid fd") + elif not isinstance(self.fd, int): + raise ValueError("I/O operation on invalid fd") @property def fileno(self): @@ -72,7 +79,6 @@ class File(object): Return the internal file descriptor (glfd) that is used by the underlying implementation to request I/O operations. """ - # TODO: Make self.fd private (self._fd) return self.fd @property @@ -98,22 +104,22 @@ class File(object): Bool indicating the current state of the file object. This is a read-only attribute; the close() method changes the value. """ - return self._closed + return not self.fd + @validate_glfd def close(self): """ Close the file. A closed file cannot be read or written any more. - Calling close() more than once is allowed. :raises: OSError on failure """ - if not self._closed: - ret = api.glfs_close(self.fd) - if ret < 0: - err = ctypes.get_errno() - raise OSError(err, os.strerror(err)) - self._closed = True + ret = api.glfs_close(self.fd) + if ret < 0: + err = ctypes.get_errno() + raise OSError(err, os.strerror(err)) + self.fd = None + @validate_glfd def discard(self, offset, length): """ This is similar to UNMAP command that is used to return the @@ -133,6 +139,7 @@ class File(object): err = ctypes.get_errno() raise OSError(err, os.strerror(err)) + @validate_glfd def dup(self): """ Return a duplicate of File object. This duplicate File class instance @@ -146,6 +153,7 @@ class File(object): raise OSError(err, os.strerror(err)) return File(dupfd, self.originalpath) + @validate_glfd def fallocate(self, mode, offset, length): """ This is a Linux-specific sys call, unlike posix_fallocate() @@ -164,6 +172,7 @@ class File(object): err = ctypes.get_errno() raise OSError(err, os.strerror(err)) + @validate_glfd def fchmod(self, mode): """ Change this file's mode @@ -176,6 +185,7 @@ class File(object): err = ctypes.get_errno() raise OSError(err, os.strerror(err)) + @validate_glfd def fchown(self, uid, gid): """ Change this file's owner and group id @@ -189,6 +199,7 @@ class File(object): err = ctypes.get_errno() raise OSError(err, os.strerror(err)) + @validate_glfd def fdatasync(self): """ Flush buffer cache pages pertaining to data, but not the ones @@ -201,6 +212,7 @@ class File(object): err = ctypes.get_errno() raise OSError(err, os.strerror(err)) + @validate_glfd def fgetsize(self): """ Return the size of a file, as reported by fstat() @@ -209,6 +221,7 @@ class File(object): """ return self.fstat().st_size + @validate_glfd def fgetxattr(self, key, size=0): """ Retrieve the value of the extended attribute identified by key @@ -235,6 +248,7 @@ class File(object): raise OSError(err, os.strerror(err)) return buf.value[:rc] + @validate_glfd def flistxattr(self, size=0): """ Retrieve list of extended attributes for the file. @@ -275,6 +289,7 @@ class File(object): xattrs.sort() return xattrs + @validate_glfd def fsetxattr(self, key, value, flags=0): """ Set extended attribute of file. @@ -295,6 +310,7 @@ class File(object): err = ctypes.get_errno() raise OSError(err, os.strerror(err)) + @validate_glfd def fremovexattr(self, key): """ Remove a extended attribute of the file. @@ -307,6 +323,7 @@ class File(object): err = ctypes.get_errno() raise OSError(err, os.strerror(err)) + @validate_glfd def fstat(self): """ Returns Stat object for this file. @@ -321,6 +338,7 @@ class File(object): raise OSError(err, os.strerror(err)) return s + @validate_glfd def fsync(self): """ Flush buffer cache pages pertaining to data and metadata. @@ -332,6 +350,7 @@ class File(object): err = ctypes.get_errno() raise OSError(err, os.strerror(err)) + @validate_glfd def ftruncate(self, length): """ Truncated the file to a size of length bytes. @@ -348,6 +367,7 @@ class File(object): err = ctypes.get_errno() raise OSError(err, os.strerror(err)) + @validate_glfd def lseek(self, pos, how): """ Set the read/write offset position of this file. @@ -367,6 +387,7 @@ class File(object): raise OSError(err, os.strerror(err)) return ret + @validate_glfd def read(self, size=-1): """ Read at most size bytes from the file. @@ -389,6 +410,7 @@ class File(object): err = ctypes.get_errno() raise OSError(err, os.strerror(err)) + @validate_glfd def readinto(self, buf): """ Read up to len(buf) bytes into buf which must be a bytearray. @@ -413,6 +435,7 @@ class File(object): raise OSError(err, os.strerror(err)) return nread + @validate_glfd def write(self, data, flags=0): """ Write data to the file. @@ -434,6 +457,7 @@ class File(object): raise OSError(err, os.strerror(err)) return ret + @validate_glfd def zerofill(self, offset, length): """ Fill 'length' number of bytes with zeroes starting from 'offset'. @@ -619,6 +643,7 @@ class Volume(object): self.log_file = log_file self.log_level = log_level + @validate_mount def access(self, path, mode): """ Use the real uid/gid to test for access to path. @@ -635,6 +660,7 @@ class Volume(object): else: return False + @validate_mount def chdir(self, path): """ Change the current working directory to the given path. @@ -647,6 +673,7 @@ class Volume(object): err = ctypes.get_errno() raise OSError(err, os.strerror(err)) + @validate_mount def chmod(self, path, mode): """ Change mode of path @@ -660,6 +687,7 @@ class Volume(object): err = ctypes.get_errno() raise OSError(err, os.strerror(err)) + @validate_mount def chown(self, path, uid, gid): """ Change owner and group id of path @@ -701,6 +729,7 @@ class Volume(object): """ return self.stat(path).st_ctime + @validate_mount def getcwd(self): """ Returns current working directory. @@ -726,6 +755,7 @@ class Volume(object): """ return self.stat(path).st_size + @validate_mount def getxattr(self, path, key, size=0): """ Retrieve the value of the extended attribute identified by key @@ -805,6 +835,7 @@ class Volume(object): dir_list.append(name) return dir_list + @validate_mount def listxattr(self, path, size=0): """ Retrieve list of extended attribute keys for the specified path. @@ -846,6 +877,7 @@ class Volume(object): xattrs.sort() return xattrs + @validate_mount def lstat(self, path): """ Return stat information of path. If path is a symbolic link, then it @@ -884,6 +916,7 @@ class Volume(object): return self.mkdir(path, mode) + @validate_mount def mkdir(self, path, mode=0777): """ Create a directory named path with numeric mode mode. @@ -896,6 +929,7 @@ class Volume(object): err = ctypes.get_errno() raise OSError(err, os.strerror(err)) + @validate_mount def fopen(self, path, mode='r'): """ Similar to Python's built-in File object returned by Python's open() @@ -936,6 +970,7 @@ class Volume(object): raise OSError(err, os.strerror(err)) return File(fd, path=path, mode=mode) + @validate_mount def open(self, path, flags, mode=0777): """ Similar to Python's os.open() @@ -965,6 +1000,7 @@ class Volume(object): return fd + @validate_mount def opendir(self, path): """ Open a directory. @@ -979,6 +1015,7 @@ class Volume(object): raise OSError(err, os.strerror(err)) return Dir(fd) + @validate_mount def readlink(self, path): """ Return a string representing the path to which the symbolic link @@ -1005,6 +1042,7 @@ class Volume(object): """ return self.unlink(path) + @validate_mount def removexattr(self, path, key): """ Remove a extended attribute of the path. @@ -1018,6 +1056,7 @@ class Volume(object): err = ctypes.get_errno() raise OSError(err, os.strerror(err)) + @validate_mount def rename(self, src, dst): """ Rename the file or directory from src to dst. If dst is a directory, @@ -1031,6 +1070,7 @@ class Volume(object): err = ctypes.get_errno() raise OSError(err, os.strerror(err)) + @validate_mount def rmdir(self, path): """ Remove (delete) the directory path. Only works when the directory is @@ -1112,6 +1152,7 @@ class Volume(object): err = ctypes.get_errno() raise OSError(err, os.strerror(err)) + @validate_mount def setxattr(self, path, key, value, flags=0): """ Set extended attribute of the path. @@ -1134,6 +1175,7 @@ class Volume(object): err = ctypes.get_errno() raise OSError(err, os.strerror(err)) + @validate_mount def stat(self, path): """ Returns stat information of path. @@ -1147,6 +1189,7 @@ class Volume(object): raise OSError(err, os.strerror(err)) return s + @validate_mount def statvfs(self, path): """ Returns information about a mounted glusterfs volume. path is the @@ -1167,6 +1210,7 @@ class Volume(object): raise OSError(err, os.strerror(err)) return s + @validate_mount def symlink(self, source, link_name): """ Create a symbolic link 'link_name' which points to 'source' @@ -1178,6 +1222,7 @@ class Volume(object): err = ctypes.get_errno() raise OSError(err, os.strerror(err)) + @validate_mount def unlink(self, path): """ Delete the file 'path' @@ -1189,6 +1234,7 @@ class Volume(object): err = ctypes.get_errno() raise OSError(err, os.strerror(err)) + @validate_mount def utime(self, path, times): """ Set the access and modified times of the file specified by path. If diff --git a/gluster/utils.py b/gluster/utils.py new file mode 100644 index 0000000..833302e --- /dev/null +++ b/gluster/utils.py @@ -0,0 +1,62 @@ +# Copyright (c) 2016 Red Hat, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +# implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import os +import errno +from functools import wraps +from gluster.exceptions import VolumeNotMounted + + +def validate_mount(func): + """ + Decorator to assert that volume is initialized and mounted before any + further I/O calls are invoked by methods. + + :param func: method to be decorated and checked. + """ + def _exception(volname): + raise VolumeNotMounted('Volume "%s" not mounted.' % (volname)) + + @wraps(func) + def wrapper(*args, **kwargs): + self = args[0] + if self.fs and self._mounted: + return func(*args, **kwargs) + else: + return _exception(self.volname) + wrapper.__wrapped__ = func + + return wrapper + + +def validate_glfd(func): + """ + Decorator to assert that glfd is valid. + + :param func: method to be decorated and checked. + """ + def _exception(): + raise OSError(errno.EBADF, os.strerror(errno.EBADF)) + + @wraps(func) + def wrapper(*args, **kwargs): + self = args[0] + if self.fd: + return func(*args, **kwargs) + else: + return _exception() + wrapper.__wrapped__ = func + + return wrapper diff --git a/test/functional/libgfapi-python-tests.py b/test/functional/libgfapi-python-tests.py index c29f400..4e339e0 100644 --- a/test/functional/libgfapi-python-tests.py +++ b/test/functional/libgfapi-python-tests.py @@ -127,6 +127,29 @@ class FileOpsTest(unittest.TestCase): self.assertRaises(OSError, self.vol.open, "file", 12345) + def test_double_close(self): + name = uuid4().hex + f = self.vol.fopen(name, 'w') + f.close() + for i in range(2): + try: + f.close() + except OSError as err: + self.assertEqual(err.errno, errno.EBADF) + else: + self.fail("Expecting OSError") + + def test_glfd_decorators_IO_on_invalid_glfd(self): + name = uuid4().hex + with self.vol.fopen(name, 'w') as f: + f.write("Valar Morghulis") + try: + s = f.read() + except OSError as err: + self.assertEqual(err.errno, errno.EBADF) + else: + self.fail("Expecting OSError") + def test_fopen_err(self): # mode not string self.assertRaises(TypeError, self.vol.fopen, "file", os.O_WRONLY) diff --git a/test/unit/gluster/test_gfapi.py b/test/unit/gluster/test_gfapi.py index 3934a6f..86fa621 100644 --- a/test/unit/gluster/test_gfapi.py +++ b/test/unit/gluster/test_gfapi.py @@ -11,6 +11,7 @@ import unittest import gluster +import inspect import os import stat import time @@ -70,6 +71,36 @@ class TestFile(unittest.TestCase): def tearDown(self): gluster.gfapi.api.glfs_close = self._saved_glfs_close + def test_validate_init(self): + self.assertRaises(ValueError, File, None) + self.assertRaises(ValueError, File, "not_int") + + try: + with File(None) as f: + pass + except ValueError: + pass + else: + self.fail("Expecting ValueError") + + try: + with File("not_int") as f: + pass + except ValueError: + pass + else: + self.fail("Expecting ValueError") + + def test_validate_glfd_decorator_applied(self): + for method_name, method_instance in \ + inspect.getmembers(File, predicate=inspect.ismethod): + if not method_name.startswith('_'): + try: + wrapper_attribute = method_instance.__wrapped__.__name__ + self.assertEqual(wrapper_attribute, method_name) + except AttributeError: + self.fail("Method File.%s isn't decorated" % (method_name)) + def test_fchmod_success(self): mock_glfs_fchmod = Mock() mock_glfs_fchmod.return_value = 0 diff --git a/test/unit/gluster/test_utils.py b/test/unit/gluster/test_utils.py new file mode 100644 index 0000000..eb7a15f --- /dev/null +++ b/test/unit/gluster/test_utils.py @@ -0,0 +1,70 @@ +# Copyright (c) 2016 Red Hat, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +# implied. +# See the License for the specific language governing permissions and +# limitations under the License. +import unittest + +from gluster import utils +from gluster.exceptions import VolumeNotMounted + + +class TestUtils(unittest.TestCase): + + def test_validate_mount(self): + + class _FakeVol(object): + + def __init__(self): + self.fs = None + self._mounted = None + self.volname = "vol1" + + @utils.validate_mount + def test_method(self): + return + + v = _FakeVol() + try: + v.test_method() + except VolumeNotMounted as err: + self.assertEqual(str(err), 'Volume "vol1" not mounted.') + else: + self.fail("Expected VolumeNotMounted exception.") + + v.fs = 12345 + v._mounted = True + # Shouldn't raise exception. + v.test_method() + + def test_validate_glfd(self): + + class _FakeFile(object): + + def __init__(self, fd, path=None): + self.fd = fd + + @utils.validate_glfd + def test_method(self): + return + + def close(self): + self.fd = None + + f = _FakeFile(1234) + f.close() + self.assertTrue(f.fd is None) + self.assertRaises(OSError, f.test_method) + + f.fd = 1234 + # Shouldn't raise exception. + f.test_method() -- cgit