Edgewall Software
Modify ↓

Opened 10 years ago

Closed 7 years ago

Last modified 7 years ago

#11846 closed enhancement (fixed)

Prevent invalid file in Environment plugins directory from disabling Trac instance

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.2.3
Component: general Version:
Severity: normal Keywords: plugins
Cc: Branch:
Release Notes:

A module in the plugins directory that raises SystemExit exception will no longer disable the Trac instance.

API Changes:
Internal Changes:

Description (last modified by Jun Omae)

imp.load_source can raise a SystemExit exception when there is an invalid file in the Environment's plugins directory. A common scenario seems to be that the user will select setup.py with the plugin installation file picker on the Plugins admin panel.

The patch might be as simple as (needs more testing):

  • trac/loader.py 

    diff --git a/trac/loader.py b/trac/loader.py
index df65356..444e78d 100644
    a b def load_py_files():  
    8787                    env.log.debug('Loading file plugin %s from %s',
    8888                                  plugin_name, plugin_file)
    8989                    if plugin_name not in sys.modules:
    90                         module = imp.load_source(plugin_name, plugin_file)
     90                        try:
     91                            imp.load_source(plugin_name, plugin_file)
     92                        except SystemExit, e:
     93                            env.log.warn("Can't load file plugin %s: %s",
     94                                         plugin_file, exception_to_unicode(e))
    9195                    if path == auto_enable:
    9296                        _enable_plugin(env, plugin_name)
    9397                except Exception, e:

The issue has come up a few times recently:

Attachments (0)

Change History (12)

comment:1 by Ryan J Ollos, 10 years ago

Reporter: changed from anonymous to Ryan J Ollos

comment:2 by Ryan J Ollos, 10 years ago

Here's a relevant TODO tags/trac-1.0.2/trac/admin/web_ui.py@:500#L466​. Validating the upload sounds like a good idea in addition to protecting against an invalid file in the environment plugins directory.

comment:3 by Ryan J Ollos, 10 years ago

Another report of this issue in ​SO:27905150.

comment:4 by Ryan J Ollos, 9 years ago

Milestone: next-stable-1.0.x → next-major-releases

comment:5 by Ryan J Ollos, 9 years ago

Description: modified (diff)

comment:6 by Jun Omae, 9 years ago

Good idea. Another idea in plugins is to invoke setup() only when run as a script, e.g. ​th:source:tracdragdropplugin/0.12/setup.py@14282.

comment:7 by Ryan J Ollos, 9 years ago

Description: modified (diff)

comment:8 by Jun Omae, 8 years ago

Description: modified (diff)

comment:9 by Ryan J Ollos, 7 years ago

Maybe component_guard (#12680) can help here?

comment:10 by Ryan J Ollos, 7 years ago

Milestone: next-major-releases → 1.2.3
Owner: set to Ryan J Ollos
Status: new → assigned

SystemExit is not currently trapped because it's not derived from Exception, see ​exception hierarchy. Therefore, I propose to commit the following simple change:

  • trac/loader.py 

    diff --git a/trac/loader.py b/trac/loader.py
index 5fe0e3cec..7608031c6 100644
    a b def load_py_files():  
    9292                except (ImportError, VersionConflict) as e:
    9393                    env.log.error('Skipping "%s": %s', plugin_name,
    9494                                  exception_to_unicode(e))
    95                 except Exception as e:
     95                except (Exception, SystemExit) as e:
    9696                    env.log.error(
    9797                        "Failed to load plugin from %s: %s", plugin_file,
    9898                        exception_to_unicode(e, traceback=True))

comment:11 by Jun Omae, 7 years ago

The change looks good to me.

comment:12 by Ryan J Ollos, 7 years ago

Release Notes: modified (diff)
Resolution: → fixed
Status: assigned → closed

Thanks for review. Committed to 1.2-stable in r16165, merged to trunk in r16166.

Last edited 7 years ago by Ryan J Ollos (previous) (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos to the specified user.

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