From 85b298e49cad821d0fccce92670e618088f0aa4b Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Sat, 12 Sep 2015 12:56:28 -0400 Subject: [PATCH 1/3] RF: try/except/finally in a single statement (while at it) since we support now python >= 2.6 --- fail2ban/server/action.py | 42 +++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/fail2ban/server/action.py b/fail2ban/server/action.py index c58fde2c..a8e61fb4 100644 --- a/fail2ban/server/action.py +++ b/fail2ban/server/action.py @@ -560,32 +560,32 @@ class CommandAction(ActionBase): return True _cmd_lock.acquire() - try: # Try wrapped within another try needed for python version < 2.5 + try: stdout = tempfile.TemporaryFile(suffix=".stdout", prefix="fai2ban_") stderr = tempfile.TemporaryFile(suffix=".stderr", prefix="fai2ban_") - try: - popen = subprocess.Popen( - realCmd, stdout=stdout, stderr=stderr, shell=True, - preexec_fn=os.setsid # so that killpg does not kill our process - ) - stime = time.time() + + popen = subprocess.Popen( + realCmd, stdout=stdout, stderr=stderr, shell=True, + preexec_fn=os.setsid # so that killpg does not kill our process + ) + stime = time.time() + retcode = popen.poll() + while time.time() - stime <= timeout and retcode is None: + time.sleep(0.1) retcode = popen.poll() - while time.time() - stime <= timeout and retcode is None: + if retcode is None: + logSys.error("%s -- timed out after %i seconds." % + (realCmd, timeout)) + pgid = os.getpgid(popen.pid) + os.killpg(pgid, signal.SIGTERM) # Terminate the process + time.sleep(0.1) + retcode = popen.poll() + if retcode is None: # Still going... + os.killpg(pgid, signal.SIGKILL) # Kill the process time.sleep(0.1) retcode = popen.poll() - if retcode is None: - logSys.error("%s -- timed out after %i seconds." % - (realCmd, timeout)) - pgid = os.getpgid(popen.pid) - os.killpg(pgid, signal.SIGTERM) # Terminate the process - time.sleep(0.1) - retcode = popen.poll() - if retcode is None: # Still going... - os.killpg(pgid, signal.SIGKILL) # Kill the process - time.sleep(0.1) - retcode = popen.poll() - except OSError, e: - logSys.error("%s -- failed with %s" % (realCmd, e)) + except OSError as e: + logSys.error("%s -- failed with %s" % (realCmd, e)) finally: _cmd_lock.release() From 7cbb3980ebaf123eec0013169172f15c2877e711 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Sat, 12 Sep 2015 19:39:44 -0400 Subject: [PATCH 2/3] BF+RF: only return bool status on failed commands execution + mitigate different exit codes between bash/dash Closes #1155 --- fail2ban/server/action.py | 12 +++++++----- fail2ban/tests/actiontestcase.py | 14 ++++++-------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/fail2ban/server/action.py b/fail2ban/server/action.py index a8e61fb4..de0c8efc 100644 --- a/fail2ban/server/action.py +++ b/fail2ban/server/action.py @@ -561,6 +561,7 @@ class CommandAction(ActionBase): _cmd_lock.acquire() try: + retcode = None # to guarantee being defined upon early except stdout = tempfile.TemporaryFile(suffix=".stdout", prefix="fai2ban_") stderr = tempfile.TemporaryFile(suffix=".stderr", prefix="fai2ban_") @@ -603,15 +604,16 @@ class CommandAction(ActionBase): return True elif retcode is None: logSys.error("%s -- unable to kill PID %i" % (realCmd, popen.pid)) - elif retcode < 0: - logSys.error("%s -- killed with %s" % - (realCmd, signame.get(-retcode, "signal %i" % -retcode))) + elif retcode < 0 or retcode > 128: + # dash would return negative while bash 128 + n + sigcode = -retcode if retcode < 0 else retcode - 128 + logSys.error("%s -- killed with %s (return code: %s)" % + (realCmd, signame.get(sigcode, "signal %i" % sigcode), retcode)) else: msg = _RETCODE_HINTS.get(retcode, None) logSys.error("%s -- returned %i" % (realCmd, retcode)) if msg: logSys.info("HINT on %i: %s" % (retcode, msg % locals())) - return False - raise RuntimeError("Command execution failed: %s" % realCmd) + return False diff --git a/fail2ban/tests/actiontestcase.py b/fail2ban/tests/actiontestcase.py index febbc619..9e703bbf 100644 --- a/fail2ban/tests/actiontestcase.py +++ b/fail2ban/tests/actiontestcase.py @@ -196,11 +196,10 @@ class CommandActionTest(LogCaptureTestCase): def testExecuteTimeout(self): stime = time.time() # Should take a minute - self.assertRaises( - RuntimeError, CommandAction.executeCmd, 'sleep 60', timeout=2) + self.assertFalse(CommandAction.executeCmd('sleep 60', timeout=2)) # give a test still 1 second, because system could be too busy self.assertTrue(time.time() >= stime + 2 and time.time() <= stime + 3) - self.assertTrue(self._is_logged('sleep 60 -- timed out after 2 seconds') + self.assertTrue(self._is_logged('sleep 60 -- timed out after 2 seconds') or self._is_logged('sleep 60 -- timed out after 3 seconds')) self.assertTrue(self._is_logged('sleep 60 -- killed with SIGTERM')) @@ -222,17 +221,16 @@ class CommandActionTest(LogCaptureTestCase): return int(f.read()) # First test if can kill the bastard - self.assertRaises( - RuntimeError, CommandAction.executeCmd, 'bash %s' % tmpFilename, timeout=.1) + self.assertFalse(CommandAction.executeCmd( + 'bash %s' % tmpFilename, timeout=.1)) # Verify that the proccess itself got killed self.assertFalse(pid_exists(getnastypid())) # process should have been killed self.assertTrue(self._is_logged('timed out')) self.assertTrue(self._is_logged('killed with SIGTERM')) # A bit evolved case even though, previous test already tests killing children processes - self.assertRaises( - RuntimeError, CommandAction.executeCmd, 'out=`bash %s`; echo ALRIGHT' % tmpFilename, - timeout=.2) + self.assertFalse(CommandAction.executeCmd( + 'out=`bash %s`; echo ALRIGHT' % tmpFilename, timeout=.2)) # Verify that the proccess itself got killed self.assertFalse(pid_exists(getnastypid())) self.assertTrue(self._is_logged('timed out')) From fbdd0b74a10f65336724e78ac9b03b8eb86b84d6 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Sun, 13 Sep 2015 10:45:39 -0400 Subject: [PATCH 3/3] DOC: Changelog entry for this fix --- ChangeLog | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ChangeLog b/ChangeLog index 811473e7..4a95727b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -14,6 +14,9 @@ ver. 0.9.4 (2015/XX/XXX) - wanna-be-released * Fix dnsToIp resolver for fqdn with large list of IPs (gh-1164) * filter.d/apache-badbots.conf - Updated useragent string regex adding escape for `+` + * Treat failed and killed execution of commands identically (only + different log messages), which addresses different behavior on different + exit codes of dash and bash (gh-1155) - New Features: