プロジェクト

全般

プロフィール

Vote #75046

完了

Regexp bug in JournalsController regexp handling when quoting existing journal entries

Admin Redmine さんが約4年前に追加. 約4年前に更新.

ステータス:
Closed
優先度:
通常
担当者:
-
カテゴリ:
Issues_2
対象バージョン:
開始日:
2022/05/09
期日:
進捗率:

0%

予定工数:
category_id:
2
version_id:
82
issue_org_id:
16353
author_id:
87711
assigned_to_id:
1
comments:
4
status_id:
5
tracker_id:
1
plus1:
0
affected_version:
closed_on:
affected_version_id:
71
ステータス-->[Closed]

説明

I have stumbled upon a very nasty bug with journal entries quoting, when a user writes an entry with a "pre" tag but forgets to put a proper "/pre" tag.

When trying to quote such an entry where a "pre" tag is not closed properly, the regexp engine can go haywire (process stuck in gsub() forever, CPU usage 100%, if it happens with all threads of an existing instance, the whole Redmine instance becomes unusable until the process finishes/is terminated) with cases where a lot of whitespaces/tabs/newlines are present.

Upon stumbling on that bug, and killing the affected Rails processes, I got the following trace :

Completed 500 Internal Server Error in 7016997ms
SignalException (SIGTERM):
  app/controllers/journals_controller.rb:69:in `gsub'
  app/controllers/journals_controller.rb:69:in `new'

Which allowed me to narrow it down the code where "pre" blocks are replaced with "[...]".

Studying the ruby regular expression spec (http://www.tutorialspoint.com/ruby/ruby_regular_expressions.htm) it seems the following expression

%r{
((.|\s)*?)
}m
has redundant elements : a "dot" is supposed to match anything including newlines, when the regexp has the 'm' flag active, so there is no purpose in having a "or" match with whitespaces.

I tried the following fix on my side (basically, removing the redundant "or \s" check and sticking with "." and the 'm' flag) :


--- /usr/local/share/redmine/app/controllers/journals_controller.rb 2013-05-02 00:15:29.000000000 +0900
+++ /tmp/journals_controller.rb 2014-03-14 18:35:24.498448622 +0900
@@ -66,7 +66,7 @@
       text = @issue.description
     end
     # Replaces pre blocks with [...]
-    text = text.to_s.strip.gsub(%r{
((.|\s)*?)
}m, '[...]') + text = text.to_s.strip.gsub(%r{
(.*?)
}m, '[...]') @content = "#{ll(Setting.default_language, :text_user_wrote, user)}\n> " @content << text.gsub(/(\r?\n|\r\n?)/, "\n> ") + "\n\n" rescue ActiveRecord::RecordNotFound
and it seems to fix the issue. How to reproduce/check (it "works" in every case, as in, it WILL eventually end, as it is not an infinite loop, but the execution time varies greatly) : This works (obviously) :
$ time ruby -e 'print "
  lalala       \tlalala
".gsub(%r{
((.|\s)*?)
}m, '"'[...]'"')' [...] real 0m0.015s user 0m0.011s sys 0m0.004s $ time ruby -e 'print "
  lalala       \tlalala\n     lalala\r\n   lala\r    \n        lalalla \t\n
".gsub(%r{
((.|\s)*?)
}m, '"'[...]'"')' [...] real 0m0.018s user 0m0.014s sys 0m0.005s

I apologize in advance for the display break, but there is no way I can provide proper sample code for this problem otherwise... (I hope this does not trigger anything weird by writing it in the description, if this has not been fixed in latest versions...)

These take more and more time as you add more characters, rendering a whole running ruby process unusable :

# Starting with 20 characters, mixing spaces and alphabetic
$ time ruby -e 'print "
  lalala       ".gsub(%r{
((.|\s)*?)
}m, '"'[...]'"')'
  lalala       
real    0m0.069s
user    0m0.007s
sys 0m0.004s
# Adding 7 more characters (total : 27)
$ time ruby -e 'print "
  lalala       \tlalalal".gsub(%r{
((.|\s)*?)
}m, '"'[...]'"')'
  lalala           lalalal
real    0m0.018s
user    0m0.013s
sys 0m0.008s
# Adding 14 more characters (total : 41)
$ time ruby -e 'print "
  lalala        \tlalala\n     lalala\r\n".gsub(%r{
((.|\s)*?)
}m, '"'[...]'"')'
  lalala           lalala
     lalala

real    0m0.080s
user    0m0.080s
sys 0m0.001s
# Adding 3 more characters (total : 44)
$ time ruby -e 'print "
  lalala        \tlalala\n     lalala\r\n   ".gsub(%r{
((.|\s)*?)
}m, '"'[...]'"')'
  lalala           lalala
     lalala
   
real    0m0.349s
user    0m0.344s
sys 0m0.004s
# Adding 4 more characters (total : 48)
$ time ruby -e 'print "
  lalala        \tlalala\n     lalala\r\n   lala".gsub(%r{
((.|\s)*?)
}m, '"'[...]'"')'
  lalala           lalala
     lalala
   lala
real    0m1.010s
user    0m1.009s
sys 0m0.001s
# Adding 5 more characters (total : 53)
$ time ruby -e 'print "
  lalala       \tlalala\n     lalala\r\n   lala\r    ".gsub(%r{
((.|\s)*?)
}m, '"'[...]'"')'
  lalala           lalala
     lalala
    ala
real    0m5.213s
user    0m5.208s
sys 0m0.001s
# Adding 5 more characters (total : 58)
$ time ruby -e 'print "
  lalala       \tlalala\n     lalala\r\n   lala\r    \n    ".gsub(%r{
((.|\s)*?)
}m, '"'[...]'"')'
  lalala           lalala
     lalala
    ala
    
real    2m36.084s
user    2m36.021s
sys 0m0.028s

...And it only gets worse from there.

This works, and ends instantly :

$ time ruby -e 'print "
  lalala       \tlalala\n     lalala\r\n   lala\r    \n        lalalla \t\n".gsub(%r{
(.*?)
}m, '"'[...]'"')'
  lalala           lalala
     lalala
    ala
        lalalla     

real    0m0.014s
user    0m0.014s
sys 0m0.001s
$ time ruby -e 'print "
  lalala       \tlalala\n     lalala\r\n   lala\r    \n        lalalla \t\n
".gsub(%r{
(.*?)
}m, '"'[...]'"')' [...] real 0m0.018s user 0m0.014s sys 0m0.004s

journals

I guess you're using ruby1.8, right? ruby1.9 doesn't seem to suffer from this regexp issue. The regexp was flawed anyway and your patch is applied in r12969, thanks for pointing this out.
--------------------------------------------------------------------------------
Jean-Philippe Lang wrote:
> I guess you're using ruby1.8, right? ruby1.9 doesn't seem to suffer from this regexp issue. The regexp was flawed anyway and your patch is applied in r12969, thanks for pointing this out.

Thank you.

I confirmed my environment for the sake of exhaustivity, if this can help someone pinpoint issues :
- Ruby version : I do have 1.8 and 1.9 installed on this machine, but I can certify that Redmine uses 1.9 (as follows)
- Web server : Apache + mod_passenger

The configuration for mod_passenger on my Redmine server, and the used version of Ruby are as follows :
<pre>
$ cat /etc/apache2/mods-enabled/passenger.conf
<IfModule mod_passenger.c>
PassengerRoot /usr
PassengerRuby /usr/bin/ruby
</IfModule>
$ /usr/bin/ruby -v
ruby 1.9.3p194 (2012-04-20 revision 35410) [x86_64-linux]
$ /usr/bin/ruby1.9.1 -v
ruby 1.9.3p194 (2012-04-20 revision 35410) [x86_64-linux]
$ /usr/bin/ruby1.9.3 -v
ruby 1.9.3p194 (2012-04-20 revision 35410) [x86_64-linux]
$ ps auxw | grep Rack
www-data 11471 0.2 3.3 413872 136696 ? Sl 15:43 0:35 Rack: /usr/local/share/redmine
$ lsof -p 11471 | grep -E "ruby1|ruby-1"
ruby 11471 www-data txt REG 254,0 6336 22798 /usr/bin/ruby1.9.1
ruby 11471 www-data mem REG 254,0 2072592 24013 /usr/lib/libruby-1.9.1.so.1.9.1
</pre>

I also find it extremely odd that Debian's ruby packages install stuff this way, for what it's worth. I suspect in this instance that the problem got fixed in a further version of ruby 1.9, and that I am maybe using an older unfixed version.
--------------------------------------------------------------------------------
Confirming the same problem in app/controllers/messages_controller.rb at line 116 :
<pre><code class="diff">
--- app/controllers/messages_controller.rb 2014-02-08 17:19:32.000000000 +0900
+++ /tmp/messages_controller.rb 2014-03-17 11:07:46.070637759 +0900
@@ -113,7 +113,7 @@
@subject = "RE: #{@subject}" unless @subject.starts_with?('RE:')

@content = "#{ll(Setting.default_language, :text_user_wrote, @message.author)}\n> "
- @content << @message.content.to_s.strip.gsub(%r{<pre>((.|\s)*?)</pre>}m, '[...]').gsub(/(\r?\n|\r\n?)/, "\n> ") + "\n\n"
+ @content << @message.content.to_s.strip.gsub(%r{<pre>(.*?)</pre>}m, '[...]').gsub(/(\r?\n|\r\n?)/, "\n> ") + "\n\n"
end

def preview
</code></pre>
--------------------------------------------------------------------------------
Merged.
--------------------------------------------------------------------------------

Admin Redmine さんが約4年前に更新

  • カテゴリIssues_2 にセット
  • 対象バージョン2.5.1_82 にセット

他の形式にエクスポート: Atom PDF

いいね!0
いいね!0