Commit ca182eef authored by Julien Muchembled's avatar Julien Muchembled

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.
parent 45e56b46
...@@ -1619,25 +1619,23 @@ class ERP5Generator(PortalGenerator): ...@@ -1619,25 +1619,23 @@ class ERP5Generator(PortalGenerator):
@classmethod @classmethod
def bootstrap(cls, context, bt_name, item_name, content_id_list): def bootstrap(cls, context, bt_name, item_name, content_id_list):
cwd = os.getcwd() bt_path = getBootstrapBusinessTemplateUrl(bt_name)
try: from Products.ERP5.Document.BusinessTemplate import quote
os.chdir(getBootstrapBusinessTemplateUrl(bt_name)) traverse = context.unrestrictedTraverse
from Products.ERP5.Document.BusinessTemplate import quote top = os.path.join(bt_path, item_name, context.id)
traverse = context.unrestrictedTraverse prefix_len = len(os.path.join(top, ''))
for root, dirs, files in os.walk(os.path.join(item_name, context.id)): for root, dirs, files in os.walk(top):
container_path = root.split(os.sep)[2:] container_path = root[prefix_len:]
load = traverse(container_path)._importObjectFromFile load = traverse(container_path)._importObjectFromFile
if container_path: if container_path:
id_set = set(x[:-4] for x in files if x[-4:] == '.xml') id_set = set(x[:-4] for x in files if x[-4:] == '.xml')
else: else:
id_set = set(quote(x) for x in content_id_list id_set = set(quote(x) for x in content_id_list
if not context.hasObject(x)) if not context.hasObject(x))
dirs[:] = id_set.intersection(dirs) dirs[:] = id_set.intersection(dirs)
for file in id_set: for file in id_set:
load(os.path.join(root, file + '.xml'), load(os.path.join(root, file + '.xml'),
verify=False, set_owner=False, suppress_events=True) verify=False, set_owner=False, suppress_events=True)
finally:
os.chdir(cwd)
def setupLastTools(self, p, **kw): def setupLastTools(self, p, **kw):
""" """
......
...@@ -256,27 +256,15 @@ class TemplateTool (BaseTool): ...@@ -256,27 +256,15 @@ class TemplateTool (BaseTool):
Export the Business Template as a bt5 file and offer the user to Export the Business Template as a bt5 file and offer the user to
download it. download it.
""" """
path = business_template.getTitle() export_string = business_template.export()
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()))
try: 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() return export_string.getvalue()
finally: finally:
export_string.close() export_string.close()
shutil.rmtree(tmpdir_path)
security.declareProtected( 'Import/Export objects', 'publish' ) security.declareProtected( 'Import/Export objects', 'publish' )
def publish(self, business_template, url, username=None, password=None): def publish(self, business_template, url, username=None, password=None):
......
...@@ -30,15 +30,35 @@ ...@@ -30,15 +30,35 @@
############################################################################## ##############################################################################
import errno, glob, os, re, shutil import errno, glob, os, re, shutil
import threading
from ZTUtils import make_query from ZTUtils import make_query
from Products.ERP5Type.Message import translateString from Products.ERP5Type.Message import translateString
from Products.ERP5Type.Utils import simple_decorator
from Products.ERP5.Document.BusinessTemplate import BusinessTemplateFolder from Products.ERP5.Document.BusinessTemplate import BusinessTemplateFolder
from Products.ERP5VCS.WorkingCopy import \ from Products.ERP5VCS.WorkingCopy import \
WorkingCopy, Dir, File, chdir_working_copy, selfcached, \ WorkingCopy, Dir, File, selfcached, \
NotAWorkingCopyError, NotVersionedError, VcsConflictError NotAWorkingCopyError, NotVersionedError, VcsConflictError
from Products.ERP5VCS.SubversionClient import \ from Products.ERP5VCS.SubversionClient import \
newSubversionClient, SubversionLoginError, SubversionSSLTrustError 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): class Subversion(WorkingCopy):
...@@ -238,6 +258,7 @@ class Subversion(WorkingCopy): ...@@ -238,6 +258,7 @@ class Subversion(WorkingCopy):
'Files committed successfully in revision ${revision}', 'Files committed successfully in revision ${revision}',
mapping=dict(revision=getRevisionNumber(revision)))))) mapping=dict(revision=getRevisionNumber(revision))))))
@chdir_working_copy
def _export(self, business_template): def _export(self, business_template):
bta = BusinessTemplateWorkingCopy(creation=1, client=self._getClient()) bta = BusinessTemplateWorkingCopy(creation=1, client=self._getClient())
bta.export(business_template) bta.export(business_template)
......
...@@ -42,17 +42,6 @@ from ZTUtils import make_query ...@@ -42,17 +42,6 @@ from ZTUtils import make_query
from Products.ERP5.Document.BusinessTemplate import BusinessTemplateFolder from Products.ERP5.Document.BusinessTemplate import BusinessTemplateFolder
from Products.ERP5Type.Utils import simple_decorator 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 @simple_decorator
def selfcached(func): def selfcached(func):
"""Return a function which stores a computed value in an instance """Return a function which stores a computed value in an instance
...@@ -202,15 +191,10 @@ class WorkingCopy(Implicit): ...@@ -202,15 +191,10 @@ class WorkingCopy(Implicit):
business_template.build() business_template.build()
# XXX: Big hack to make export work as expected. # XXX: Big hack to make export work as expected.
transaction.commit() transaction.commit()
old_cwd = os.getcwd() self._export(business_template)
try:
os.chdir(self.working_copy)
self._export(business_template)
finally:
os.chdir(old_cwd)
def _export(self, 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)) self.addremove(*bta.export(business_template))
def showOld(self, path): def showOld(self, path):
...@@ -386,6 +370,7 @@ class BusinessTemplateWorkingCopy(BusinessTemplateFolder): ...@@ -386,6 +370,7 @@ class BusinessTemplateWorkingCopy(BusinessTemplateFolder):
def _writeString(self, obj, path): def _writeString(self, obj, path):
self.file_set.add(path) self.file_set.add(path)
self._makeParent(path) self._makeParent(path)
path = os.path.join(self.path, path)
# write file unless unchanged # write file unless unchanged
file = None file = None
try: try:
...@@ -412,8 +397,9 @@ class BusinessTemplateWorkingCopy(BusinessTemplateFolder): ...@@ -412,8 +397,9 @@ class BusinessTemplateWorkingCopy(BusinessTemplateFolder):
path = os.path.dirname(path) path = os.path.dirname(path)
if path and path not in self.dir_set: if path and path not in self.dir_set:
self._makeParent(path) self._makeParent(path)
if not os.path.exists(path): real_path = os.path.join(self.path, path)
os.mkdir(path) if not os.path.exists(real_path):
os.mkdir(real_path)
self.dir_set.add(path) self.dir_set.add(path)
def export(self, business_template): def export(self, business_template):
...@@ -423,8 +409,8 @@ class BusinessTemplateWorkingCopy(BusinessTemplateFolder): ...@@ -423,8 +409,8 @@ class BusinessTemplateWorkingCopy(BusinessTemplateFolder):
business_template.export(bta=self) business_template.export(bta=self)
# Remove dangling files/dirs # Remove dangling files/dirs
removed_set = set() removed_set = set()
prefix_length = len(os.path.join('.', '')) prefix_length = len(os.path.join(self.path, ''))
for dirpath, dirnames, filenames in os.walk('.'): for dirpath, dirnames, filenames in os.walk(self.path):
dirpath = dirpath[prefix_length:] dirpath = dirpath[prefix_length:]
for i in xrange(len(dirnames) - 1, -1, -1): for i in xrange(len(dirnames) - 1, -1, -1):
d = dirnames[i] d = dirnames[i]
...@@ -432,13 +418,13 @@ class BusinessTemplateWorkingCopy(BusinessTemplateFolder): ...@@ -432,13 +418,13 @@ class BusinessTemplateWorkingCopy(BusinessTemplateFolder):
d = os.path.join(dirpath, d) d = os.path.join(dirpath, d)
if d in self.dir_set: if d in self.dir_set:
continue continue
shutil.rmtree(d) shutil.rmtree(os.path.join(self.path, d))
removed_set.add(d) removed_set.add(d)
del dirnames[i] del dirnames[i]
for f in filenames: for f in filenames:
f = os.path.join(dirpath, f) f = os.path.join(dirpath, f)
if f not in self.file_set: if f not in self.file_set:
os.remove(f) os.remove(os.path.join(self.path, f))
removed_set.add(f) removed_set.add(f)
return self.file_set, removed_set return self.file_set, removed_set
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment