Destroy trac.ini at race condition.

[kawamuramst]

May be destroy trac.ini when several users change trac.ini from Trac Admin page.

When change trac.ini, config module is executed save() method and parse_if_needed method in order. However, If several users change trac.ini, Happen race condition between save() method and parse_if_needed method.

A) self._section and self.parser_sectios is cleared by if_parse_needed method, B) save() method read into "section" (local value of save() method) from self.parser._sections(into self.sections())

Unable to check process that search the change option value when A and B execute at the same time. As a result, destroyed trac.ini by save() method.

I have fix this bug using RLock object of threading module.

trac/config.py

@@ -24 +24 @@
 from trac.util.text import printout, to_unicode, CRLF
 from trac.util.translation import _, N_
+import threading
 
+lock=threading.RLock()
+
 __all__ = ['Configuration', 'Option', 'BoolOption', 'IntOption', 'FloatOption',
            'ListOption', 'ChoiceOption', 'PathOption', 'ExtensionOption',
            'OrderedExtensionsOption', 'ConfigurationError']
@@ -214 +217 @@
             return
 
         # Only save options that differ from the defaults
-        sections = []
-        for section in self.sections():
-            section_str = _to_utf8(section)
-            options = []
-            for option in self[section]:
-                default_str = None
-                for parent in self.parents:
-                    if parent.has_option(section, option, defaults=False):
-                        default_str = _to_utf8(parent.get(section, option))
-                        break
-                option_str = _to_utf8(option)
-                current_str = False
-                if self.parser.has_option(section_str, option_str):
-                    current_str = self.parser.get(section_str, option_str)
-                if current_str is not False and current_str != default_str:
-                    options.append((option_str, current_str))
-            if options:
-                sections.append((section_str, sorted(options)))
-
-        # At this point, all the strings in `sections` are UTF-8 encoded `str`
         try:
-            fileobj = AtomicFile(self.filename, 'w')
-            try:
-                fileobj.write('# -*- coding: utf-8 -*-\n\n')
-                for section, options in sections:
-                    fileobj.write('[%s]\n' % section)
-                    for key_str, val_str in options:
-                        if to_unicode(key_str) in self[section].overridden:
-                            fileobj.write('# %s = <inherited>\n' % key_str)
-                        else:
-                            val_str = val_str.replace(CRLF, '\n') \
-                                             .replace('\n', '\n ')
-                            fileobj.write('%s = %s\n' % (key_str, val_str))
-                    fileobj.write('\n')
-            finally:
-                fileobj.close()
-            self._old_sections = deepcopy(self.parser._sections)
-        except Exception:
-            # Revert all changes to avoid inconsistencies
-            self.parser._sections = deepcopy(self._old_sections)
-            raise
-
+            lock.acquire()
+            sections = []
+            for section in self.sections():
+                section_str = _to_utf8(section)
+                options = []
+                for option in self[section]:
+                    default_str = None
+                    for parent in self.parents:
+                        if parent.has_option(section, option, defaults=False):
+                            default_str = _to_utf8(parent.get(section, option))
+                            break
+                    option_str = _to_utf8(option)
+                    current_str = False
+                    if self.parser.has_option(section_str, option_str):
+                        current_str = self.parser.get(section_str, option_str)
+                    if current_str is not False and current_str != default_str:
+                        options.append((option_str, current_str))
+                if options:
+                    sections.append((section_str, sorted(options)))
+        
+            
+            # At this point, all the strings in `sections` are UTF-8 encoded `str`
+            try:
+                fileobj = AtomicFile(self.filename, 'w')
+                try:
+                    fileobj.write('# -*- coding: utf-8 -*-\n\n')
+                    for section, options in sections:
+                        fileobj.write('[%s]\n' % section)
+                        for key_str, val_str in options:
+                            if to_unicode(key_str) in self[section].overridden:
+                                fileobj.write('# %s = <inherited>\n' % key_str)
+                            else:
+                                val_str = val_str.replace(CRLF, '\n') \
+                                                 .replace('\n', '\n ')
+                                fileobj.write('%s = %s\n' % (key_str, val_str))
+                        fileobj.write('\n')
+                finally:
+                    fileobj.close()
+                self._old_sections = deepcopy(self.parser._sections)
+            except Exception:
+                # Revert all changes to avoid inconsistencies
+                self.parser._sections = deepcopy(self._old_sections)
+                raise
+        finally:
+            lock.release()
+                
     def parse_if_needed(self, force=False):
         if not self.filename or not os.path.isfile(self.filename):
             return False
@@ -263 +271 @@
         changed = False
         modtime = os.path.getmtime(self.filename)
         if force or modtime > self._lastmtime:
-            self._sections = {}
-            self.parser._sections = {}
-            self.parser.read(self.filename)
-            self._lastmtime = modtime
-            self._old_sections = deepcopy(self.parser._sections)
-            changed = True
-        
+            try:
+                lock.acquire()
+                self._sections = {}
+                self.parser._sections = {}
+                self.parser.read(self.filename)
+                self._lastmtime = modtime
+                self._old_sections = deepcopy(self.parser._sections)
+                changed = True
+            finally:
+                lock.release()
+                
         if changed:
             self.parents = []
             if self.parser.has_option('inherit', 'file'):

And I confirmed fix at Windows platform Apache and tracd.

[Remy Blank]

Unfortunately, it's more complicated than that. It's a race condition between several processes, not only threads. So a treading lock won't help there. Your solution may work for you because you are running Trac using threading and a single process, but it won't work in the general case.

What is needed is a global lock, like a lock on a file. But that's notoriously difficult to implement portably (and even on a single platform, it depends on the filesystem). And even then, it's non-trivial. We currently modify an in-memory copy of trac.ini, then save the result. This would have to be changed into an atomic read-modify-write operation inside a locked section. Doable, but not easy.

So up to now, we have simply assumed that changes to trac.ini are infrequent (and often done by a single person at at time), and therefore the probability of a collision is low. This is probably true for many installation, but certainly not all.

[Christian Boos]

I'm not sure I have the same analysis. The race between processes comes maybe in addition to the problem raised here, but self._sections and related are really data structures shared among different threads, and should therefore be protected from concurrent access. Even the simple read operations should take care that at least the parsing is finished but of course that's most critical when we're collecting all the values and are about to write them back on disk.

As for the multi-process concurrency issue, we use AtomicFile which according to its contract ("A file that appears atomically with its full content") should prevent inconsistencies at that level.

So I think the problem is more that the race between threads makes it possible to write an inconsistent snapshot (atomically) on disk.

[Remy Blank]

There are indeed two races. The multi-process race comes into play when two processes want to change a different option at the same time. Both will change their in-memory copy, then write it atomically to disk. The last one to write will win, and overwrite the change from the first. The file will be consistent, but a change will have been lost.

Note that if we solve the multi-process race with an atomic read-modify-write, we can easily solve the inter-thread race by updating the data structure atomically in memory (i.e. updating a single reference to the structure).

[Christian Boos]

Replying to rblank:

Note that if we solve the multi-process race with an atomic read-modify-write, we can easily solve the inter-thread race by updating the data structure atomically in memory

Ah yes, great idea! I think we should then put the pending changes introduced by set() in thread-local storage.