Bug 1313306 - Don't expose os.path.{exists,isdir,isfile} to python configure without an @imports. r?chmanchester draft
authorMike Hommey <mh+mozilla@glandium.org>
Wed, 26 Oct 2016 11:49:58 +0900
changeset 430144 711322281e6f7b85c38c53d2607f017a56556b03
parent 430143 d3f548b9453b9c15ae854d0511a2a48521eefde0
child 430145 c60a2966c429fd01e58ae593005b9685a2b051d4
push id33750
push userbmo:mh+mozilla@glandium.org
push dateThu, 27 Oct 2016 07:43:21 +0000
reviewerschmanchester
bugs1313306
milestone52.0a1
Bug 1313306 - Don't expose os.path.{exists,isdir,isfile} to python configure without an @imports. r?chmanchester We want functions without an @imports to not have any side effects, and to not use external resources. So remove the few functions we expose from os.path without @imports('os') that do.
build/moz.configure/init.configure
build/moz.configure/old.configure
python/mozbuild/mozbuild/configure/__init__.py
python/mozbuild/mozbuild/test/configure/common.py
python/mozbuild/mozbuild/test/configure/data/moz.configure
python/mozbuild/mozbuild/test/configure/test_configure.py
--- a/build/moz.configure/init.configure
+++ b/build/moz.configure/init.configure
@@ -7,16 +7,17 @@
 include('util.configure')
 include('checks.configure')
 
 option(env='DIST', nargs=1, help='DIST directory')
 
 # Do not allow objdir == srcdir builds.
 # ==============================================================
 @depends('--help', 'DIST')
+@imports('os')
 def check_build_environment(help, dist):
     topobjdir = os.path.realpath(os.path.abspath('.'))
     topsrcdir = os.path.realpath(os.path.abspath(
         os.path.join(os.path.dirname(__file__), '..', '..')))
 
     if dist:
         dist = normsep(dist[0])
     else:
@@ -628,16 +629,17 @@ def default_project(build_env, help):
 option('--enable-project', nargs=1, default=default_project,
        help='Project to build')
 
 option('--with-external-source-dir', env='EXTERNAL_SOURCE_DIR', nargs=1,
        help='External directory containing additional build files')
 
 @depends('--enable-project', '--with-external-source-dir',
          check_build_environment, '--help')
+@imports('os')
 def include_project_configure(project, external_source_dir, build_env, help):
     if not project:
         die('--enable-project is required.')
 
     base_dir = build_env.topsrcdir
     if external_source_dir:
         base_dir = os.path.join(base_dir, external_source_dir[0])
 
--- a/build/moz.configure/old.configure
+++ b/build/moz.configure/old.configure
@@ -10,16 +10,17 @@ def encoded_open(path, mode):
     encoding = 'mbcs' if sys.platform == 'win32' else 'utf-8'
     return codecs.open(path, mode, encoding)
 
 
 option(env='AUTOCONF', nargs=1, help='Path to autoconf 2.13')
 
 @depends(mozconfig, 'AUTOCONF')
 @checking('for autoconf')
+@imports('os')
 @imports('re')
 def autoconf(mozconfig, autoconf):
     mozconfig_autoconf = None
     if mozconfig['path']:
         make_extra = mozconfig['make_extra']
         if make_extra:
             for assignment in make_extra:
                 m = re.match('(?:export\s+)?AUTOCONF\s*:?=\s*(.+)$',
@@ -59,16 +60,17 @@ set_config('AUTOCONF', autoconf)
 
 
 @depends('OLD_CONFIGURE', mozconfig, autoconf, check_build_environment, shell,
          old_configure_assignments, build_project)
 @imports(_from='__builtin__', _import='open')
 @imports(_from='__builtin__', _import='print')
 @imports('glob')
 @imports('itertools')
+@imports('os')
 @imports('subprocess')
 # Import getmtime without overwriting the sandbox os.path.
 @imports(_from='os.path', _import='getmtime')
 @imports(_from='mozbuild.shellutil', _import='quote')
 def prepare_configure(old_configure, mozconfig, autoconf, build_env, shell,
                       old_configure_assignments, build_project):
     # os.path.abspath in the sandbox will ensure forward slashes on Windows,
     # which is actually necessary because this path actually ends up literally
--- a/python/mozbuild/mozbuild/configure/__init__.py
+++ b/python/mozbuild/mozbuild/configure/__init__.py
@@ -193,19 +193,18 @@ class ConfigureSandbox(dict):
         for b in ('None', 'False', 'True', 'int', 'bool', 'any', 'all', 'len',
                   'list', 'tuple', 'set', 'dict', 'isinstance', 'getattr',
                   'hasattr', 'enumerate', 'range', 'zip')
     }, __import__=forbidden_import, str=unicode)
 
     # Expose a limited set of functions from os.path
     OS = ReadOnlyNamespace(path=ReadOnlyNamespace(**{
         k: getattr(mozpath, k, getattr(os.path, k))
-        for k in ('abspath', 'basename', 'dirname', 'exists', 'isabs', 'isdir',
-                  'isfile', 'join', 'normcase', 'normpath', 'realpath',
-                  'relpath')
+        for k in ('abspath', 'basename', 'dirname', 'isabs', 'join',
+                  'normcase', 'normpath', 'realpath', 'relpath')
     }))
 
     def __init__(self, config, environ=os.environ, argv=sys.argv,
                  stdout=sys.stdout, stderr=sys.stderr, logger=None):
         dict.__setitem__(self, '__builtins__', self.BUILTINS)
 
         self._paths = []
         self._all_paths = set()
--- a/python/mozbuild/mozbuild/test/configure/common.py
+++ b/python/mozbuild/mozbuild/test/configure/common.py
@@ -81,21 +81,25 @@ class ConfigureTestSandbox(ConfigureSand
         if 'CONFIG_SHELL' not in environ:
             environ['CONFIG_SHELL'] = mozpath.abspath('/bin/sh')
             self._subprocess_paths[environ['CONFIG_SHELL']] = self.shell
             paths.append(environ['CONFIG_SHELL'])
         self._environ = copy.copy(environ)
 
         vfs = ConfigureTestVFS(paths)
 
-        self.OS = ReadOnlyNamespace(path=ReadOnlyNamespace(**{
-            k: v if k not in ('exists', 'isfile')
-            else getattr(vfs, k)
-            for k, v in ConfigureSandbox.OS.path.__dict__.iteritems()
-        }))
+        os_path = {
+            k: getattr(vfs, k)
+            for k in dir(vfs)
+            if not k.startswith('_')
+        }
+
+        os_path.update(self.OS.path.__dict__)
+
+        self.imported_os = ReadOnlyNamespace(path=ReadOnlyNamespace(**os_path))
 
         super(ConfigureTestSandbox, self).__init__(config, environ, *args,
                                                    **kwargs)
 
     def _get_one_import(self, what):
         if what == 'which.which':
             return self.which
 
@@ -166,17 +170,17 @@ class ConfigureTestSandbox(ConfigureSand
     def GetShortPathNameW(self, path_in, path_out, length):
         path_out.value = path_in
         return length
 
     def which(self, command, path=None):
         for parent in (path or self._search_path):
             c = mozpath.abspath(mozpath.join(parent, command))
             for candidate in (c, ensure_exe_extension(c)):
-                if self.OS.path.exists(candidate):
+                if self.imported_os.path.exists(candidate):
                     return candidate
         raise WhichError()
 
     def Popen(self, args, stdin=None, stdout=None, stderr=None, **kargs):
         try:
             program = self.which(args[0])
         except WhichError:
             raise OSError(errno.ENOENT, 'File not found')
--- a/python/mozbuild/mozbuild/test/configure/data/moz.configure
+++ b/python/mozbuild/mozbuild/test/configure/data/moz.configure
@@ -147,19 +147,19 @@ include(include_path)
 # The order of the decorators matter: @imports needs to appear after other
 # decorators.
 option('--with-imports', nargs='?', help='Imports')
 
 # A limited set of functions from os.path are exposed by default.
 @depends('--with-imports')
 def with_imports(value):
     if len(value):
-        return os.path.isfile(value[0])
+        return hasattr(os.path, 'abspath')
 
-set_config('IS_FILE', with_imports)
+set_config('HAS_ABSPATH', with_imports)
 
 # It is still possible to import the full set from os.path.
 # It is also possible to cherry-pick builtins.
 @depends('--with-imports')
 @imports('os.path')
 def with_imports(value):
     if len(value):
         return hasattr(os.path, 'getatime')
--- a/python/mozbuild/mozbuild/test/configure/test_configure.py
+++ b/python/mozbuild/mozbuild/test/configure/test_configure.py
@@ -361,23 +361,18 @@ class TestConfigure(unittest.TestCase):
             foo()'''),
             sandbox
         )
 
         self.assertEquals(len(imports), 1)
 
     def test_os_path(self):
         config = self.get_config(['--with-imports=%s' % __file__])
-        self.assertIn('IS_FILE', config)
-        self.assertEquals(config['IS_FILE'], True)
-
-        config = self.get_config(['--with-imports=%s.no-exist' % __file__])
-        self.assertIn('IS_FILE', config)
-        self.assertEquals(config['IS_FILE'], False)
-
+        self.assertIn('HAS_ABSPATH', config)
+        self.assertEquals(config['HAS_ABSPATH'], True)
         self.assertIn('HAS_GETATIME', config)
         self.assertEquals(config['HAS_GETATIME'], True)
         self.assertIn('HAS_GETATIME2', config)
         self.assertEquals(config['HAS_GETATIME2'], False)
 
     def test_template_call(self):
         config = self.get_config(env={'CC': 'gcc'})
         self.assertIn('TEMPLATE_VALUE', config)