From 25188ca49950267a74b35aab1359bd5d3b919fc7 Mon Sep 17 00:00:00 2001 From: Prashanth Pai Date: Thu, 3 Mar 2016 17:25:48 +0530 Subject: Fix dup fd leak A fd was not being closed after it was duplicated. This code path can be easily hit when doing a GET on a file that needs Etag (md5sum) to be recalculated. Change-Id: Ib2e10d990b9b2e1fa85d0079767892de8c8d4eec Signed-off-by: Prashanth Pai Reviewed-on: http://review.gluster.org/13593 Reviewed-by: Thiago da Silva Tested-by: Thiago da Silva --- gluster/swift/common/utils.py | 15 +++++++++++---- test/functional_auth/common_conf/object-server.conf | 2 -- test/unit/common/test_utils.py | 16 ++++++++++++++++ 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/gluster/swift/common/utils.py b/gluster/swift/common/utils.py index 3bcd074..ded2f0b 100644 --- a/gluster/swift/common/utils.py +++ b/gluster/swift/common/utils.py @@ -415,19 +415,26 @@ def _get_etag(path_or_fd): Since we don't have that we should yield after each chunk read and computed so that we don't consume the worker thread. """ + etag = '' if isinstance(path_or_fd, int): # We are given a file descriptor, so this is an invocation from the # DiskFile.open() method. fd = path_or_fd - etag = _read_for_etag(do_dup(fd)) - do_lseek(fd, 0, os.SEEK_SET) + dup_fd = do_dup(fd) + try: + etag = _read_for_etag(dup_fd) + do_lseek(fd, 0, os.SEEK_SET) + finally: + do_close(dup_fd) else: # We are given a path to the object when the DiskDir.list_objects_iter # method invokes us. path = path_or_fd fd = do_open(path, os.O_RDONLY) - etag = _read_for_etag(fd) - do_close(fd) + try: + etag = _read_for_etag(fd) + finally: + do_close(fd) return etag diff --git a/test/functional_auth/common_conf/object-server.conf b/test/functional_auth/common_conf/object-server.conf index 28076cd..2599c87 100644 --- a/test/functional_auth/common_conf/object-server.conf +++ b/test/functional_auth/common_conf/object-server.conf @@ -51,5 +51,3 @@ network_chunk_size = 65556 # across all conf files! auto_create_account_prefix = gs expiring_objects_account_name = expiring - -gluster_swift_mode = true diff --git a/test/unit/common/test_utils.py b/test/unit/common/test_utils.py index 5ab6f36..f88229a 100644 --- a/test/unit/common/test_utils.py +++ b/test/unit/common/test_utils.py @@ -439,6 +439,22 @@ class TestUtils(unittest.TestCase): md5.update(chunk) assert hd == md5.hexdigest() + def test_get_etag_dup_fd_closed(self): + fd, path = tempfile.mkstemp() + data = "It's not who we are underneath, but what we do that defines us" + os.write(fd, data) + os.lseek(fd, 0, os.SEEK_SET) + + mock_do_close = Mock() + with patch("gluster.swift.common.utils.do_close", mock_do_close): + etag = utils._get_etag(fd) + self.assertEqual(etag, hashlib.md5(data).hexdigest()) + self.assertTrue(mock_do_close.called) + + # We mocked out close, so we have to close the fd for real + os.close(mock_do_close.call_args[0][0]) + os.close(fd) + def test_get_object_metadata_dne(self): md = utils.get_object_metadata("/tmp/doesNotEx1st") assert md == {} -- cgit