BF+RF: only return bool status on failed commands execution + mitigate different exit codes between bash/dash

Closes #1155

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:
+		elif retcode < 0 or retcode > 128:
-			logSys.error("%s -- killed with %s" %
+			# dash would return negative while bash 128 + n
-				(realCmd, signame.get(-retcode, "signal %i" % -retcode)))
+			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
+		return False
-		raise RuntimeError("Command execution failed: %s" % realCmd)
 

fail2ban/tests/actiontestcase.py

@@ -196,11 +196,10 @@ class CommandActionTest(LogCaptureTestCase):
 	def testExecuteTimeout(self):
 		stime = time.time()
 		# Should take a minute
-		self.assertRaises(
+		self.assertFalse(CommandAction.executeCmd('sleep 60', timeout=2))
-			RuntimeError, 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(
+		self.assertFalse(CommandAction.executeCmd(
-			RuntimeError, CommandAction.executeCmd, 'bash %s' % tmpFilename, timeout=.1)
+		                 '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(
+		self.assertFalse(CommandAction.executeCmd(
-			RuntimeError, CommandAction.executeCmd, 'out=`bash %s`; echo ALRIGHT' % tmpFilename,
+			'out=`bash %s`; echo ALRIGHT' % tmpFilename, timeout=.2))
-			timeout=.2)
 		# Verify that the proccess itself got killed
 		self.assertFalse(pid_exists(getnastypid()))
 		self.assertTrue(self._is_logged('timed out'))