Skip to content

Commit

Permalink
avocado.main(): avoid an infinite fork loop bomb
Browse files Browse the repository at this point in the history
Because the current implementation of avocado.main() creates a job
and runs "sys.argv[0]" to implement standalone mode, it ends up
running itself over and over.

This simple proposed fix, prevents avocado.main() from running
itself again if called from itself. Since they are on different
processes, the mechanism chosen to do this is to set an environment
variable, that will be seen by the next process.

Also, by exiting from main() with an error code, the test first
level test will fail. This will let the user know that the chosen
approach (SIMPLE tests written in Python and calling main()) are
not worthy of a PASS.

This adresses issue avocado-framework#961.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
  • Loading branch information
clebergnu committed Jan 27, 2016
1 parent b9b0be8 commit bd19f35
Show file tree
Hide file tree
Showing 2 changed files with 80 additions and 1 deletion.
10 changes: 10 additions & 0 deletions avocado/core/job.py
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,16 @@ class TestProgram(object):
"""

def __init__(self):
# Avoid fork loop/bomb when running a test via avocado.main() that
# calls avocado.main() itself
if os.environ.get('AVOCADO_STANDALONE_IN_MAIN', False):
sys.stderr.write('AVOCADO_STANDALONE_IN_MAIN environment variable '
'found. This means that this code is being '
'called recursively. Exiting to avoid an infinite'
' fork loop.\n')
sys.exit(exit_codes.AVOCADO_FAIL)
os.environ['AVOCADO_STANDALONE_IN_MAIN'] = 'True'

self.defaultTest = sys.argv[0]
self.progName = os.path.basename(sys.argv[0])
self.parseArgs(sys.argv[1:])
Expand Down
71 changes: 70 additions & 1 deletion selftests/functional/test_loader.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
else:
import unittest

from avocado.core import exit_codes
from avocado.utils import script
from avocado.utils import process

Expand Down Expand Up @@ -82,7 +83,6 @@ def test(self):
main()
"""


NOT_A_TEST = """
def hello():
print('Hello World!')
Expand All @@ -100,6 +100,40 @@ def hello():
true
"""

AVOCADO_SIMPLE_PYTHON_LIKE_MULTIPLE_FILES = """#!/usr/bin/env python
# A simple test (executable bit set when saved to file) that looks like
# an Avocado instrumented test, with base class on separate file
from avocado import Test
from avocado import main
from test2 import *
class BasicTestSuite(SuperTest):
def test1(self):
self.xxx()
self.assertTrue(True)
if __name__ == '__main__':
main()
"""

AVOCADO_SIMPLE_PYTHON_LIKE_MULTIPLE_FILES_LIB = """
#!/usr/bin/python
from avocado import Test
class SuperTest(Test):
def xxx(self):
print "ahoj"
"""

AVOCADO_TEST_SIMPLE_USING_MAIN = """#!/usr/bin/env python
from avocado import main
if __name__ == "__main__":
main()
"""


class LoaderTestFunctional(unittest.TestCase):

Expand Down Expand Up @@ -161,6 +195,41 @@ def test_load_not_a_test(self):
def test_load_not_a_test_not_exec(self):
self._test('notatest.py', NOT_A_TEST, 'NOT_A_TEST')

def test_runner_simple_python_like_multiple_files(self):
mylib = script.TemporaryScript(
'test2.py',
AVOCADO_SIMPLE_PYTHON_LIKE_MULTIPLE_FILES_LIB,
'avocado_simpletest_functional',
0644)
mylib.save()
mytest = script.Script(
os.path.join(os.path.dirname(mylib.path), 'test.py'),
AVOCADO_SIMPLE_PYTHON_LIKE_MULTIPLE_FILES)
os.chdir(basedir)
mytest.save()
cmd_line = "./scripts/avocado list -V %s" % mytest
result = process.run(cmd_line)
self.assertIn('SIMPLE: 1', result.stdout)
# job should be able to finish under 5 seconds. If this fails, it's
# possible that we hit the "simple test fork bomb" bug

This comment has been minimized.

Copy link
@lmr

lmr Jan 27, 2016

One doubt here, if the simple test fork bomb bug happens, will avocado handle things correctly just by passing --job-timeout to it? Isn't it better to add a timeout to the process.run call?

This comment has been minimized.

Copy link
@clebergnu

clebergnu Jan 27, 2016

Author Owner

Avocado should behave correctly and not "fork bomb" no matter if --job-timeout is used or not.

The point of using --job-timeout is that the functional test finishes properly, and it also tests how that part of avocado behaves when things go awry (like in a fork bomb). We could indeed swap the timeout from the avocado job to process.run but I fail to see the point. Those functional tests can be run without the fix and they report the failure. Again, the "better behavior" of main() (it's hard to call it the "right behavior) does not depend on the timeouts.

This comment has been minimized.

Copy link
@ldoktor

ldoktor Jan 27, 2016

Well actually in case of job timeout, the test fails and avocado also reports exit_codes.AVOCADO_TESTS_FAIL. So this unittest passes on current master.

How about using --show-job-log and look for the message?

This comment has been minimized.

Copy link
@ldoktor

ldoktor Jan 27, 2016

Or, as suggested by @lmr, how about using the process.run timeout, that would distinguish the results...

This comment has been minimized.

Copy link
@ldoktor

ldoktor Jan 27, 2016

Actually it's even worse, when you execute this unittest on current master, it passes AND it keeps the fork bomb active... (thanks god for pkill -f)

This comment has been minimized.

Copy link
@ldoktor

ldoktor Jan 27, 2016

cool, when you try that, it won't finish ever... There is an odor of decay in the kingdom of avocado.utils.process...

This comment has been minimized.

Copy link
@ldoktor

ldoktor Jan 27, 2016

It actually hangs in _fill_streams in self.stdout_thread.join(), so the process is dead, but the stdout thread is not finished.

This comment has been minimized.

Copy link
@ldoktor

ldoktor Jan 27, 2016

Well it's too late to think. One way to avoid hang of this unittest when running it on the old code is:

# avocado.utils.process
    def _fill_streams(self):
        """
        Close subprocess stdout and stderr, and put values into result obj.
        """
        # Cleaning up threads
        self.stdout_thread.join(1)
        self.stderr_thread.join(1)
        # Clean subprocess pipes and populate stdout/err
        self._popen.stdout.close()
        self._popen.stderr.close()
        if self.stdout_thread.isAlive() or self.stderr_thread.isAlive():
            self.stdout_thread.join(1)
            self.stderr_thread.join(1)
            if self.stdout_thread.isAlive() or self.stderr_thread.isAlive():
                import sys
                sys.exit(-1)
        self.result.stdout = self.get_stdout()
        self.result.stderr = self.get_stderr()

This comment has been minimized.

Copy link
@ldoktor

ldoktor Jan 27, 2016

Anyway we have to investigate this bug, because I saw avocado hanging for infinity, which is usually caused by unfinished background threads, so our process is not always behaving correct.

PS: the fd_drainer hangs in the os.read
PPS: In the example above I also made the thread using non-blocking read

the whole diff is:

diff --git a/avocado/utils/process.py b/avocado/utils/process.py
index a1f5cab..872d17b 100644
--- a/avocado/utils/process.py
+++ b/avocado/utils/process.py
@@ -26,6 +26,7 @@ import shlex
 import shutil
 import threading
 import fnmatch
+import fcntl

 try:
     import subprocess32 as subprocess
@@ -244,6 +245,7 @@ class SubProcess(object):
         self.allow_output_check = allow_output_check
         self.result = CmdResult(self.cmd)
         self.shell = shell
+        self.exiting = False
         if env:
             self.env = os.environ.copy()
             self.env.update(env)
@@ -341,9 +343,11 @@ class SubProcess(object):
             lock = self.stderr_lock

         fileno = input_pipe.fileno()
+        flag = fcntl.fcntl(fileno, fcntl.F_GETFD)
+        fcntl.fcntl(fileno, fcntl.F_SETFD, flag | os.O_NONBLOCK)

         bfr = ''
-        while True:
+        while not self.exiting:
             tmp = os.read(fileno, 1024)
             if tmp == '':
                 if self.verbose and bfr:
@@ -378,11 +382,18 @@ class SubProcess(object):
         Close subprocess stdout and stderr, and put values into result obj.
         """
         # Cleaning up threads
-        self.stdout_thread.join()
-        self.stderr_thread.join()
+        self.stdout_thread.join(1)
+        self.stderr_thread.join(1)
         # Clean subprocess pipes and populate stdout/err
         self._popen.stdout.close()
         self._popen.stderr.close()
+        if self.stdout_thread.isAlive() or self.stderr_thread.isAlive():
+            self.exiting = True
+            self.stdout_thread.join(1)
+            self.stderr_thread.join(1)
+            if self.stdout_thread.isAlive() or self.stderr_thread.isAlive():
+                import sys
+                sys.exit(-1)
         self.result.stdout = self.get_stdout()
         self.result.stderr = self.get_stderr()

diff --git a/selftests/functional/test_loader.py b/selftests/functional/test_loader.py
index 7e31e54..ac9b2f5 100644
--- a/selftests/functional/test_loader.py
+++ b/selftests/functional/test_loader.py
@@ -9,6 +9,7 @@ if sys.version_info[:2] == (2, 6):
 else:
     import unittest

+from avocado.core import exit_codes
 from avocado.utils import script
 from avocado.utils import process

@@ -82,7 +83,6 @@ if __name__ == "__main__":
     main()
 """

-
 NOT_A_TEST = """
 def hello():
     print('Hello World!')
@@ -100,6 +100,40 @@ SIMPLE_TEST = """#!/bin/sh
 true
 """

+AVOCADO_SIMPLE_PYTHON_LIKE_MULTIPLE_FILES = """#!/usr/bin/env python
+# A simple test (executable bit set when saved to file) that looks like
+# an Avocado instrumented test, with base class on separate file
+from avocado import Test
+from avocado import main
+from test2 import *
+
+class BasicTestSuite(SuperTest):
+
+    def test1(self):
+        self.xxx()
+        self.assertTrue(True)
+
+if __name__ == '__main__':
+    main()
+"""
+
+AVOCADO_SIMPLE_PYTHON_LIKE_MULTIPLE_FILES_LIB = """
+#!/usr/bin/python
+
+from avocado import Test
+
+class SuperTest(Test):
+    def xxx(self):
+        print "ahoj"
+"""
+
+AVOCADO_TEST_SIMPLE_USING_MAIN = """#!/usr/bin/env python
+from avocado import main
+
+if __name__ == "__main__":
+    main()
+"""
+

 class LoaderTestFunctional(unittest.TestCase):

@@ -117,49 +151,18 @@ class LoaderTestFunctional(unittest.TestCase):
         self.assertIn('%s: %s' % (exp_str, count), result.stdout)
         test_script.remove()

-    def test_simple(self):
-        self._test('simpletest.sh', SIMPLE_TEST, 'SIMPLE', 0775)
-
-    def test_simple_not_exec(self):
-        self._test('simpletest.sh', SIMPLE_TEST, 'NOT_A_TEST')
-
-    def test_pass(self):
-        self._test('passtest.py', AVOCADO_TEST_OK, 'INSTRUMENTED')
-
-    def test_sleep_a_lot(self):
-        """
-        Verifies that the test loader, at list time, does not load the Python
-        module and thus executes its contents.
-        """
-        test_script = script.TemporaryScript('sleepeleven.py',
-                                             AVOCADO_TEST_SLEEP_ELEVEN,
-                                             'avocado_loader_test',
-                                             mode=0664)
-        test_script.save()
-        cmd_line = ('./scripts/avocado list -V %s' % test_script.path)
-        initial_time = time.time()
-        result = process.run(cmd_line, ignore_status=True)
-        test_script.remove()
-        actual_time = time.time() - initial_time
-        self.assertLess(actual_time, 3.0,
-                        ("Took more than 3 seconds to list tests. Loader "
-                         "probably loaded/executed Python code and slept for "
-                         "eleven seconds."))
-        self.assertIn('INSTRUMENTED: 2', result.stdout)
-
-    def test_multiple_class(self):
-        self._test('multipleclasses.py', AVOCADO_TEST_MULTIPLE_CLASSES,
-                   'INSTRUMENTED', 0664, 2)
-
-    def test_multiple_methods_same_name(self):
-        self._test('multiplemethods.py', AVOCADO_TEST_MULTIPLE_METHODS_SAME_NAME,
-                   'INSTRUMENTED', 0664, 1)
-
-    def test_load_not_a_test(self):
-        self._test('notatest.py', NOT_A_TEST, 'SIMPLE', 0775)
-
-    def test_load_not_a_test_not_exec(self):
-        self._test('notatest.py', NOT_A_TEST, 'NOT_A_TEST')
+    def test_simple_using_main(self):
+        mytest = script.TemporaryScript("simple_using_main.py",
+                                        AVOCADO_TEST_SIMPLE_USING_MAIN,
+                                        'avocado_simpletest_functional')
+        mytest.save()
+        os.chdir(basedir)
+        # job should be able to finish under 5 seconds. If this fails, it's
+        # possible that we hit the "simple test fork bomb" bug
+        cmd_line = ("./scripts/avocado run --sysinfo=off --job-results-dir %s "
+                    "%s" % (self.tmpdir, mytest))
+        result = process.run(cmd_line, ignore_status=True, timeout=5)
+        self.assertEquals(result.exit_status, exit_codes.AVOCADO_TESTS_FAIL)

     def tearDown(self):
         shutil.rmtree(self.tmpdir)

This comment has been minimized.

Copy link
@lmr

lmr Jan 27, 2016

@ldoktor, I must say I liked the Shakespeare reference in your comments.

This comment has been minimized.

Copy link
@lmr

lmr Jan 27, 2016

And well, it's good that we found out process that (mis)behave in such a fashion that it exposes bugs in the process.SubProcess implementation. Adding unittests to the process module was something that I postponed for way too long, I've started to do it now, and I hope we can write more as we fix this bug that was found.

cmd_line = ("./scripts/avocado run --sysinfo=off --job-results-dir %s "
"--job-timeout=5s %s" % (self.tmpdir, mytest))
result = process.run(cmd_line, ignore_status=True)
self.assertEquals(result.exit_status, exit_codes.AVOCADO_TESTS_FAIL)

def test_simple_using_main(self):
mytest = script.TemporaryScript("simple_using_main.py",
AVOCADO_TEST_SIMPLE_USING_MAIN,
'avocado_simpletest_functional')
mytest.save()
os.chdir(basedir)
# job should be able to finish under 5 seconds. If this fails, it's
# possible that we hit the "simple test fork bomb" bug
cmd_line = ("./scripts/avocado run --sysinfo=off --job-results-dir %s "
"--job-timeout=5s %s" % (self.tmpdir, mytest))
result = process.run(cmd_line, ignore_status=True)
self.assertEquals(result.exit_status, exit_codes.AVOCADO_TESTS_FAIL)

def tearDown(self):
shutil.rmtree(self.tmpdir)

Expand Down

1 comment on commit bd19f35

@clebergnu
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ldoktor right, the unittest passes because of the change in behavior on this v3 that looks for exit_codes.AVOCADO_TESTS_FAIL. This has indeed to be fixed.

Now, if we noticed more broken things, say in process.run(), it'd be wise to take one at a time.

Please sign in to comment.