commit 8d5a2fd10829a580699eac13c06a7dbb8516aeaf Author: Philipp Hahn Date: Wed Nov 14 17:32:54 2012 +0100 Bug #15723: Fix locking and data corruption issue in UCR When updating UCR keep the file opened in read-write mode. Lock the file itself instead of using a separate lock file. Be more careful when re-writing the file from scratch. Remove now unused .lock() and .unlock() functions. Fix callers to use handler_set() instead of calling save(). diff --git a/branches/ucs-3.1/ucs/base/univention-config-registry/debian/changelog b/branches/ucs-3.1/ucs/base/univention-config-registry/debian/changelog index 5b7ddf6..703f036 100644 --- a/branches/ucs-3.1/ucs/base/univention-config-registry/debian/changelog +++ b/branches/ucs-3.1/ucs/base/univention-config-registry/debian/changelog @@ -1,3 +1,9 @@ +univention-config-registry (8.0.4-10) unstable; urgency=low + + * Fix locking and data corruption issue in UCR (Bug #15723) + + -- Philipp Hahn Wed, 14 Nov 2012 17:32:42 +0100 + univention-config-registry (8.0.4-9) unstable; urgency=low * Fix ucr unregister for Multifile (Bug #26476) diff --git a/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/backend.py b/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/backend.py index af8f2a8..8cfcc4c 100644 --- a/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/backend.py +++ b/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/backend.py @@ -94,11 +94,11 @@ class ConfigRegistry(dict): ConfigRegistry.BASES[reg]) return _ConfigRegistry(filename=filename) - def load(self): + def load(self, writable=False): """Load registry from file.""" for reg in self._registry: if isinstance(reg, _ConfigRegistry): - reg.load() + reg.load(writable and reg is self._write_registry) strict = self.is_true('ucr/encoding/strict') for reg in self._registry: if isinstance(reg, _ConfigRegistry): @@ -108,14 +108,6 @@ class ConfigRegistry(dict): """Save registry to file.""" self._write_registry.save() - def lock(self): - """Lock registry file.""" - self._write_registry.lock() - - def unlock(self): - """Un-lock registry file.""" - self._write_registry.unlock() - def __delitem__(self, key): """Delete registry key.""" del self._write_registry[key] @@ -241,12 +233,11 @@ class _ConfigRegistry(dict): self.file = filename or '/etc/univention/base.conf' self.__create_base_conf() self.backup_file = self.file + '.bak' - self.lock_filename = self.file + '.lock' # will be set by for each <_ConfigRegistry> - # means the backend files are valid UTF-8 and should stay that way --> # only accept valid UTF-8 self.strict_encoding = False - self.lock_file = None + self.reg_file = None def __create_base_conf(self): """Create sub registry file.""" @@ -262,57 +253,72 @@ class _ConfigRegistry(dict): def __load_file(self, reg_file): """Load sub registry from opened file.""" self.clear() - for line in reg_file.readlines(): + nr = -1 + for nr, line in enumerate(reg_file): line = re.sub(r'^[^:]*#.*$', "", line) - if line == '': + if not line: continue - if line.find(': ') == -1: + try: + key, value = line.split(': ', 1) + except ValueError: continue + else: + self[key] = value.strip() + return nr >= 2 - key, value = line.split(': ', 1) - value = value.strip() - if len(value) == 0: # if variable was set without an value - value = '' - - self[key] = value - - def load(self): + def load(self, writable=False): """Load sub registry from file.""" - import_failed = False try: - reg_file = open(self.file, 'r') + if writable: + self.reg_file = reg_file = os.fdopen(os.open(self.file, os.O_RDWR | os.O_CREAT, 0644), 'w') + fcntl.flock(self.reg_file.fileno(), fcntl.LOCK_EX) + if self.__load_file(reg_file): + return + else: + reg_file = open(self.file, 'r') + try: + fcntl.flock(reg_file.fileno(), fcntl.LOCK_SH) + if self.__load_file(reg_file): + return + finally: + reg_file.close() except EnvironmentError: - import_failed = True - else: - if len(reg_file.readlines()) < 3: # comment or nothing - import_failed = True - - if import_failed: - try: - reg_file = open(self.backup_file, 'r') - except EnvironmentError: - return + pass - reg_file.seek(0) - self.__load_file(reg_file) - reg_file.close() + # open or loading failed, try to load backup + try: + bak_file = open(self.backup_file, 'r') + fcntl.flock(bak_file.fileno(), fcntl.LOCK_SH) + self.__load_file(bak_file) + bak_file.close() + except EnvironmentError: + return - if import_failed: - self.__save_file(self.file) + # Try to write main file + try: + if writable: + self.__save_file(self.reg_file) + else: + reg_file = os.fdopen(os.open(self.file, os.O_RDWR | os.O_CREAT, 0644), 'w') + try: + fcntl.flock(reg_file.fileno(), fcntl.LOCK_EX) + self.__save_file(reg_file) + finally: + reg_file.close() + except EnvironmentError: + pass - def __save_file(self, filename): + def __save_file(self, reg_file): """Save sub registry to file.""" try: - # open temporary file for writing - reg_file = open(filename, 'w') + reg_file.seek(0) # write data to file reg_file.write('# univention_ base.conf\n\n') reg_file.write(self.__str__()) # flush (meta)data reg_file.flush() + reg_file.truncate() os.fsync(reg_file.fileno()) - # close fd - reg_file.close() except EnvironmentError, ex: # suppress certain errors if ex.errno != errno.EACCES: @@ -320,17 +326,24 @@ class _ConfigRegistry(dict): def save(self): """Save sub registry to file.""" - for filename in (self.backup_file, self.file): - self.__save_file(filename) - - def lock(self): - """Lock sub registry file.""" - self.lock_file = open(self.lock_filename, "a+") - fcntl.flock(self.lock_file.fileno(), fcntl.LOCK_EX) + assert self.reg_file + try: + bak_file = os.fdopen(os.open(self.backup_file, os.O_RDWR | os.O_CREAT, 0644), 'w') + try: + fcntl.flock(bak_file.fileno(), fcntl.LOCK_EX) + self.__save_file(bak_file) + finally: + bak_file.close() - def unlock(self): - """Un-lock sub registry file.""" - self.lock_file.close() + try: + self.__save_file(self.reg_file) + finally: + self.reg_file.close() + self.reg_file = None + except EnvironmentError, ex: + # suppress certain errors + if ex.errno != errno.EACCES: + raise def __getitem__(self, key): """Return value from sub registry.""" diff --git a/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/frontend.py b/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/frontend.py index 9fd6dd0..e268557 100644 --- a/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/frontend.py +++ b/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/frontend.py @@ -136,9 +136,8 @@ def handler_set(args, opts=dict(), quiet=False): else: reg = ConfigRegistry() - reg.lock() + reg.load(writable=True) try: - reg.load() changed = {} for arg in args: sep_set = arg.find('=') # set @@ -178,9 +177,8 @@ def handler_set(args, opts=dict(), quiet=False): print 'Not updating %s' % key else: print 'Not setting %s' % key - reg.save() finally: - reg.unlock() + reg.save() handlers(changed.keys(), (reg, changed)) @@ -205,9 +203,8 @@ def handler_unset(args, opts=dict()): else: reg = ConfigRegistry() - reg.lock() + reg.load(writable=True) try: - reg.load() changed = {} for arg in args: if reg.has_key(arg, write_registry_only=True): @@ -224,9 +221,8 @@ def handler_unset(args, opts=dict()): else: msg = "W: The config registry variable '%s' does not exist" print >> sys.stderr, msg % (arg,) - reg.save() finally: - reg.unlock() + reg.save() handlers(changed.keys(), (reg, changed)) diff --git a/branches/ucs-3.1/ucs/base/univention-ssl/debian/changelog b/branches/ucs-3.1/ucs/base/univention-ssl/debian/changelog index 17b5d73..919f43a 100644 --- a/branches/ucs-3.1/ucs/base/univention-ssl/debian/changelog +++ b/branches/ucs-3.1/ucs/base/univention-ssl/debian/changelog @@ -1,3 +1,9 @@ +univention-ssl (7.0.5-4) unstable; urgency=low + + * Fix locking and data corruption issue in UCR (Bug #15723) + + -- Philipp Hahn Wed, 14 Nov 2012 17:32:11 +0100 + univention-ssl (7.0.5-3) unstable; urgency=low * Fix errexit (Bug #26572) diff --git a/branches/ucs-3.1/ucs/base/univention-ssl/univention-certificate-check-validity b/branches/ucs-3.1/ucs/base/univention-ssl/univention-certificate-check-validity index d36efe9..b1f1669 100755 --- a/branches/ucs-3.1/ucs/base/univention-ssl/univention-certificate-check-validity +++ b/branches/ucs-3.1/ucs/base/univention-ssl/univention-certificate-check-validity @@ -37,7 +37,7 @@ import calendar from M2Crypto import X509 -from univention.config_registry import ConfigRegistry +from univention.config_registry import ConfigRegistry, handler_set _bc = ConfigRegistry() _bc.load() @@ -68,10 +68,11 @@ if __name__ == '__main__': days = get_validity_days('/etc/univention/ssl/%s/cert.pem' % fqdn) days_ca = get_validity_days('/etc/univention/ssl/ucsCA/CAcert.pem') + changes = [] if days and days != _bc.get( 'ssl/validity/host', -1 ): - _bc[ 'ssl/validity/host' ] = str( days ) - _bc.save() + changes.append('ssl/validity/host=%d' % (days,)) if days_ca and days_ca != _bc.get( 'ssl/validity/root', -1 ): - _bc[ 'ssl/validity/root' ] = str( days_ca ) - _bc.save() + changes.append('ssl/validity/root=%d' % (days_cs,)) + if changes: + handler_set(changes) diff --git a/branches/ucs-3.1/ucs/base/univention-system-setup/debian/changelog b/branches/ucs-3.1/ucs/base/univention-system-setup/debian/changelog index 9a7de83..c4c30d7 100644 --- a/branches/ucs-3.1/ucs/base/univention-system-setup/debian/changelog +++ b/branches/ucs-3.1/ucs/base/univention-system-setup/debian/changelog @@ -1,5 +1,12 @@ +univention-system-setup (6.0.51-1) unstable; urgency=low + + * Fix locking and data corruption issue in UCR (Bug #15723) + + -- Philipp Hahn Wed, 14 Nov 2012 17:32:26 +0100 + univention-system-setup (6.0.50-1) unstable; urgency=low + * Revert changes in NetworkPage; Bug #28389 -- Dirk Wiesenthal Mon, 12 Nov 2012 19:52:36 +0100 diff --git a/branches/ucs-3.1/ucs/base/univention-system-setup/umc/python/setup/setup_script.py b/branches/ucs-3.1/ucs/base/univention-system-setup/umc/python/setup/setup_script.py index 53c1943..e3bd88d 100644 --- a/branches/ucs-3.1/ucs/base/univention-system-setup/umc/python/setup/setup_script.py +++ b/branches/ucs-3.1/ucs/base/univention-system-setup/umc/python/setup/setup_script.py @@ -44,9 +44,9 @@ translation.set_language(default_locale[0]) _ = translation.translate locale.setlocale(locale.LC_ALL, default_locale) # needed for external translation (e.g. apt) -import univention.config_registry +from univention.config_registry import ConfigRegistry, handler_set from util import PATH_SETUP_SCRIPTS, PATH_PROFILE -ucr = univention.config_registry.ConfigRegistry() +ucr = ConfigRegistry() class SetupScript(object): '''Baseclass for all Python-based Setup-Scripts. @@ -193,7 +193,6 @@ class SetupScript(object): # nothing to do return self._ucr_changes[var_name] = (oldval, value) - ucr[var_name] = value def commit_ucr(self): '''Saves ucr variables previously @@ -203,15 +202,11 @@ class SetupScript(object): not raise an exception*. You can call it manually if you need to do it (e.g. in down())''' - ucr.save() - ucr.load() if self._ucr_changes: - handler = univention.config_registry.configHandlers() - handler.load() - handler(self._ucr_changes.keys(), (ucr, self._ucr_changes)) - # reset (in case it is called multiple - # times in a script - self._ucr_changes = {} + handler_set(['%s=%s' % (var_name, value) for var_name, (_, value) in self._ucr_changes.items()]) + # reset (in case it is called multiple times in a script) + self._ucr_changes.clear() + ucr.load() def get_ucr_var(self, var_name): '''Retrieve the value of var_name from ucr''' commit 2cee22eea737f38155e34b139b7f4a7a1876f4e1 Author: Philipp Hahn Date: Wed Nov 14 17:18:32 2012 +0100 Bug #15723: Shuffle UCR code Move some code in preparation to fix locking issue. diff --git a/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/backend.py b/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/backend.py index c264060..af8f2a8 100644 --- a/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/backend.py +++ b/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/backend.py @@ -248,9 +248,36 @@ class _ConfigRegistry(dict): self.strict_encoding = False self.lock_file = None + def __create_base_conf(self): + """Create sub registry file.""" + if not os.path.exists(self.file): + try: + reg_file = os.open(self.file, os.O_CREAT | os.O_RDONLY, 0644) + os.close(reg_file) + except EnvironmentError: + msg = "E: file '%s' does not exist and could not be created" + print >> sys.stderr, msg % (self.file,) + exception_occured() + + def __load_file(self, reg_file): + """Load sub registry from opened file.""" + self.clear() + for line in reg_file.readlines(): + line = re.sub(r'^[^:]*#.*$', "", line) + if line == '': + continue + if line.find(': ') == -1: + continue + + key, value = line.split(': ', 1) + value = value.strip() + if len(value) == 0: # if variable was set without an value + value = '' + + self[key] = value + def load(self): """Load sub registry from file.""" - self.clear() import_failed = False try: reg_file = open(self.file, 'r') @@ -267,35 +294,12 @@ class _ConfigRegistry(dict): return reg_file.seek(0) - for line in reg_file.readlines(): - line = re.sub(r'^[^:]*#.*$', "", line) - if line == '': - continue - if line.find(': ') == -1: - continue - - key, value = line.split(': ', 1) - value = value.strip() - if len(value) == 0: # if variable was set without an value - value = '' - - self[key] = value + self.__load_file(reg_file) reg_file.close() if import_failed: self.__save_file(self.file) - def __create_base_conf(self): - """Create sub registry file.""" - if not os.path.exists(self.file): - try: - reg_file = os.open(self.file, os.O_CREAT | os.O_RDONLY, 0644) - os.close(reg_file) - except EnvironmentError: - msg = "E: file '%s' does not exist and could not be created" - print >> sys.stderr, msg % (self.file,) - exception_occured() - def __save_file(self, filename): """Save sub registry to file.""" try: diff --git a/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/frontend.py b/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/frontend.py index d4df5bf..9fd6dd0 100644 --- a/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/frontend.py +++ b/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/frontend.py @@ -139,7 +139,6 @@ def handler_set(args, opts=dict(), quiet=False): reg.lock() try: reg.load() - changed = {} for arg in args: sep_set = arg.find('=') # set @@ -179,11 +178,9 @@ def handler_set(args, opts=dict(), quiet=False): print 'Not updating %s' % key else: print 'Not setting %s' % key - reg.save() finally: reg.unlock() - handlers(changed.keys(), (reg, changed)) @@ -191,6 +188,9 @@ def handler_unset(args, opts=dict()): """ Unset config registry variables in args. """ + handlers = ConfigHandlers() + handlers.load() + current_scope = ConfigRegistry.NORMAL reg = None if opts.get('ldap-policy', False): @@ -204,13 +204,10 @@ def handler_unset(args, opts=dict()): reg = ConfigRegistry(write_registry=current_scope) else: reg = ConfigRegistry() + reg.lock() try: reg.load() - - handlers = ConfigHandlers() - handlers.load() - changed = {} for arg in args: if reg.has_key(arg, write_registry_only=True): commit 1d503d12babeaf307bf2dd5983a92f2ef8bd8c56 Author: Philipp Hahn Date: Wed Nov 14 15:41:39 2012 +0100 Bug #15723: direct _write_registry usage Make _write_registry a direct reference to the writeable ConfigRegistry instead of of being an index info _registry[] diff --git a/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/backend.py b/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/backend.py index 3bebeeb..c264060 100644 --- a/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/backend.py +++ b/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/backend.py @@ -75,10 +75,6 @@ class ConfigRegistry(dict): def __init__(self, filename=None, write_registry=NORMAL): dict.__init__(self) self.file = os.getenv('UNIVENTION_BASECONF') or filename or None - if self.file: - self._write_registry = ConfigRegistry.CUSTOM - else: - self._write_registry = write_registry self._registry = [None] * ConfigRegistry.MAX for reg in range(ConfigRegistry.MAX): if self.file and reg != ConfigRegistry.CUSTOM: @@ -87,6 +83,7 @@ class ConfigRegistry(dict): self._registry[reg] = {} else: self._registry[reg] = self._create_registry(reg) + self._write_registry = self._registry[ConfigRegistry.CUSTOM if self.file else write_registry] def _create_registry(self, reg): """Create internal sub registry.""" @@ -109,23 +106,19 @@ class ConfigRegistry(dict): def save(self): """Save registry to file.""" - registry = self._registry[self._write_registry] - registry.save() + self._write_registry.save() def lock(self): """Lock registry file.""" - registry = self._registry[self._write_registry] - registry.lock() + self._write_registry.lock() def unlock(self): """Un-lock registry file.""" - registry = self._registry[self._write_registry] - registry.unlock() + self._write_registry.unlock() def __delitem__(self, key): """Delete registry key.""" - registry = self._registry[self._write_registry] - del registry[key] + del self._write_registry[key] def __getitem__(self, key): """Return registry value.""" @@ -133,8 +126,7 @@ class ConfigRegistry(dict): def __setitem__(self, key, value): """Set registry value.""" - registry = self._registry[self._write_registry] - registry[key] = value + self._write_registry[key] = value def __contains__(self, key): """Check if registry key is set.""" @@ -181,8 +173,7 @@ class ConfigRegistry(dict): def has_key(self, key, write_registry_only=False): """Check if registry key is set (DEPRECATED).""" if write_registry_only: - registry = self._registry[self._write_registry] - return key in registry + return key in self._write_registry else: return key in self commit 295f7720c610e3efd4ad1c83e290b20a3dbec071 Author: Philipp Hahn Date: Wed Nov 14 15:38:59 2012 +0100 Bug #15723: Convert dict to array _registry is only indexed by number, so use more efficient array. diff --git a/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/backend.py b/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/backend.py index 1a66966..3bebeeb 100644 --- a/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/backend.py +++ b/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/backend.py @@ -79,7 +79,7 @@ class ConfigRegistry(dict): self._write_registry = ConfigRegistry.CUSTOM else: self._write_registry = write_registry - self._registry = {} + self._registry = [None] * ConfigRegistry.MAX for reg in range(ConfigRegistry.MAX): if self.file and reg != ConfigRegistry.CUSTOM: self._registry[reg] = {} @@ -99,11 +99,11 @@ class ConfigRegistry(dict): def load(self): """Load registry from file.""" - for reg in self._registry.values(): + for reg in self._registry: if isinstance(reg, _ConfigRegistry): reg.load() strict = self.is_true('ucr/encoding/strict') - for reg in self._registry.values(): + for reg in self._registry: if isinstance(reg, _ConfigRegistry): reg.strict_encoding = strict commit b4933393f953ff3b97cb638aaaa80c4443ad2466 Author: Philipp Hahn Date: Wed Nov 14 17:04:10 2012 +0100 Bug #15723: Fix file(name) variable file() is a Python build-in, so the test always evaluates to True. Use right "filename" instead. diff --git a/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/backend.py b/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/backend.py index 35a4ffd..1a66966 100644 --- a/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/backend.py +++ b/branches/ucs-3.1/ucs/base/univention-config-registry/python/univention/config_registry/backend.py @@ -247,10 +247,7 @@ class _ConfigRegistry(dict): """ def __init__(self, filename=None): dict.__init__(self) - if file: - self.file = filename - else: - self.file = '/etc/univention/base.conf' + self.file = filename or '/etc/univention/base.conf' self.__create_base_conf() self.backup_file = self.file + '.bak' self.lock_filename = self.file + '.lock'