From ca182eef0729059582744c38169de9956072360e Mon Sep 17 00:00:00 2001 From: Julien Muchembled <jm@nexedi.com> Date: Wed, 22 Jun 2011 15:17:48 +0200 Subject: [PATCH] Remove most dangerous uses of 'chdir' syscall Because chdir/getcwd is global to the whole process, it is not thread-safe and may cause very serious bugs like data loss (for example when 'os.remove' or 'shutil.rmtree' are called with relative paths). There still remain uses of 'chdir' in ERP5 Subversion. A temporary quick change is done to reduce the probability of race conditions, but this should really be fixed. --- product/ERP5/ERP5Site.py | 36 +++++++++++++++---------------- product/ERP5/Tool/TemplateTool.py | 22 +++++-------------- product/ERP5VCS/Subversion.py | 23 +++++++++++++++++++- product/ERP5VCS/WorkingCopy.py | 34 +++++++++-------------------- 4 files changed, 54 insertions(+), 61 deletions(-) diff --git a/product/ERP5/ERP5Site.py b/product/ERP5/ERP5Site.py index 9dc95e13ed..aef3eb2ae8 100644 --- a/product/ERP5/ERP5Site.py +++ b/product/ERP5/ERP5Site.py @@ -1619,25 +1619,23 @@ class ERP5Generator(PortalGenerator): @classmethod def bootstrap(cls, context, bt_name, item_name, content_id_list): - cwd = os.getcwd() - try: - os.chdir(getBootstrapBusinessTemplateUrl(bt_name)) - from Products.ERP5.Document.BusinessTemplate import quote - traverse = context.unrestrictedTraverse - for root, dirs, files in os.walk(os.path.join(item_name, context.id)): - container_path = root.split(os.sep)[2:] - load = traverse(container_path)._importObjectFromFile - if container_path: - id_set = set(x[:-4] for x in files if x[-4:] == '.xml') - else: - id_set = set(quote(x) for x in content_id_list - if not context.hasObject(x)) - dirs[:] = id_set.intersection(dirs) - for file in id_set: - load(os.path.join(root, file + '.xml'), - verify=False, set_owner=False, suppress_events=True) - finally: - os.chdir(cwd) + bt_path = getBootstrapBusinessTemplateUrl(bt_name) + from Products.ERP5.Document.BusinessTemplate import quote + traverse = context.unrestrictedTraverse + top = os.path.join(bt_path, item_name, context.id) + prefix_len = len(os.path.join(top, '')) + for root, dirs, files in os.walk(top): + container_path = root[prefix_len:] + load = traverse(container_path)._importObjectFromFile + if container_path: + id_set = set(x[:-4] for x in files if x[-4:] == '.xml') + else: + id_set = set(quote(x) for x in content_id_list + if not context.hasObject(x)) + dirs[:] = id_set.intersection(dirs) + for file in id_set: + load(os.path.join(root, file + '.xml'), + verify=False, set_owner=False, suppress_events=True) def setupLastTools(self, p, **kw): """ diff --git a/product/ERP5/Tool/TemplateTool.py b/product/ERP5/Tool/TemplateTool.py index d6c346c6b4..37dbc5b19e 100644 --- a/product/ERP5/Tool/TemplateTool.py +++ b/product/ERP5/Tool/TemplateTool.py @@ -256,27 +256,15 @@ class TemplateTool (BaseTool): Export the Business Template as a bt5 file and offer the user to download it. """ - path = business_template.getTitle() - path = pathname2url(path) - # XXX Why is it necessary to create a temporary directory? - tmpdir_path = mkdtemp() - # XXX not thread safe - current_directory = os.getcwd() - os.chdir(tmpdir_path) - absolute_path = os.path.abspath(path) - export_string = business_template.export(path=absolute_path) - os.chdir(current_directory) - if RESPONSE is not None: - RESPONSE.setHeader('Content-type','tar/x-gzip') - RESPONSE.setHeader('Content-Disposition', - 'inline;filename=%s-%s.bt5' % \ - (path, - business_template.getVersion())) + export_string = business_template.export() try: + if RESPONSE is not None: + RESPONSE.setHeader('Content-type','tar/x-gzip') + RESPONSE.setHeader('Content-Disposition', 'inline;filename=%s-%s.bt5' + % (business_template.getTitle(), business_template.getVersion())) return export_string.getvalue() finally: export_string.close() - shutil.rmtree(tmpdir_path) security.declareProtected( 'Import/Export objects', 'publish' ) def publish(self, business_template, url, username=None, password=None): diff --git a/product/ERP5VCS/Subversion.py b/product/ERP5VCS/Subversion.py index c9cecb3ace..d3b62cb4ad 100644 --- a/product/ERP5VCS/Subversion.py +++ b/product/ERP5VCS/Subversion.py @@ -30,15 +30,35 @@ ############################################################################## import errno, glob, os, re, shutil +import threading from ZTUtils import make_query from Products.ERP5Type.Message import translateString +from Products.ERP5Type.Utils import simple_decorator from Products.ERP5.Document.BusinessTemplate import BusinessTemplateFolder from Products.ERP5VCS.WorkingCopy import \ - WorkingCopy, Dir, File, chdir_working_copy, selfcached, \ + WorkingCopy, Dir, File, selfcached, \ NotAWorkingCopyError, NotVersionedError, VcsConflictError from Products.ERP5VCS.SubversionClient import \ newSubversionClient, SubversionLoginError, SubversionSSLTrustError +# XXX Still not thread safe !!! Proper fix is to never use 'os.chdir' +# Using a RLock is a temporary quick change that only protects against +# concurrent uses of ERP5 Subversion. +_chdir_lock = threading.RLock() +@simple_decorator +def chdir_working_copy(func): + def decorator(self, *args, **kw): + _chdir_lock.acquire() + try: + cwd = os.getcwd() + try: + os.chdir(self.working_copy) + return func(self, *args, **kw) + finally: + os.chdir(cwd) + finally: + _chdir_lock.release() + return decorator class Subversion(WorkingCopy): @@ -238,6 +258,7 @@ class Subversion(WorkingCopy): 'Files committed successfully in revision ${revision}', mapping=dict(revision=getRevisionNumber(revision)))))) + @chdir_working_copy def _export(self, business_template): bta = BusinessTemplateWorkingCopy(creation=1, client=self._getClient()) bta.export(business_template) diff --git a/product/ERP5VCS/WorkingCopy.py b/product/ERP5VCS/WorkingCopy.py index e33ec5a32c..33ab9612e2 100644 --- a/product/ERP5VCS/WorkingCopy.py +++ b/product/ERP5VCS/WorkingCopy.py @@ -42,17 +42,6 @@ from ZTUtils import make_query from Products.ERP5.Document.BusinessTemplate import BusinessTemplateFolder from Products.ERP5Type.Utils import simple_decorator -@simple_decorator -def chdir_working_copy(func): - def decorator(self, *args, **kw): - cwd = os.getcwd() - try: - os.chdir(self.working_copy) - return func(self, *args, **kw) - finally: - os.chdir(cwd) - return decorator - @simple_decorator def selfcached(func): """Return a function which stores a computed value in an instance @@ -202,15 +191,10 @@ class WorkingCopy(Implicit): business_template.build() # XXX: Big hack to make export work as expected. transaction.commit() - old_cwd = os.getcwd() - try: - os.chdir(self.working_copy) - self._export(business_template) - finally: - os.chdir(old_cwd) + self._export(business_template) def _export(self, business_template): - bta = BusinessTemplateWorkingCopy(creation=1) + bta = BusinessTemplateWorkingCopy(creation=1, path=self.working_copy) self.addremove(*bta.export(business_template)) def showOld(self, path): @@ -386,6 +370,7 @@ class BusinessTemplateWorkingCopy(BusinessTemplateFolder): def _writeString(self, obj, path): self.file_set.add(path) self._makeParent(path) + path = os.path.join(self.path, path) # write file unless unchanged file = None try: @@ -412,8 +397,9 @@ class BusinessTemplateWorkingCopy(BusinessTemplateFolder): path = os.path.dirname(path) if path and path not in self.dir_set: self._makeParent(path) - if not os.path.exists(path): - os.mkdir(path) + real_path = os.path.join(self.path, path) + if not os.path.exists(real_path): + os.mkdir(real_path) self.dir_set.add(path) def export(self, business_template): @@ -423,8 +409,8 @@ class BusinessTemplateWorkingCopy(BusinessTemplateFolder): business_template.export(bta=self) # Remove dangling files/dirs removed_set = set() - prefix_length = len(os.path.join('.', '')) - for dirpath, dirnames, filenames in os.walk('.'): + prefix_length = len(os.path.join(self.path, '')) + for dirpath, dirnames, filenames in os.walk(self.path): dirpath = dirpath[prefix_length:] for i in xrange(len(dirnames) - 1, -1, -1): d = dirnames[i] @@ -432,13 +418,13 @@ class BusinessTemplateWorkingCopy(BusinessTemplateFolder): d = os.path.join(dirpath, d) if d in self.dir_set: continue - shutil.rmtree(d) + shutil.rmtree(os.path.join(self.path, d)) removed_set.add(d) del dirnames[i] for f in filenames: f = os.path.join(dirpath, f) if f not in self.file_set: - os.remove(f) + os.remove(os.path.join(self.path, f)) removed_set.add(f) return self.file_set, removed_set -- 2.30.9