From 2f077ceaa9be680360d5ad33bd2ec8f89a9035ab Mon Sep 17 00:00:00 2001 From: Alex Pyrgiotis Date: Jun 14 2019 05:48:19 +0000 Subject: [PATCH 1/4] Sanitize file descriptors Add a helper function that removes invalid file descriptors from a set. Invalid file descriptors can be negative numbers, or numbers larger than the highest possible file descriptor for a process. Signed-off-by: Alex Pyrgiotis --- diff --git a/daemon/daemon.py b/daemon/daemon.py index f8e0af3..7d9dff2 100644 --- a/daemon/daemon.py +++ b/daemon/daemon.py @@ -890,6 +890,22 @@ def _get_candidate_file_descriptors(exclude): return candidates +def _sanitize_file_descriptors(fds, maxfd): + """ Remove invalid file descriptors from a set. + + :param fds: A collection (set) of file descriptors. + :param maxfd: The maximum number of open file descriptors in the + process. + :return: A subset of the input, where invalid file descriptors are + removed. + + Remove invalid file descriptors from a set. Invalid file descriptors + are negative numbers, or numbers larger than the highest possible file + descriptor number for the process. + """ + return {fd for fd in fds if 0 <= fd < maxfd} + + FileDescriptorRange = collections.namedtuple( 'FileDescriptorRange', ['low', 'high']) diff --git a/test/test_daemon.py b/test/test_daemon.py index 739c3c1..1a87ea9 100644 --- a/test/test_daemon.py +++ b/test/test_daemon.py @@ -1477,6 +1477,40 @@ class _get_candidate_file_descriptors_TestCase(scaffold.TestCaseWithScenarios): self.assertEqual(result, self.expected_result) +class _sanitize_file_descriptors_TestCase(scaffold.TestCaseWithScenarios): + """ Test cases for function `_sanitize_file_descriptors`. """ + + scenarios = [ + ('exclude-one', { + 'test_kwargs': { + 'fds': {4}, + 'maxfd': 5, + }, + 'expected_result': {4}, + }), + ('exclude-none', { + 'test_kwargs': { + 'fds': set(), + 'maxfd': 5, + }, + 'expected_result': set(), + }), + ('exclude-out-of-bounds', { + 'test_kwargs': { + 'fds': {-1, 6}, + 'maxfd': 5, + }, + 'expected_result': set(), + }), + ] + + def test_returns_expected_file_descriptors(self): + """ Should return the expected set of file descriptors. """ + result = daemon.daemon._sanitize_file_descriptors( + **self.test_kwargs) + self.assertEqual(result, self.expected_result) + + class _get_candidate_file_descriptor_ranges_TestCase( scaffold.TestCaseWithScenarios): """ Test cases for function `_get_candidate_file_descriptor_ranges`. """ From 499046e1edbca06e2b750922d80ddcd82426601f Mon Sep 17 00:00:00 2001 From: Alex Pyrgiotis Date: Jun 14 2019 05:48:30 +0000 Subject: [PATCH 2/4] Make creation of file descriptor ranges faster During daemonization, we need to close all open file descriptors, except for the ones that correspond to the standard streams and the ones that the user has explicitly specified that they must be preserved. Since these file descriptors are passed to `os.closerange()', we need to construct a list of file descriptor ranges. The previous way of creating these fd ranges operated on a list of fds up to the theoretical maximum, which it then sorted and iterated. In many environments, the highest possible fd is 1048576, which can make this method costly and take a lot of time. This commit speeds up the process of creating fd ranges by iterating the list of *excluded* file descriptors instead, which is usually very small. Signed-off-by: Alex Pyrgiotis --- diff --git a/daemon/daemon.py b/daemon/daemon.py index 7d9dff2..380525c 100644 --- a/daemon/daemon.py +++ b/daemon/daemon.py @@ -926,30 +926,24 @@ def _get_candidate_file_descriptor_ranges(exclude): in this process, excluding those integers in the `exclude` collection. """ - candidates_list = sorted(_get_candidate_file_descriptors(exclude)) - ranges = [] + start = 0 + maxfd = get_maximum_file_descriptors() + exclude = _sanitize_file_descriptors(exclude, maxfd) - def append_range_if_needed(candidate_range): - if (candidate_range.low < candidate_range.high): - # The range is not empty. - ranges.append(candidate_range) - - this_range = ( - FileDescriptorRange( - low=min(candidates_list), - high=(min(candidates_list) + 1)) - if candidates_list else FileDescriptorRange(low=0, high=0)) - for fd in candidates_list[1:]: - high = fd + 1 - if this_range.high == fd: - # This file descriptor extends the current range. - this_range = this_range._replace(high=high) - else: - # The previous range has ended at a gap. - append_range_if_needed(this_range) - # This file descriptor begins a new range. - this_range = FileDescriptorRange(low=fd, high=high) - append_range_if_needed(this_range) + ranges = [] + for fd in sorted(exclude): + # Create a file descriptor range from a starting point to an + # excluded file descriptor. The starting point is either 0 or the end + # of the previous range. Note that consecutive excluded file + # descriptors are skipped. + if fd > start: + ranges.append(FileDescriptorRange(low=start, high=fd)) + start = fd + 1 + + # If we didn't cover all possible file descriptors until the highest one, + # add an extra range from the last starting point. + if start < maxfd: + ranges.append(FileDescriptorRange(low=start, high=maxfd)) return ranges From 76cc6e0b96161ea4d7dac32c29ee0fa227fefc1a Mon Sep 17 00:00:00 2001 From: Alex Pyrgiotis Date: Jun 14 2019 05:49:21 +0000 Subject: [PATCH 3/4] Add more test scenarios for fd ranges Add some extra test scenarios for fd ranges, that cover some corner cases. Signed-off-by: Alex Pyrgiotis --- diff --git a/test/test_daemon.py b/test/test_daemon.py index 1a87ea9..25af45e 100644 --- a/test/test_daemon.py +++ b/test/test_daemon.py @@ -1570,6 +1570,44 @@ class _get_candidate_file_descriptor_ranges_TestCase( (0, 4), ], }), + ('exclude-zero', { + 'fake_maxfd': 5, + 'test_kwargs': { + 'exclude': {0}, + }, + 'expected_result': [(1, 5)], + }), + ('exclude-consecutive', { + 'fake_maxfd': 5, + 'test_kwargs': { + 'exclude': {1, 2}, + }, + 'expected_result': [ + (0, 1), + (3, 5), + ], + }), + ('exclude-all', { + 'fake_maxfd': 5, + 'test_kwargs': { + 'exclude': {0, 1, 2, 3, 4}, + }, + 'expected_result': [] + }), + ('exclude-maxfd', { + 'fake_maxfd': 5, + 'test_kwargs': { + 'exclude': {5}, + }, + 'expected_result': [(0, 5)] + }), + ('exclude-out-of-bounds', { + 'fake_maxfd': 5, + 'test_kwargs': { + 'exclude': {-9, -3, 6, 10}, + }, + 'expected_result': [(0, 5)] + }), ] def test_returns_expected_file_descriptors(self): From d401ebf681c61426db687ea221c9975b24f0323c Mon Sep 17 00:00:00 2001 From: Alex Pyrgiotis Date: Jun 14 2019 05:49:38 +0000 Subject: [PATCH 4/4] Remove a redundant helper and its tests Remove the `_get_candidate_file_descriptors' helper along with its tests, since it's not used in the code anymore. Signed-off-by: Alex Pyrgiotis --- diff --git a/daemon/daemon.py b/daemon/daemon.py index 380525c..a069591 100644 --- a/daemon/daemon.py +++ b/daemon/daemon.py @@ -869,27 +869,6 @@ def get_maximum_file_descriptors(): return result -def _get_candidate_file_descriptors(exclude): - """ Get the collection of candidate file descriptors. - - :param exclude: A collection of file descriptors that should - be excluded from the return set. - :return: The collection (a `set`) of file descriptors that are - candidates for files that may be open in this process. - - Determine the set of all `int` values that could be open file - descriptors in this process. A file descriptor is a candidate - if it is within the range (0, `maxfd`), excluding those - integers in the `exclude` collection. - - The `maxfd` value is determined from the standard library - `resource` module. - """ - maxfd = get_maximum_file_descriptors() - candidates = set(range(0, maxfd)).difference(exclude) - return candidates - - def _sanitize_file_descriptors(fds, maxfd): """ Remove invalid file descriptors from a set. diff --git a/test/test_daemon.py b/test/test_daemon.py index 25af45e..37f7249 100644 --- a/test/test_daemon.py +++ b/test/test_daemon.py @@ -1440,43 +1440,6 @@ def fake_get_maximum_file_descriptors(): return fake_default_maxfd -class _get_candidate_file_descriptors_TestCase(scaffold.TestCaseWithScenarios): - """ Test cases for function `_get_candidate_file_descriptors`. """ - - scenarios = [ - ('exclude-three', { - 'fake_maxfd': 10, - 'test_kwargs': { - 'exclude': {3, 5, 8}, - }, - 'expected_result': {0, 1, 2, 4, 6, 7, 9}, - }), - ('exclude-one', { - 'fake_maxfd': 5, - 'test_kwargs': { - 'exclude': {4}, - }, - 'expected_result': {0, 1, 2, 3}, - }), - ('exclude-none', { - 'fake_maxfd': 5, - 'test_kwargs': { - 'exclude': set(), - }, - 'expected_result': {0, 1, 2, 3, 4}, - }), - ] - - def test_returns_expected_file_descriptors(self): - """ Should return the expected set of file descriptors. """ - with mock.patch.object( - daemon.daemon, "get_maximum_file_descriptors", - return_value=self.fake_maxfd): - result = daemon.daemon._get_candidate_file_descriptors( - **self.test_kwargs) - self.assertEqual(result, self.expected_result) - - class _sanitize_file_descriptors_TestCase(scaffold.TestCaseWithScenarios): """ Test cases for function `_sanitize_file_descriptors`. """