Edgewall Software
Modify

Opened 11 years ago

Last modified 8 years ago

#10423 new defect

Destroy trac.ini at race condition.

Reported by: kawamuramst Owned by:
Priority: normal Milestone: next-major-releases
Component: admin/web Version: 0.12.2
Severity: normal Keywords: trac.ini
Cc: Jun Omae Branch:
Release Notes:
API Changes:
Internal Changes:

Description

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

     
    2424from trac.util.text import printout, to_unicode, CRLF
    2525from trac.util.translation import _, N_
     26import threading
    2627
     28lock=threading.RLock()
     29
    2730__all__ = ['Configuration', 'Option', 'BoolOption', 'IntOption', 'FloatOption',
    2831           'ListOption', 'ChoiceOption', 'PathOption', 'ExtensionOption',
    2932           'OrderedExtensionsOption', 'ConfigurationError']
     
    214217            return
    215218
    216219        # Only save options that differ from the defaults
    217         sections = []
    218         for section in self.sections():
    219             section_str = _to_utf8(section)
    220             options = []
    221             for option in self[section]:
    222                 default_str = None
    223                 for parent in self.parents:
    224                     if parent.has_option(section, option, defaults=False):
    225                         default_str = _to_utf8(parent.get(section, option))
    226                         break
    227                 option_str = _to_utf8(option)
    228                 current_str = False
    229                 if self.parser.has_option(section_str, option_str):
    230                     current_str = self.parser.get(section_str, option_str)
    231                 if current_str is not False and current_str != default_str:
    232                     options.append((option_str, current_str))
    233             if options:
    234                 sections.append((section_str, sorted(options)))
    235 
    236         # At this point, all the strings in `sections` are UTF-8 encoded `str`
    237220        try:
    238             fileobj = AtomicFile(self.filename, 'w')
    239             try:
    240                 fileobj.write('# -*- coding: utf-8 -*-\n\n')
    241                 for section, options in sections:
    242                     fileobj.write('[%s]\n' % section)
    243                     for key_str, val_str in options:
    244                         if to_unicode(key_str) in self[section].overridden:
    245                             fileobj.write('# %s = <inherited>\n' % key_str)
    246                         else:
    247                             val_str = val_str.replace(CRLF, '\n') \
    248                                              .replace('\n', '\n ')
    249                             fileobj.write('%s = %s\n' % (key_str, val_str))
    250                     fileobj.write('\n')
    251             finally:
    252                 fileobj.close()
    253             self._old_sections = deepcopy(self.parser._sections)
    254         except Exception:
    255             # Revert all changes to avoid inconsistencies
    256             self.parser._sections = deepcopy(self._old_sections)
    257             raise
    258 
     221            lock.acquire()
     222            sections = []
     223            for section in self.sections():
     224                section_str = _to_utf8(section)
     225                options = []
     226                for option in self[section]:
     227                    default_str = None
     228                    for parent in self.parents:
     229                        if parent.has_option(section, option, defaults=False):
     230                            default_str = _to_utf8(parent.get(section, option))
     231                            break
     232                    option_str = _to_utf8(option)
     233                    current_str = False
     234                    if self.parser.has_option(section_str, option_str):
     235                        current_str = self.parser.get(section_str, option_str)
     236                    if current_str is not False and current_str != default_str:
     237                        options.append((option_str, current_str))
     238                if options:
     239                    sections.append((section_str, sorted(options)))
     240       
     241           
     242            # At this point, all the strings in `sections` are UTF-8 encoded `str`
     243            try:
     244                fileobj = AtomicFile(self.filename, 'w')
     245                try:
     246                    fileobj.write('# -*- coding: utf-8 -*-\n\n')
     247                    for section, options in sections:
     248                        fileobj.write('[%s]\n' % section)
     249                        for key_str, val_str in options:
     250                            if to_unicode(key_str) in self[section].overridden:
     251                                fileobj.write('# %s = <inherited>\n' % key_str)
     252                            else:
     253                                val_str = val_str.replace(CRLF, '\n') \
     254                                                 .replace('\n', '\n ')
     255                                fileobj.write('%s = %s\n' % (key_str, val_str))
     256                        fileobj.write('\n')
     257                finally:
     258                    fileobj.close()
     259                self._old_sections = deepcopy(self.parser._sections)
     260            except Exception:
     261                # Revert all changes to avoid inconsistencies
     262                self.parser._sections = deepcopy(self._old_sections)
     263                raise
     264        finally:
     265            lock.release()
     266               
    259267    def parse_if_needed(self, force=False):
    260268        if not self.filename or not os.path.isfile(self.filename):
    261269            return False
     
    263271        changed = False
    264272        modtime = os.path.getmtime(self.filename)
    265273        if force or modtime > self._lastmtime:
    266             self._sections = {}
    267             self.parser._sections = {}
    268             self.parser.read(self.filename)
    269             self._lastmtime = modtime
    270             self._old_sections = deepcopy(self.parser._sections)
    271             changed = True
    272        
     274            try:
     275                lock.acquire()
     276                self._sections = {}
     277                self.parser._sections = {}
     278                self.parser.read(self.filename)
     279                self._lastmtime = modtime
     280                self._old_sections = deepcopy(self.parser._sections)
     281                changed = True
     282            finally:
     283                lock.release()
     284               
    273285        if changed:
    274286            self.parents = []
    275287            if self.parser.has_option('inherit', 'file'):

And I confirmed fix at Windows platform Apache and tracd.

Attachments (0)

Change History (5)

comment:1 by Remy Blank, 11 years ago

Keywords: trac.ini added
Milestone: next-major-0.1X

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.

comment:2 by Christian Boos, 11 years ago

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.

comment:3 by Remy Blank, 11 years ago

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).

in reply to:  3 comment:4 by Christian Boos, 11 years ago

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.

comment:5 by Jun Omae, 8 years ago

Cc: Jun Omae added

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
The ticket will be disowned.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from (none) to anonymous. Next status will be 'assigned'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.