Project

General

Profile

Actions

Patch #9484

closed

Limit SCM annotate to text files under the maximum file size for viewing

Added by Antonio García-Domínguez over 12 years ago. Updated over 12 years ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Category:
SCM
Target version:
Start date:
2011-10-28
Due date:
% Done:

0%

Estimated time:

Description

After recently upgrading to Redmine 1.2.1 + patch #4905 and moving from Mongrel 1.1.5 to Passenger 3.0.9 (using Ruby 1.8.7 as packaged in Ubuntu's ruby1.8 1.8.7.249-2 package, for amd64), I started getting rogue processes which used up 100% CPU every now and then. Killing these rogue processes using "kill -SIGABRT pid" produced entries like these:

Processing RepositoriesController#annotate (for 10.182.221.1 at 2011-10-28 01:51:08) [GET]
  Parameters: {"action"=>"annotate", "id"=>"sources-fm", "path"=>["trunk", "xmleye-1.2.3_svn473-bpelunit_results.tar.gz"], "controller"=>"repositories"}
Rendering template within layouts/base
Rendering repositories/annotate

ActionView::TemplateError (SIGTERM) on line #17 of app/views/repositories/annotate.rhtml:
14: <table class="filecontent annotate syntaxhl">
15:   <tbody>
16:     <% line_num = 1 %>
17:     <% syntax_highlight(@path, to_utf8(@annotate.content)).each_line do |line| %>
18:     <% revision = @annotate.revisions[line_num-1] %>
19:     <tr class="bloc-<%= revision.nil? ? 0 : colors[revision.identifier || revision.revision] %>">
20:       <th class="line-num" id="L<%= line_num %>"><a href="#L<%= line_num %>"><%= line_num %></a></th>

    lib/redmine/codeset_util.rb:21:in `replace_invalid_utf8'
    app/helpers/repositories_helper.rb:146:in `to_utf8_internal'
    app/helpers/repositories_helper.rb:121:in `to_utf8'
    app/views/repositories/annotate.rhtml:17:in `_run_rhtml_app47views47repositories47annotate46rhtml'
    passenger (3.0.9) lib/phusion_passenger/rack/request_handler.rb:96:in `process_request'
    passenger (3.0.9) lib/phusion_passenger/abstract_request_handler.rb:513:in `accept_and_process_next_request'
    passenger (3.0.9) lib/phusion_passenger/abstract_request_handler.rb:274:in `main_loop'
    passenger (3.0.9) lib/phusion_passenger/classic_rails/application_spawner.rb:321:in `start_request_handler'
    passenger (3.0.9) lib/phusion_passenger/classic_rails/application_spawner.rb:275:in `send'
    passenger (3.0.9) lib/phusion_passenger/classic_rails/application_spawner.rb:275:in `handle_spawn_application'
    passenger (3.0.9) lib/phusion_passenger/utils.rb:479:in `safe_fork'
    passenger (3.0.9) lib/phusion_passenger/classic_rails/application_spawner.rb:270:in `handle_spawn_application'
    passenger (3.0.9) lib/phusion_passenger/abstract_server.rb:357:in `__send__'
    passenger (3.0.9) lib/phusion_passenger/abstract_server.rb:357:in `server_main_loop'
    passenger (3.0.9) lib/phusion_passenger/abstract_server.rb:206:in `start_synchronously'
    passenger (3.0.9) lib/phusion_passenger/abstract_server.rb:180:in `start'
    passenger (3.0.9) lib/phusion_passenger/classic_rails/application_spawner.rb:149:in `start'
    passenger (3.0.9) lib/phusion_passenger/spawn_manager.rb:219:in `spawn_rails_application'
    passenger (3.0.9) lib/phusion_passenger/abstract_server_collection.rb:132:in `lookup_or_add'
    passenger (3.0.9) lib/phusion_passenger/spawn_manager.rb:214:in `spawn_rails_application'
    passenger (3.0.9) lib/phusion_passenger/abstract_server_collection.rb:82:in `synchronize'
    passenger (3.0.9) lib/phusion_passenger/abstract_server_collection.rb:79:in `synchronize'
    passenger (3.0.9) lib/phusion_passenger/spawn_manager.rb:213:in `spawn_rails_application'
    passenger (3.0.9) lib/phusion_passenger/spawn_manager.rb:132:in `spawn_application'
    passenger (3.0.9) lib/phusion_passenger/spawn_manager.rb:275:in `handle_spawn_application'
    passenger (3.0.9) lib/phusion_passenger/abstract_server.rb:357:in `__send__'
    passenger (3.0.9) lib/phusion_passenger/abstract_server.rb:357:in `server_main_loop'
    passenger (3.0.9) lib/phusion_passenger/abstract_server.rb:206:in `start_synchronously'
    passenger (3.0.9) helper-scripts/passenger-spawn-server:99

As you can see, it was trying to show the output of calling 'svn annotate' on a medium (~18MB) tar.gz file, while trying to interpret its contents as UTF-8 text.

We already have a setting for limiting the maximum size of the text files which can be viewed (instead of forcing the download) using 'View'. I think we should use the same limit for annotating the files, and also prevent Redmine from annotating binary files (using the same heuristics as in 'View').

I have attached a patch which modifies 'annotate' so it checks that the file is really a text file under the specified size limit before trying to annotate it. Otherwise, it displays a new error message explaining why it could not be annotated.


Files

0001-Fix-for-expensive-annotations.patch (4.1 KB) 0001-Fix-for-expensive-annotations.patch Patch which limits 'annotate' to text files under the global size limit for viewing files directly Antonio García-Domínguez, 2011-10-28 02:07
revised-9484.patch (4.79 KB) revised-9484.patch Antonio García-Domínguez, 2011-10-30 16:47
Actions #1

Updated by Toshi MARUYAMA over 12 years ago

Actions #2

Updated by Antonio García-Domínguez over 12 years ago

Toshi MARUYAMA wrote:

Great work!
Could you add a test such as source:tags/1.2.1/test/functional/repositories_filesystem_controller_test.rb#L115 ?

I'm glad you liked the patch :-). Sorry, I forgot to watch the issue and didn't see your message until now. I'll write it tomorrow: it's midnight right now in Spain. Basically, I just need to test that the proper error is shown when trying to annotate a file that is too big, right?

Actions #4

Updated by Antonio García-Domínguez over 12 years ago

I have revised the patch with a new test case for the Subversion test suite. I had to change the new code a bit as well, as it was breaking one of the functional tests for Git repositories. It seems that with the old patch, a different error message would have been shown when trying to annotate binary files.

I have tried this one with 'rake test RAILS_ENV=test' using a SQLite database: all the tests seem to pass. I haven't run the tests that require an LDAP server, though.

Actions #5

Updated by Toshi MARUYAMA over 12 years ago

  • Status changed from New to Closed
  • Target version set to 1.3.0

Committed in trunk r7728 and r7729, thanks.

Actions

Also available in: Atom PDF