FIX check the type of the entries in sys.modules (#326)

FIX type-check entries of sys.modules in _whichmodule


Some modules such as coverage can inject some non-modules
objects inside sys.modules, creating unpredictable bugs while
trying to infer the module of a function.

Author: pierreglaser

Diff for: CHANGES.md

@@ -1,6 +1,10 @@
 1.2.3
 =====
 
+- Fix a bug affecting cloudpickle when non-modules objects are added into
+  sys.modules
+  ([PR #326](https://github.com/cloudpipe/cloudpickle/pull/326)).
+  
 - Fix a regression in cloudpickle and python3.8 causing an error when trying to
   pickle property objects.
   ([PR #329](https://github.com/cloudpipe/cloudpickle/pull/329)).

Diff for: cloudpickle/cloudpickle.py

@@ -155,7 +155,13 @@ def _whichmodule(obj, name):
     # modules that trigger imports of other modules upon calls to getattr or
     # other threads importing at the same time.
     for module_name, module in sys.modules.copy().items():
-        if module_name == '__main__' or module is None:
+        # Some modules such as coverage can inject non-module objects inside
+        # sys.modules
+        if (
+                module_name == '__main__' or
+                module is None or
+                not isinstance(module, types.ModuleType)
+        ):
             continue
         try:
             if _getattribute(module, name)[0] is obj:

Diff for: tests/cloudpickle_test.py

@@ -42,7 +42,7 @@
 import cloudpickle
 from cloudpickle.cloudpickle import _is_dynamic
 from cloudpickle.cloudpickle import _make_empty_cell, cell_set
-from cloudpickle.cloudpickle import _extract_class_dict
+from cloudpickle.cloudpickle import _extract_class_dict, _whichmodule
 
 from .testutils import subprocess_pickle_echo
 from .testutils import assert_run_python_script
@@ -1048,35 +1048,94 @@ def __init__(self, x):
 
         self.assertEqual(set(weakset), {depickled1, depickled2})
 
-    def test_faulty_module(self):
-        for module_name in ['_missing_module', None]:
-            class FaultyModule(object):
-                def __getattr__(self, name):
-                    # This throws an exception while looking up within
-                    # pickle.whichmodule or getattr(module, name, None)
-                    raise Exception()
+    def test_non_module_object_passing_whichmodule_test(self):
+        # https://github.com/cloudpipe/cloudpickle/pull/326: cloudpickle should
+        # not try to instrospect non-modules object when trying to discover the
+        # module of a function/class. This happenened because codecov injects
+        # tuples (and not modules) into sys.modules, but type-checks were not
+        # carried out on the entries of sys.modules, causing cloupdickle to
+        # then error in unexpected ways
+        def func(x):
+            return x ** 2
+
+        # Trigger a loop during the execution of whichmodule(func) by
+        # explicitly setting the function's module to None
+        func.__module__ = None
+
+        class NonModuleObject(object):
+            def __getattr__(self, name):
+                # We whitelist func so that a _whichmodule(func, None) call returns
+                # the NonModuleObject instance if a type check on the entries
+                # of sys.modules is not carried out, but manipulating this
+                # instance thinking it really is a module later on in the
+                # pickling process of func errors out
+                if name == 'func':
+                    return func
+                else:
+                    raise AttributeError
+
+        non_module_object = NonModuleObject()
+
+        assert func(2) == 4
+        assert func is non_module_object.func
+
+        # Any manipulation of non_module_object relying on attribute access
+        # will raise an Exception
+        with pytest.raises(AttributeError):
+            _is_dynamic(non_module_object)
+
+        try:
+            sys.modules['NonModuleObject'] = non_module_object
 
-            class Foo(object):
-                __module__ = module_name
+            func_module_name = _whichmodule(func, None)
+            assert func_module_name != 'NonModuleObject'
+            assert func_module_name is None
 
-                def foo(self):
+            depickled_func = pickle_depickle(func, protocol=self.protocol)
+            assert depickled_func(2) == 4
+
+        finally:
+            sys.modules.pop('NonModuleObject')
+
+    def test_unrelated_faulty_module(self):
+        # Check that pickling a dynamically defined function or class does not
+        # fail when introspecting the currently loaded modules in sys.modules
+        # as long as those faulty modules are unrelated to the class or
+        # function we are currently pickling.
+        for base_class in (object, types.ModuleType):
+            for module_name in ['_missing_module', None]:
+                class FaultyModule(base_class):
+                    def __getattr__(self, name):
+                        # This throws an exception while looking up within
+                        # pickle.whichmodule or getattr(module, name, None)
+                        raise Exception()
+
+                class Foo(object):
+                    __module__ = module_name
+
+                    def foo(self):
+                        return "it works!"
+
+                def foo():
                     return "it works!"
 
-            def foo():
-                return "it works!"
+                foo.__module__ = module_name
 
-            foo.__module__ = module_name
+                if base_class is types.ModuleType:  # noqa
+                    faulty_module = FaultyModule('_faulty_module')
+                else:
+                    faulty_module = FaultyModule()
+                sys.modules["_faulty_module"] = faulty_module
 
-            sys.modules["_faulty_module"] = FaultyModule()
-            try:
-                # Test whichmodule in save_global.
-                self.assertEqual(pickle_depickle(Foo()).foo(), "it works!")
+                try:
+                    # Test whichmodule in save_global.
+                    self.assertEqual(pickle_depickle(Foo()).foo(), "it works!")
 
-                # Test whichmodule in save_function.
-                cloned = pickle_depickle(foo, protocol=self.protocol)
-                self.assertEqual(cloned(), "it works!")
-            finally:
-                sys.modules.pop("_faulty_module", None)
+                    # Test whichmodule in save_function.
+                    cloned = pickle_depickle(foo, protocol=self.protocol)
+                    self.assertEqual(cloned(), "it works!")
+                finally:
+                    sys.modules.pop("_faulty_module", None)
 
     def test_dynamic_pytest_module(self):
         # Test case for pull request https://github.com/cloudpipe/cloudpickle/pull/116